vpn-manager: Add l2tp/ipsec vpn manager

vpn-manager: Add l2tp/ipsec vpn manager

Change-Id: I3d0e36bc8595b9038782364368ff2e7d77b39472

Related to flimflam review:
http://codereview.chromium.org/6513009/

BUG=chromium-os:11814
TEST=unit tests / run connect-vpn script with l2tpipsec type

Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=e92e668

[kmixter1, 2011-03-04 02:15:28]

Looking for some early feedback.  Plan to tweak some code but should
substantially stay the same.

[petkov, 2011-03-04 18:42:56]

While including all these changes into a single CL makes it easier to see the
bigger picture, it definitely makes it harder to do a good review from
functionality standpoint. I would have probably split this into 4 CLs -- basic
service class, the two services classes, integration.

http://codereview.chromium.org/6508016/diff/11001/LICENSE
File LICENSE (right):

http://codereview.chromium.org/6508016/diff/11001/LICENSE#newcode1
LICENSE:1: // Copyright (c) 2010 The Chromium OS Authors. All rights reserved.
2011?

http://codereview.chromium.org/6508016/diff/11001/bin/pluto_updown
File bin/pluto_updown (right):

http://codereview.chromium.org/6508016/diff/11001/bin/pluto_updown#newcode12
bin/pluto_updown:12: log "Received PLUTO_VERB=$PLUTO_VERB for pid
$IPSEC_MANAGER_PID"
use {} around variables? is there a good reason to pass arguments through
environment variables?

http://codereview.chromium.org/6508016/diff/11001/bin/pluto_updown#newcode15
bin/pluto_updown:15: touch /tmp/ipsec-up
is this 3-space indent?

http://codereview.chromium.org/6508016/diff/11001/ipsec_manager.cc
File ipsec_manager.cc (right):

http://codereview.chromium.org/6508016/diff/11001/ipsec_manager.cc#newcode42
ipsec_manager.cc:42: : ServiceManager("ipsec"), output_fd_(-1), ike_version_(0),
shouldn't the initializer be on separate lines?

http://codereview.chromium.org/6508016/diff/11001/ipsec_manager.cc#newcode59
ipsec_manager.cc:59: if (psk_file.empty() && server_ca_file.empty() &&
client_key_file.empty() &&
move this check under if psk_file.empty() block (just like you've done for the
else case)

http://codereview.chromium.org/6508016/diff/11001/ipsec_manager.cc#newcode67
ipsec_manager.cc:67: if (!file_util::PathExists(FilePath(server_ca_file))) {
server_ca_file could be empty here. maybe you want to change/split the
PSK/certificates check above?

http://codereview.chromium.org/6508016/diff/11001/ipsec_manager.cc#newcode123
ipsec_manager.cc:123: //TODO: use trim function
// TODO(kmixter) or fix the todo

http://codereview.chromium.org/6508016/diff/11001/ipsec_manager.cc#newcode137
ipsec_manager.cc:137: if (Process::ProcessExists(starter_->pid()))
you don't just starter_->Reset(0) to avoid ERROR logs from Process::Kill?

http://codereview.chromium.org/6508016/diff/11001/ipsec_manager.cc#newcode149
ipsec_manager.cc:149: starter_->AddArg(IPSEC_STARTER);
where IPSEC_STARTER declared?

http://codereview.chromium.org/6508016/diff/11001/ipsec_manager.cc#newcode268
ipsec_manager.cc:268: // TODO: run whack/stroke --rereadsecrets instead and use
TODO(kmixter)

http://codereview.chromium.org/6508016/diff/11001/ipsec_manager.cc#newcode292
ipsec_manager.cc:292: if (!starter_->Kill(SIGTERM, kTermTimeout)) {
this SIGTERM->SIGKILL sequence seems like a good utility for Process

http://codereview.chromium.org/6508016/diff/11001/ipsec_manager.h
File ipsec_manager.h (right):

http://codereview.chromium.org/6508016/diff/11001/ipsec_manager.h#newcode12
ipsec_manager.h:12: #include "vpn-manager/service_manager.h"
add a blank line before this project headers?

http://codereview.chromium.org/6508016/diff/11001/ipsec_manager.h#newcode23
ipsec_manager.h:23: bool Initialize(int ike_version,
seems evil -- you're overloading the method name for a static method...

http://codereview.chromium.org/6508016/diff/11001/ipsec_manager.h#newcode30
ipsec_manager.h:30: bool Start();
add virtual keyword for all inherited virtual methods

http://codereview.chromium.org/6508016/diff/11001/ipsec_manager.h#newcode33
ipsec_manager.h:33: int Poll();
docstrings (throughout)

http://codereview.chromium.org/6508016/diff/11001/ipsec_manager.h#newcode49
ipsec_manager.h:49: bool StartStarter();
StartStarter?

http://codereview.chromium.org/6508016/diff/11001/l2tp_manager.cc
File l2tp_manager.cc (right):

http://codereview.chromium.org/6508016/diff/11001/l2tp_manager.cc#newcode30
l2tp_manager.cc:30: // TODO: enable PPPoL2TP or at least measure efficiency gap.
TODO(kmixter)

http://codereview.chromium.org/6508016/diff/11001/l2tp_manager.cc#newcode32
l2tp_manager.cc:32: L2tpManager::L2tpManager() : ServiceManager("l2tp"),
output_fd_(-1),
initializers on separate lines?

http://codereview.chromium.org/6508016/diff/11001/l2tp_manager.cc#newcode150
l2tp_manager.cc:150: // TODO: better synchronization - maybe wait for file to
appear.
TODO(kmixter)

http://codereview.chromium.org/6508016/diff/11001/l2tp_manager.cc#newcode162
l2tp_manager.cc:162: if ((base::TimeTicks::Now() - start_ticks_).InSecondsF() >
why InSecondsF rather than InSeconds? also, I've usually seen this implemented
through comparing TimeDeltas --

if (diff > TimeDelta::FromSeconds(FLAGS_authentication_timeout))

http://codereview.chromium.org/6508016/diff/11001/l2tp_manager.cc#newcode167
l2tp_manager.cc:167: // Return 1 in order to get check exit conditions.
return 1000?

http://codereview.chromium.org/6508016/diff/11001/l2tp_manager.h
File l2tp_manager.h (right):

http://codereview.chromium.org/6508016/diff/11001/l2tp_manager.h#newcode12
l2tp_manager.h:12: #include "vpn-manager/service_manager.h"
add blank line

http://codereview.chromium.org/6508016/diff/11001/l2tp_manager.h#newcode23
l2tp_manager.h:23: bool Initialize(const std::string& remote);
overloading (of static)

http://codereview.chromium.org/6508016/diff/11001/l2tp_manager.h#newcode25
l2tp_manager.h:25: bool Start();
virtual for inherited methods

http://codereview.chromium.org/6508016/diff/11001/l2tp_manager_test.cc
File l2tp_manager_test.cc (right):

http://codereview.chromium.org/6508016/diff/11001/l2tp_manager_test.cc#newcode9
l2tp_manager_test.cc:9: #include "vpn-manager/l2tp_manager.h"
add blank line

http://codereview.chromium.org/6508016/diff/11001/l2tpipsec_vpn.cc
File l2tpipsec_vpn.cc (right):

http://codereview.chromium.org/6508016/diff/11001/l2tpipsec_vpn.cc#newcode29
l2tpipsec_vpn.cc:29: bool s_terminate_request = false;
static?

http://codereview.chromium.org/6508016/diff/11001/l2tpipsec_vpn.cc#newcode87
l2tpipsec_vpn.cc:87: // Event loop.  Events are:
move this event loop into a separate function

http://codereview.chromium.org/6508016/diff/11001/l2tpipsec_vpn.cc#newcode107
l2tpipsec_vpn.cc:107: pid_t pid = waitpid(-1, &status, WNOHANG);
add a comment what this does...

http://codereview.chromium.org/6508016/diff/11001/l2tpipsec_vpn.cc#newcode119
l2tpipsec_vpn.cc:119: 
remove blank line

http://codereview.chromium.org/6508016/diff/11001/service_manager.cc
File service_manager.cc (right):

http://codereview.chromium.org/6508016/diff/11001/service_manager.cc#newcode19
service_manager.cc:19: : is_running_(false), was_stopped_(false),
inner_service_(NULL),
each initializer should be on a separate line, no?

http://codereview.chromium.org/6508016/diff/11001/service_manager.cc#newcode27
service_manager.cc:27: is_running_ = true;
CHECK(!is_running_ && !was_stopped)?

http://codereview.chromium.org/6508016/diff/11001/service_manager.cc#newcode32
service_manager.cc:32: if (!inner_service_->Start()) {
Hmm, so you always start the chain from the outermost service and stop be the
chain from the innermost service? It seems that you need checks and docstrings
that this is the intended mechanism.

http://codereview.chromium.org/6508016/diff/11001/service_manager.cc#newcode41
service_manager.cc:41: is_running_ = false;
CHECK(is_running && !was_stopped)?

http://codereview.chromium.org/6508016/diff/11001/service_manager.cc#newcode52
service_manager.cc:52: FilePath base = temp_path_.DirName();
why do you split it into base and the rest? CreateDirectory will create all
parents for you, if necessary, right?

http://codereview.chromium.org/6508016/diff/11001/service_manager.cc#newcode71
service_manager.cc:71: // data are going to split into different syslog lines. 
Seems ok
how is this ensured, by multiple calls to this method? don't you need a while
loop or something?

http://codereview.chromium.org/6508016/diff/11001/service_manager.h
File service_manager.h (right):

http://codereview.chromium.org/6508016/diff/11001/service_manager.h#newcode18
service_manager.h:18: // Initialize ServiceManager.
always succeeds?

http://codereview.chromium.org/6508016/diff/11001/service_manager.h#newcode58
service_manager.h:58: static void WriteFdToSyslog(int fd, const char* prefix);
what does this do?

http://codereview.chromium.org/6508016/diff/11001/service_manager.h#newcode86
service_manager.h:86: static FilePath temp_path_;
static variables of class type are forbidden

http://codereview.chromium.org/6508016/diff/11001/service_manager_test.cc
File service_manager_test.cc (right):

http://codereview.chromium.org/6508016/diff/11001/service_manager_test.cc#new...
service_manager_test.cc:11: #include "vpn-manager/service_manager.h"
add a blank line before

[kmixter1, 2011-03-05 02:48:59]

PTAL, a bunch of improvements.  I think I've addressed all comments except for
the one saying "stay tuned".  Will have that Monday hopefully.

http://codereview.chromium.org/6508016/diff/11001/LICENSE
File LICENSE (right):

http://codereview.chromium.org/6508016/diff/11001/LICENSE#newcode1
LICENSE:1: // Copyright (c) 2010 The Chromium OS Authors. All rights reserved.
On 2011/03/04 18:42:56, petkov wrote:
> 2011?

Done.

http://codereview.chromium.org/6508016/diff/11001/bin/pluto_updown
File bin/pluto_updown (right):

http://codereview.chromium.org/6508016/diff/11001/bin/pluto_updown#newcode12
bin/pluto_updown:12: log "Received PLUTO_VERB=$PLUTO_VERB for pid
$IPSEC_MANAGER_PID"
On 2011/03/04 18:42:56, petkov wrote:
> use {} around variables? is there a good reason to pass arguments through
> environment variables?

Originally I was going to use SIGUSR1 to notify IPSEC_MANAGER_PID when this
layer is up, but the daemon runs as a different user, so that wouldn't work. 
Removing the setting of PID since it's no longer used.

http://codereview.chromium.org/6508016/diff/11001/bin/pluto_updown#newcode15
bin/pluto_updown:15: touch /tmp/ipsec-up
On 2011/03/04 18:42:56, petkov wrote:
> is this 3-space indent?

Done.

http://codereview.chromium.org/6508016/diff/11001/ipsec_manager.cc
File ipsec_manager.cc (right):

http://codereview.chromium.org/6508016/diff/11001/ipsec_manager.cc#newcode42
ipsec_manager.cc:42: : ServiceManager("ipsec"), output_fd_(-1), ike_version_(0),
On 2011/03/04 18:42:56, petkov wrote:
> shouldn't the initializer be on separate lines?
Oops, you're right. Done.

http://codereview.chromium.org/6508016/diff/11001/ipsec_manager.cc#newcode59
ipsec_manager.cc:59: if (psk_file.empty() && server_ca_file.empty() &&
client_key_file.empty() &&
On 2011/03/04 18:42:56, petkov wrote:
> move this check under if psk_file.empty() block (just like you've done for the
> else case)

Done.

http://codereview.chromium.org/6508016/diff/11001/ipsec_manager.cc#newcode67
ipsec_manager.cc:67: if (!file_util::PathExists(FilePath(server_ca_file))) {
On 2011/03/04 18:42:56, petkov wrote:
> server_ca_file could be empty here. maybe you want to change/split the
> PSK/certificates check above?

If everything's empty, user will get a message that one needs to be set.  If any
one of the cert-path files is set, then the empty ones will be indicated by
saying the file '' isn't valid.  Seems ok.

http://codereview.chromium.org/6508016/diff/11001/ipsec_manager.cc#newcode123
ipsec_manager.cc:123: //TODO: use trim function
On 2011/03/04 18:42:56, petkov wrote:
> // TODO(kmixter) or fix the todo

Done.

http://codereview.chromium.org/6508016/diff/11001/ipsec_manager.cc#newcode137
ipsec_manager.cc:137: if (Process::ProcessExists(starter_->pid()))
On 2011/03/04 18:42:56, petkov wrote:
> you don't just starter_->Reset(0) to avoid ERROR logs from Process::Kill?

Yes. And those ERROR logs are useful since they indicate something has died
without being anticipated/noticed.

http://codereview.chromium.org/6508016/diff/11001/ipsec_manager.cc#newcode149
ipsec_manager.cc:149: starter_->AddArg(IPSEC_STARTER);
On 2011/03/04 18:42:56, petkov wrote:
> where IPSEC_STARTER declared?

Makefile

http://codereview.chromium.org/6508016/diff/11001/ipsec_manager.cc#newcode268
ipsec_manager.cc:268: // TODO: run whack/stroke --rereadsecrets instead and use
On 2011/03/04 18:42:56, petkov wrote:
> TODO(kmixter)

Done.

http://codereview.chromium.org/6508016/diff/11001/ipsec_manager.cc#newcode292
ipsec_manager.cc:292: if (!starter_->Kill(SIGTERM, kTermTimeout)) {
On 2011/03/04 18:42:56, petkov wrote:
> this SIGTERM->SIGKILL sequence seems like a good utility for Process

I agree, but it's complicated.  If the process is a child process, then this
sequence makes sense. If it's not, then the Kill will always succeed but the
process will not necessarily die (because it's impossible to tell with certainty
in POSIX unless the parent/child relationship exists).

http://codereview.chromium.org/6508016/diff/11001/ipsec_manager.h
File ipsec_manager.h (right):

http://codereview.chromium.org/6508016/diff/11001/ipsec_manager.h#newcode12
ipsec_manager.h:12: #include "vpn-manager/service_manager.h"
On 2011/03/04 18:42:56, petkov wrote:
> add a blank line before this project headers?

Done.

http://codereview.chromium.org/6508016/diff/11001/ipsec_manager.h#newcode23
ipsec_manager.h:23: bool Initialize(int ike_version,
On 2011/03/04 18:42:56, petkov wrote:
> seems evil -- you're overloading the method name for a static method...
Yikes, I was once told not to be, so fixing this with expediency.  I didn't
realize they both had the same name.

http://codereview.chromium.org/6508016/diff/11001/ipsec_manager.h#newcode30
ipsec_manager.h:30: bool Start();
On 2011/03/04 18:42:56, petkov wrote:
> add virtual keyword for all inherited virtual methods

Is this style guide thing?  I'll add them assuming it makes it easier to read,
but they're not necessary and less optimal.

http://codereview.chromium.org/6508016/diff/11001/ipsec_manager.h#newcode33
ipsec_manager.h:33: int Poll();
On 2011/03/04 18:42:56, petkov wrote:
> docstrings (throughout)

Done.

http://codereview.chromium.org/6508016/diff/11001/ipsec_manager.h#newcode49
ipsec_manager.h:49: bool StartStarter();
On 2011/03/04 18:42:56, petkov wrote:
> StartStarter?

Yeah, the ipsec executable we start is called "starter".

http://codereview.chromium.org/6508016/diff/11001/l2tp_manager.cc
File l2tp_manager.cc (right):

http://codereview.chromium.org/6508016/diff/11001/l2tp_manager.cc#newcode30
l2tp_manager.cc:30: // TODO: enable PPPoL2TP or at least measure efficiency gap.
On 2011/03/04 18:42:56, petkov wrote:
> TODO(kmixter)

Removed here and added crosbug.com/12769

http://codereview.chromium.org/6508016/diff/11001/l2tp_manager.cc#newcode32
l2tp_manager.cc:32: L2tpManager::L2tpManager() : ServiceManager("l2tp"),
output_fd_(-1),
On 2011/03/04 18:42:56, petkov wrote:
> initializers on separate lines?

Done.

http://codereview.chromium.org/6508016/diff/11001/l2tp_manager.cc#newcode150
l2tp_manager.cc:150: // TODO: better synchronization - maybe wait for file to
appear.
On 2011/03/04 18:42:56, petkov wrote:
> TODO(kmixter)

Done.

http://codereview.chromium.org/6508016/diff/11001/l2tp_manager.cc#newcode162
l2tp_manager.cc:162: if ((base::TimeTicks::Now() - start_ticks_).InSecondsF() >
On 2011/03/04 18:42:56, petkov wrote:
> why InSecondsF rather than InSeconds? also, I've usually seen this implemented
> through comparing TimeDeltas --
> 
> if (diff > TimeDelta::FromSeconds(FLAGS_authentication_timeout))
> 

Not entirely sure why that form is better, but changed.

http://codereview.chromium.org/6508016/diff/11001/l2tp_manager.cc#newcode167
l2tp_manager.cc:167: // Return 1 in order to get check exit conditions.
On 2011/03/04 18:42:56, petkov wrote:
> return 1000?

Done.

http://codereview.chromium.org/6508016/diff/11001/l2tp_manager.h
File l2tp_manager.h (right):

http://codereview.chromium.org/6508016/diff/11001/l2tp_manager.h#newcode12
l2tp_manager.h:12: #include "vpn-manager/service_manager.h"
On 2011/03/04 18:42:56, petkov wrote:
> add blank line

Done.

http://codereview.chromium.org/6508016/diff/11001/l2tp_manager.h#newcode23
l2tp_manager.h:23: bool Initialize(const std::string& remote);
On 2011/03/04 18:42:56, petkov wrote:
> overloading (of static)

Done.

http://codereview.chromium.org/6508016/diff/11001/l2tp_manager.h#newcode25
l2tp_manager.h:25: bool Start();
On 2011/03/04 18:42:56, petkov wrote:
> virtual for inherited methods

Done.

http://codereview.chromium.org/6508016/diff/11001/l2tp_manager_test.cc
File l2tp_manager_test.cc (right):

http://codereview.chromium.org/6508016/diff/11001/l2tp_manager_test.cc#newcode9
l2tp_manager_test.cc:9: #include "vpn-manager/l2tp_manager.h"
On 2011/03/04 18:42:56, petkov wrote:
> add blank line

Done.

http://codereview.chromium.org/6508016/diff/11001/l2tpipsec_vpn.cc
File l2tpipsec_vpn.cc (right):

http://codereview.chromium.org/6508016/diff/11001/l2tpipsec_vpn.cc#newcode29
l2tpipsec_vpn.cc:29: bool s_terminate_request = false;
On 2011/03/04 18:42:56, petkov wrote:
> static?

Done.

http://codereview.chromium.org/6508016/diff/11001/l2tpipsec_vpn.cc#newcode87
l2tpipsec_vpn.cc:87: // Event loop.  Events are:
On 2011/03/04 18:42:56, petkov wrote:
> move this event loop into a separate function

Done.

http://codereview.chromium.org/6508016/diff/11001/l2tpipsec_vpn.cc#newcode107
l2tpipsec_vpn.cc:107: pid_t pid = waitpid(-1, &status, WNOHANG);
On 2011/03/04 18:42:56, petkov wrote:
> add a comment what this does...

Done.

http://codereview.chromium.org/6508016/diff/11001/l2tpipsec_vpn.cc#newcode119
l2tpipsec_vpn.cc:119: 
On 2011/03/04 18:42:56, petkov wrote:
> remove blank line

Done.

http://codereview.chromium.org/6508016/diff/11001/service_manager.cc
File service_manager.cc (right):

http://codereview.chromium.org/6508016/diff/11001/service_manager.cc#newcode19
service_manager.cc:19: : is_running_(false), was_stopped_(false),
inner_service_(NULL),
On 2011/03/04 18:42:56, petkov wrote:
> each initializer should be on a separate line, no?

Done.

http://codereview.chromium.org/6508016/diff/11001/service_manager.cc#newcode27
service_manager.cc:27: is_running_ = true;
On 2011/03/04 18:42:56, petkov wrote:
> CHECK(!is_running_ && !was_stopped)?

Done.

http://codereview.chromium.org/6508016/diff/11001/service_manager.cc#newcode32
service_manager.cc:32: if (!inner_service_->Start()) {
On 2011/03/04 18:42:56, petkov wrote:
> Hmm, so you always start the chain from the outermost service and stop be the
> chain from the innermost service? It seems that you need checks and docstrings
> that this is the intended mechanism.

Yes - start from the outermost and stop from the innermost that's already
started.  Added docstrings and extra checks.

http://codereview.chromium.org/6508016/diff/11001/service_manager.cc#newcode41
service_manager.cc:41: is_running_ = false;
On 2011/03/04 18:42:56, petkov wrote:
> CHECK(is_running && !was_stopped)?

It's ok to stop an already stopped service, see new docstring.

http://codereview.chromium.org/6508016/diff/11001/service_manager.cc#newcode52
service_manager.cc:52: FilePath base = temp_path_.DirName();
On 2011/03/04 18:42:56, petkov wrote:
> why do you split it into base and the rest? CreateDirectory will create all
> parents for you, if necessary, right?

No kidding?  I didn't know it was so kind.  See simpler code.

http://codereview.chromium.org/6508016/diff/11001/service_manager.cc#newcode71
service_manager.cc:71: // data are going to split into different syslog lines. 
Seems ok
On 2011/03/04 18:42:56, petkov wrote:
> how is this ensured, by multiple calls to this method? don't you need a while
> loop or something?

Right now the code does arbitrarily split lines.  I didn't think it would happen
that often which is why I said it was ok for now.  It actually does happen quite
often and I plan to fix this before submitting... stay tuned.

http://codereview.chromium.org/6508016/diff/11001/service_manager.h
File service_manager.h (right):

http://codereview.chromium.org/6508016/diff/11001/service_manager.h#newcode18
service_manager.h:18: // Initialize ServiceManager.
On 2011/03/04 18:42:56, petkov wrote:
> always succeeds?

No, but if it returns it has.

http://codereview.chromium.org/6508016/diff/11001/service_manager.h#newcode58
service_manager.h:58: static void WriteFdToSyslog(int fd, const char* prefix);
On 2011/03/04 18:42:56, petkov wrote:
> what does this do?

Done.

http://codereview.chromium.org/6508016/diff/11001/service_manager.h#newcode86
service_manager.h:86: static FilePath temp_path_;
On 2011/03/04 18:42:56, petkov wrote:
> static variables of class type are forbidden

Replaced with point to FilePath which is now cleaned up by DeleteTemp.  Picked
that instead of pointing to string constants because it made the tests cleaner
to write.

[Will Drewry, 2011-03-05 04:06:38]

Thanks for adding me.  I took a first pass and thought I'd go ahead and send
over the comments.  I haven't read for comprehension, just getting it in my
head, and called out some style nits.

This framework looks to be coming along nicely! I hadn't realized it was this
far along!

will

http://codereview.chromium.org/6508016/diff/11004/bin/pluto_updown
File bin/pluto_updown (right):

http://codereview.chromium.org/6508016/diff/11004/bin/pluto_updown#newcode6
bin/pluto_updown:6: touch /tmp/ipsec-up
Can we put this in a /var/run/ipsec/up directory with permissions that make
sense?  Since no data is written (and Delete() just calls unlink(2)), it's not a
huge deal, but it be more assuring to know any user on the system with /tmp
access can't confuse the manager.

http://codereview.chromium.org/6508016/diff/11004/ipsec_manager.cc
File ipsec_manager.cc (right):

http://codereview.chromium.org/6508016/diff/11004/ipsec_manager.cc#newcode15
ipsec_manager.cc:15: #include "base/file_util.h"
nit: use <> unless the files are part of your package,
(applies to all the files in the cl)

http://codereview.chromium.org/6508016/diff/11004/ipsec_manager.cc#newcode19
ipsec_manager.cc:19: #include "gflags/gflags.h"
I like gflags myself, but I thought we were trying to stick with chrome's
commandline flags model instead?

http://codereview.chromium.org/6508016/diff/11004/ipsec_manager.cc#newcode30
ipsec_manager.cc:30: const char kStarterPidFile[] = "/var/run/starter.pid";
Any reason not to use a subdirectory to clean up the namespace and potentially
make access more clear?

http://codereview.chromium.org/6508016/diff/11004/ipsec_manager.cc#newcode32
ipsec_manager.cc:32: const char kIpsecUpFile[] = "/tmp/ipsec-up";
note for myself: trace through and see where this is used.

http://codereview.chromium.org/6508016/diff/11004/ipsec_manager.cc#newcode43
ipsec_manager.cc:43: : ServiceManager("ipsec"),
optional style nit: kServiceName ;)

http://codereview.chromium.org/6508016/diff/11004/ipsec_manager.cc#newcode113
ipsec_manager.cc:113: file_util::Delete(FilePath(kIpsecUpFile), false);
Is this being created by the ipsec daemon or somewhere else in the code?  I
always get a little concerned when I see a fixed path in /tmp being directly
accessed. :)

http://codereview.chromium.org/6508016/diff/11004/l2tp_manager.cc
File l2tp_manager.cc (right):

http://codereview.chromium.org/6508016/diff/11004/l2tp_manager.cc#newcode26
l2tp_manager.cc:26: const char kPppInterfacePath[] = "/sys/class/net/ppp0";
nice!

http://codereview.chromium.org/6508016/diff/11004/l2tp_manager.cc#newcode104
l2tp_manager.cc:104: FLAGS_password.c_str()));
Is the password being passed in on the commandline or is the FLAGS value being
set in another way?  (It'll leave the password in "ps" if so :/ )  Feel free to
tell me to read the code more closely if I've skimmed over it.

Alternatively, does this just make testing 10,000 times easier? If so, might be
worth some LOG(WARNING)s when a password is passed in on the cmdline.

http://codereview.chromium.org/6508016/diff/11004/l2tpipsec_vpn.cc
File l2tpipsec_vpn.cc (right):

http://codereview.chromium.org/6508016/diff/11004/l2tpipsec_vpn.cc#newcode62
l2tpipsec_vpn.cc:62: static void RunEventLoop(IpsecManager* ipsec, L2tpManager*
l2tp) {
not necessarily important now, but it seems like this would be useful to
abstract a bit and include even just as a static ServiceManager function such
that you give it the outer service, it gets the inner from the outet, and then
does the poll and process loop.

http://codereview.chromium.org/6508016/diff/11004/l2tpipsec_vpn.cc#newcode77
l2tpipsec_vpn.cc:77: CHECK(poll_result >= 0 || errno == EINTR) << "Unexpected
poll result";
Do you want this to hard-terminate or should it try to call Stop on the
services?

http://codereview.chromium.org/6508016/diff/11004/service_manager.cc
File service_manager.cc (right):

http://codereview.chromium.org/6508016/diff/11004/service_manager.cc#newcode56
service_manager.cc:56: temp_path_ = new FilePath(kTempDirectory);
Would it make sense to do the converse:

if (temp_path_)
  return;

This isn't idempotent if called multiple times since the atexit handler will be
appended.

http://codereview.chromium.org/6508016/diff/11004/service_manager.cc#newcode57
service_manager.cc:57: CHECK(file_util::CreateDirectory(*temp_path_))
Any reason not to use a ScopedTempDir()?

If not, seems like a scoped_ptr wouldn't go amiss as it simplifies DeleteTemp.

http://codereview.chromium.org/6508016/diff/11004/service_manager.cc#newcode75
service_manager.cc:75: int written = HANDLE_EINTR(read(fd, &buffer,
sizeof(buffer) - 1));
nit: read returns a ssize_t which can cause problems if we compile 64-bit.

http://codereview.chromium.org/6508016/diff/11004/service_manager.cc#newcode85
service_manager.cc:85: for (size_t i = 0; i < lines.size(); ++i)
style side note: seems like a vector iterator is more appropriate

http://codereview.chromium.org/6508016/diff/11004/service_manager.h
File service_manager.h (right):

http://codereview.chromium.org/6508016/diff/11004/service_manager.h#newcode68
service_manager.h:68: static void SetLayerOrder(ServiceManager* outer,
So is this currently limited to two managers for a given service?

(I'd almost prefer two lines versus a static func. E.g., 
  ipsec->Requires(l2tp);
  l2tp->Helps(ipsec);
Especially since it could let you arbitrarily layer in the future.

Ignoring that, I like the violation of encapsulation for consistency, but it
could still cause weird if someone assumes this supports an arbitrary number of
services and tries to SetLayerOrder beyond 2.  But that really doesn't matter
now I suspect.)

http://codereview.chromium.org/6508016/diff/11004/service_manager.h#newcode103
service_manager.h:103: ServiceManager* outer_service_;
nit: Is there any reason these are protected instead of private with public or
protected accessors?   (applies to the other files in the cl too)

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Inheri...

[James Simonsen, 2011-03-07 20:32:35]

http://codereview.chromium.org/6508016/diff/16004/LICENSE
File LICENSE (right):

http://codereview.chromium.org/6508016/diff/16004/LICENSE#newcode1
LICENSE:1: // Copyright (c) 2011 The Chromium OS Authors. All rights reserved.
The comments seem redundant, since this is a text file.

http://codereview.chromium.org/6508016/diff/16004/ipsec_manager.cc
File ipsec_manager.cc (right):

http://codereview.chromium.org/6508016/diff/16004/ipsec_manager.cc#newcode23
ipsec_manager.cc:23: DEFINE_string(local, "", "local IP address (needed for
PSK)");
local_address would help readability.

http://codereview.chromium.org/6508016/diff/16004/ipsec_manager.cc#newcode27
ipsec_manager.cc:27: DEFINE_string(rightprotoport, "17/1701", "server
protocol/port");
Why is this one a command line flag? I don't know if anyone actually does it,
but it seems like someone could configure their server to run on another port.
It seems like it belongs in the same place we define remote address, etc.

http://codereview.chromium.org/6508016/diff/16004/ipsec_manager.cc#newcode59
ipsec_manager.cc:59: if (remote.empty()) {
Remote isn't clear to me. Is it remote server address?

http://codereview.chromium.org/6508016/diff/16004/ipsec_manager.cc#newcode136
ipsec_manager.cc:136: void IpsecManager::KillCurrentlyRunning() {
Do you know if just killing the process is safe when unloading everything? I'm
worried it'll not clean up nicely and leave its routes or DNS servers installed.
I usually do ipsec down and ipsec stop.

http://codereview.chromium.org/6508016/diff/16004/ipsec_manager.cc#newcode198
ipsec_manager.cc:198: AppendStringSetting(&config, "auto", "start");
We should probably have rightca too, if we're given a CA file.

http://codereview.chromium.org/6508016/diff/16004/ipsec_manager.cc#newcode209
ipsec_manager.cc:209: // reading secrets.
FYI, this affects secrets in the TPM too. It fails with a useless "Invalid PIN"
message.

http://codereview.chromium.org/6508016/diff/16004/ipsec_manager.cc#newcode227
ipsec_manager.cc:227: LOG(FATAL) << "Certificate mode not yet implemented";
I can send you my configs if you'd like to add this.

http://codereview.chromium.org/6508016/diff/16004/ipsec_manager.h
File ipsec_manager.h (right):

http://codereview.chromium.org/6508016/diff/16004/ipsec_manager.h#newcode38
ipsec_manager.h:38: const std::string& client_cert_file);
What's the plan for these once they're in the TPM? Just use the ID?

http://codereview.chromium.org/6508016/diff/16004/l2tp_manager.cc
File l2tp_manager.cc (right):

http://codereview.chromium.org/6508016/diff/16004/l2tp_manager.cc#newcode21
l2tp_manager.cc:21: DEFINE_string(user, "", "user name");
Why aren't these passed in through Initialize?

[kmixter1, 2011-03-11 01:34:27]

PTAL - Will I addressed all comments.

http://codereview.chromium.org/6508016/diff/11004/bin/pluto_updown
File bin/pluto_updown (right):

http://codereview.chromium.org/6508016/diff/11004/bin/pluto_updown#newcode6
bin/pluto_updown:6: touch /tmp/ipsec-up
On 2011/03/05 04:06:39, Will Drewry wrote:
> Can we put this in a /var/run/ipsec/up directory with permissions that make
> sense?  Since no data is written (and Delete() just calls unlink(2)), it's not
a
> huge deal, but it be more assuring to know any user on the system with /tmp
> access can't confuse the manager.

Done.

http://codereview.chromium.org/6508016/diff/11004/ipsec_manager.cc
File ipsec_manager.cc (right):

http://codereview.chromium.org/6508016/diff/11004/ipsec_manager.cc#newcode15
ipsec_manager.cc:15: #include "base/file_util.h"
On 2011/03/05 04:06:39, Will Drewry wrote:
> nit: use <> unless the files are part of your package,
> (applies to all the files in the cl)

There's some disagreement about this in the chrome os style regarding if files
part of chrome/chromeos are quoted or angle bracketed:
crash-reporter (granted, mine) - quotes
cromo - quotes
libcros - quotes
cryptohome - brackets
entd - quotes
login manager - brackets
metrics - brackets
minijail - brackets
power manager - quotes
tpm init - brackets
update engine - brackets
verity - brackets
window manager - quotes

http://codereview.chromium.org/6508016/diff/11004/ipsec_manager.cc#newcode19
ipsec_manager.cc:19: #include "gflags/gflags.h"
On 2011/03/05 04:06:39, Will Drewry wrote:
> I like gflags myself, but I thought we were trying to stick with chrome's
> commandline flags model instead?

Same thing - crash-reporter, cromo, metrics, monitor_reconfig, power manager,
and window manager all use gflags.  Almost the same set.

http://codereview.chromium.org/6508016/diff/11004/ipsec_manager.cc#newcode30
ipsec_manager.cc:30: const char kStarterPidFile[] = "/var/run/starter.pid";
On 2011/03/05 04:06:39, Will Drewry wrote:
> Any reason not to use a subdirectory to clean up the namespace and potentially
> make access more clear?

This is written as part of strongswan, I'm just reading this file.

http://codereview.chromium.org/6508016/diff/11004/ipsec_manager.cc#newcode32
ipsec_manager.cc:32: const char kIpsecUpFile[] = "/tmp/ipsec-up";
On 2011/03/05 04:06:39, Will Drewry wrote:
> note for myself: trace through and see where this is used.

Changed to /var/run/ipsec/up.

http://codereview.chromium.org/6508016/diff/11004/ipsec_manager.cc#newcode43
ipsec_manager.cc:43: : ServiceManager("ipsec"),
On 2011/03/05 04:06:39, Will Drewry wrote:
> optional style nit: kServiceName ;)

Done.

http://codereview.chromium.org/6508016/diff/11004/ipsec_manager.cc#newcode113
ipsec_manager.cc:113: file_util::Delete(FilePath(kIpsecUpFile), false);
On 2011/03/05 04:06:39, Will Drewry wrote:
> Is this being created by the ipsec daemon or somewhere else in the code?  I
> always get a little concerned when I see a fixed path in /tmp being directly
> accessed. :)

In the ipdown script that we install - and now it's no longer in tmp. :)

http://codereview.chromium.org/6508016/diff/11004/l2tp_manager.cc
File l2tp_manager.cc (right):

http://codereview.chromium.org/6508016/diff/11004/l2tp_manager.cc#newcode26
l2tp_manager.cc:26: const char kPppInterfacePath[] = "/sys/class/net/ppp0";
On 2011/03/05 04:06:39, Will Drewry wrote:
> nice!

Done.

http://codereview.chromium.org/6508016/diff/11004/l2tp_manager.cc#newcode104
l2tp_manager.cc:104: FLAGS_password.c_str()));
On 2011/03/05 04:06:39, Will Drewry wrote:
> Is the password being passed in on the commandline or is the FLAGS value being
> set in another way?  (It'll leave the password in "ps" if so :/ )  Feel free
to
> tell me to read the code more closely if I've skimmed over it.
> 
> Alternatively, does this just make testing 10,000 times easier? If so, might
be
> worth some LOG(WARNING)s when a password is passed in on the cmdline.

Exactly - it's for testing only.  Warning added.  For reals flimflam passes a
pppd plugin which asks back to flimflam over dbus for the password (which may
eventually then end up asking the user).

http://codereview.chromium.org/6508016/diff/11004/l2tpipsec_vpn.cc
File l2tpipsec_vpn.cc (right):

http://codereview.chromium.org/6508016/diff/11004/l2tpipsec_vpn.cc#newcode62
l2tpipsec_vpn.cc:62: static void RunEventLoop(IpsecManager* ipsec, L2tpManager*
l2tp) {
On 2011/03/05 04:06:39, Will Drewry wrote:
> not necessarily important now, but it seems like this would be useful to
> abstract a bit and include even just as a static ServiceManager function such
> that you give it the outer service, it gets the inner from the outet, and then
> does the poll and process loop.

Good point.  Yeah, I'm not sure how much this framework will apply to any other
VPNs, but I agree it if does this could be abstracted without much trouble -
though until then, this concrete version is a little easier to follow I think.

http://codereview.chromium.org/6508016/diff/11004/l2tpipsec_vpn.cc#newcode77
l2tpipsec_vpn.cc:77: CHECK(poll_result >= 0 || errno == EINTR) << "Unexpected
poll result";
On 2011/03/05 04:06:39, Will Drewry wrote:
> Do you want this to hard-terminate or should it try to call Stop on the
> services?

Good point again - changing to behave more gracefully.

http://codereview.chromium.org/6508016/diff/11004/service_manager.cc
File service_manager.cc (right):

http://codereview.chromium.org/6508016/diff/11004/service_manager.cc#newcode56
service_manager.cc:56: temp_path_ = new FilePath(kTempDirectory);
On 2011/03/05 04:06:39, Will Drewry wrote:
> Would it make sense to do the converse:
> 
> if (temp_path_)
>   return;
> 
> This isn't idempotent if called multiple times since the atexit handler will
be
> appended.

Done.

http://codereview.chromium.org/6508016/diff/11004/service_manager.cc#newcode57
service_manager.cc:57: CHECK(file_util::CreateDirectory(*temp_path_))
On 2011/03/05 04:06:39, Will Drewry wrote:
> Any reason not to use a ScopedTempDir()?
> 
> If not, seems like a scoped_ptr wouldn't go amiss as it simplifies DeleteTemp.

This pointer nastiness is related to Darin mentioning that we can't use file
scoped non-POD objects.  So I'm going to just pass this thing in from main and
get rid of manual deletion/atexit/DeleteTemp for the small price of holding onto
a reference to an object constructed elsewhere.

http://codereview.chromium.org/6508016/diff/11004/service_manager.cc#newcode75
service_manager.cc:75: int written = HANDLE_EINTR(read(fd, &buffer,
sizeof(buffer) - 1));
On 2011/03/05 04:06:39, Will Drewry wrote:
> nit: read returns a ssize_t which can cause problems if we compile 64-bit.

Thanks!

http://codereview.chromium.org/6508016/diff/11004/service_manager.cc#newcode85
service_manager.cc:85: for (size_t i = 0; i < lines.size(); ++i)
On 2011/03/05 04:06:39, Will Drewry wrote:
> style side note: seems like a vector iterator is more appropriate

I can see that, but I usually prefer to use C style loops for vectors because
they are easier to read and are as efficient.

http://codereview.chromium.org/6508016/diff/11004/service_manager.h
File service_manager.h (right):

http://codereview.chromium.org/6508016/diff/11004/service_manager.h#newcode68
service_manager.h:68: static void SetLayerOrder(ServiceManager* outer,
On 2011/03/05 04:06:39, Will Drewry wrote:
> So is this currently limited to two managers for a given service?
> 
> (I'd almost prefer two lines versus a static func. E.g., 
>   ipsec->Requires(l2tp);
>   l2tp->Helps(ipsec);

> Especially since it could let you arbitrarily layer in the future.
> 
> Ignoring that, I like the violation of encapsulation for consistency, but it
> could still cause weird if someone assumes this supports an arbitrary number
of
> services and tries to SetLayerOrder beyond 2.  But that really doesn't matter
> now I suspect.)

No, it's not limited to two.  This would work:
ServiceTurkey tuckey;
ServiceDuck duck;
ServiceChicken chicken;

ServiceManager::SetLayerOrder(&turkey, &duck);
ServiceManager::SetLayerOrder(&duck, &chicken);

It creates a linked list two elements at a time.

Added comments for clarification.

If this abstraction becomes generalizable I'd probably create a ServiceStack
class which takes push_back operators to avoid this kind of interface.

http://codereview.chromium.org/6508016/diff/11004/service_manager.h#newcode103
service_manager.h:103: ServiceManager* outer_service_;
On 2011/03/05 04:06:39, Will Drewry wrote:
> nit: Is there any reason these are protected instead of private with public or
> protected accessors?   (applies to the other files in the cl too)
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Inheri...

I don't think I've ever seen that one before.  Done.

[kmixter1, 2011-03-11 04:48:44]

PTAL - James - responded to your comments.

http://codereview.chromium.org/6508016/diff/16004/LICENSE
File LICENSE (right):

http://codereview.chromium.org/6508016/diff/16004/LICENSE#newcode1
LICENSE:1: // Copyright (c) 2011 The Chromium OS Authors. All rights reserved.
On 2011/03/07 20:32:36, James Simonsen wrote:
> The comments seem redundant, since this is a text file.

It's standard across all Chrome OS packages though.

http://codereview.chromium.org/6508016/diff/16004/ipsec_manager.cc
File ipsec_manager.cc (right):

http://codereview.chromium.org/6508016/diff/16004/ipsec_manager.cc#newcode23
ipsec_manager.cc:23: DEFINE_string(local, "", "local IP address (needed for
PSK)");
On 2011/03/07 20:32:36, James Simonsen wrote:
> local_address would help readability.

Done.

http://codereview.chromium.org/6508016/diff/16004/ipsec_manager.cc#newcode27
ipsec_manager.cc:27: DEFINE_string(rightprotoport, "17/1701", "server
protocol/port");
On 2011/03/07 20:32:36, James Simonsen wrote:
> Why is this one a command line flag? I don't know if anyone actually does it,
> but it seems like someone could configure their server to run on another port.
> It seems like it belongs in the same place we define remote address, etc.

Ideally every setting could be a command line flag so that flimflam can override
them (so that an admin can override them with enterprise policy).

http://codereview.chromium.org/6508016/diff/16004/ipsec_manager.cc#newcode59
ipsec_manager.cc:59: if (remote.empty()) {
On 2011/03/07 20:32:36, James Simonsen wrote:
> Remote isn't clear to me. Is it remote server address?

Done.

http://codereview.chromium.org/6508016/diff/16004/ipsec_manager.cc#newcode136
ipsec_manager.cc:136: void IpsecManager::KillCurrentlyRunning() {
On 2011/03/07 20:32:36, James Simonsen wrote:
> Do you know if just killing the process is safe when unloading everything? I'm
> worried it'll not clean up nicely and leave its routes or DNS servers
installed.
> I usually do ipsec down and ipsec stop.

I think it's safe.  I'm actually completely overriding the _updown script that
comes with strongswan with pluto_updown.  I believe _updown is doing nothing
because we're using the kernel builtin ipsec support (!KLIPS).

http://codereview.chromium.org/6508016/diff/16004/ipsec_manager.cc#newcode198
ipsec_manager.cc:198: AppendStringSetting(&config, "auto", "start");
On 2011/03/07 20:32:36, James Simonsen wrote:
> We should probably have rightca too, if we're given a CA file.

Same comment - better to hold off on all the ca stuff.

http://codereview.chromium.org/6508016/diff/16004/ipsec_manager.cc#newcode209
ipsec_manager.cc:209: // reading secrets.
On 2011/03/07 20:32:36, James Simonsen wrote:
> FYI, this affects secrets in the TPM too. It fails with a useless "Invalid
PIN"
> message.

Yuck.... I hope we don't have to keep pluto as root just for that, but it might
be the case.  Will - any thoughts?

http://codereview.chromium.org/6508016/diff/16004/ipsec_manager.cc#newcode227
ipsec_manager.cc:227: LOG(FATAL) << "Certificate mode not yet implemented";
On 2011/03/07 20:32:36, James Simonsen wrote:
> I can send you my configs if you'd like to add this.

I'd prefer that one of us does that in a separate CL just to not add any more to
this one since it's pretty big and blocking other work.

http://codereview.chromium.org/6508016/diff/16004/ipsec_manager.h
File ipsec_manager.h (right):

http://codereview.chromium.org/6508016/diff/16004/ipsec_manager.h#newcode38
ipsec_manager.h:38: const std::string& client_cert_file);
On 2011/03/07 20:32:36, James Simonsen wrote:
> What's the plan for these once they're in the TPM? Just use the ID?

Yeah - good point.  We'll not be passing these by filenames - probably we'll
throw these away entirely.  I can remove them now if you'd prefer.  We'll need
to pass the pkcs#11 module to load, the token #, pin, and id I guess.

http://codereview.chromium.org/6508016/diff/16004/l2tp_manager.cc
File l2tp_manager.cc (right):

http://codereview.chromium.org/6508016/diff/16004/l2tp_manager.cc#newcode21
l2tp_manager.cc:21: DEFINE_string(user, "", "user name");
On 2011/03/07 20:32:36, James Simonsen wrote:
> Why aren't these passed in through Initialize?

I decided to try to only pass in the options that are shared between layers
(remote_address for now) and to keep the others in their own files - I think it
makes --help a little more understandable as well.

[petkov, 2011-03-11 19:12:53]

A few more nits from my side, nothing critical so overall -- LGTM

http://codereview.chromium.org/6508016/diff/11001/ipsec_manager.h
File ipsec_manager.h (right):

http://codereview.chromium.org/6508016/diff/11001/ipsec_manager.h#newcode30
ipsec_manager.h:30: bool Start();
On 2011/03/05 02:48:59, kmixter1 wrote:
> On 2011/03/04 18:42:56, petkov wrote:
> > add virtual keyword for all inherited virtual methods
> 
> Is this style guide thing?  I'll add them assuming it makes it easier to read,
> but they're not necessary and less optimal.

yes, style says so.

http://codereview.chromium.org/6508016/diff/11004/ipsec_manager.cc
File ipsec_manager.cc (right):

http://codereview.chromium.org/6508016/diff/11004/ipsec_manager.cc#newcode15
ipsec_manager.cc:15: #include "base/file_util.h"
FWIW, I agree with Will on this one. Although style isn't quite clear about
this, there's no point in using "" for headers that are not part of your project
because the headers will come from sysroot and never from source file directory.
This is different from core Google and Chrome builds.

On 2011/03/11 01:34:27, kmixter1 wrote:
> On 2011/03/05 04:06:39, Will Drewry wrote:
> > nit: use <> unless the files are part of your package,
> > (applies to all the files in the cl)
> 
> There's some disagreement about this in the chrome os style regarding if files
> part of chrome/chromeos are quoted or angle bracketed:
> crash-reporter (granted, mine) - quotes
> cromo - quotes
> libcros - quotes
> cryptohome - brackets
> entd - quotes
> login manager - brackets
> metrics - brackets
> minijail - brackets
> power manager - quotes
> tpm init - brackets
> update engine - brackets
> verity - brackets
> window manager - quotes

http://codereview.chromium.org/6508016/diff/23015/ipsec_manager.cc#newcode126
ipsec_manager.cc:126: LOG(ERROR) << "Local IP address must be supplied for PSK
mode";
80 chars

http://codereview.chromium.org/6508016/diff/23015/ipsec_manager.cc#newcode159
ipsec_manager.cc:159: LOG(INFO) << "Starter started as pid " << starter_pid;
You could define a ScopedSocketCloser and ScopedAddrInfoReleaser and avoid gotos
(and close(sock) above).

http://codereview.chromium.org/6508016/diff/23015/ipsec_manager_test.cc
File ipsec_manager_test.cc (right):

http://codereview.chromium.org/6508016/diff/23015/ipsec_manager_test.cc#newco...
ipsec_manager_test.cc:12: #include "vpn-manager/ipsec_manager.h"
add blank line before

http://codereview.chromium.org/6508016/diff/23015/l2tpipsec_vpn.cc
File l2tpipsec_vpn.cc (right):

http://codereview.chromium.org/6508016/diff/23015/l2tpipsec_vpn.cc#newcode18
l2tpipsec_vpn.cc:18: #include "vpn-manager/ipsec_manager.h"
blank line before

http://codereview.chromium.org/6508016/diff/23015/service_manager.h
File service_manager.h (right):

http://codereview.chromium.org/6508016/diff/23015/service_manager.h#newcode89
service_manager.h:89: // of output.
The amount of data repeat per call is undefined? Document |partial_line|

[kmixter1, 2011-03-11 20:53:04]

pushing...

Will, James, please comment on this review though if you have other issues. 
I'll roll those fixes into another CL.

http://codereview.chromium.org/6508016/diff/11004/ipsec_manager.cc
File ipsec_manager.cc (right):

http://codereview.chromium.org/6508016/diff/11004/ipsec_manager.cc#newcode15
ipsec_manager.cc:15: #include "base/file_util.h"
On 2011/03/11 19:12:53, petkov wrote:
> FWIW, I agree with Will on this one. Although style isn't quite clear about
> this, there's no point in using "" for headers that are not part of your
project
> because the headers will come from sysroot and never from source file
directory.
> This is different from core Google and Chrome builds.
> 
> On 2011/03/11 01:34:27, kmixter1 wrote:
> > On 2011/03/05 04:06:39, Will Drewry wrote:
> > > nit: use <> unless the files are part of your package,
> > > (applies to all the files in the cl)
> > 
> > There's some disagreement about this in the chrome os style regarding if
files
> > part of chrome/chromeos are quoted or angle bracketed:
> > crash-reporter (granted, mine) - quotes
> > cromo - quotes
> > libcros - quotes
> > cryptohome - brackets
> > entd - quotes
> > login manager - brackets
> > metrics - brackets
> > minijail - brackets
> > power manager - quotes
> > tpm init - brackets
> > update engine - brackets
> > verity - brackets
> > window manager - quotes
> 

I think it comes down to how you see code from libbase, libchromeos, and other
chromeos-base packages.  I see them as part of my project and not part of the
system (which I still see as immutable).  So to me they make sense to be grouped
together with headers from this package.  A middle ground would to be group them
together, but use angled brackets from those outside this package and quotes for
those inside.  Anyway, I can be convinced of any of the 3 styles but I'd prefer
if everyone agreed on one style and switched all at once.  Most important though
is consistency within a package.

http://codereview.chromium.org/6508016/diff/23015/ipsec_manager.cc#newcode126
ipsec_manager.cc:126: LOG(ERROR) << "Local IP address must be supplied for PSK
mode";
On 2011/03/11 19:12:53, petkov wrote:
> 80 chars

Done.

http://codereview.chromium.org/6508016/diff/23015/ipsec_manager.cc#newcode159
ipsec_manager.cc:159: LOG(INFO) << "Starter started as pid " << starter_pid;
On 2011/03/11 19:12:53, petkov wrote:
> You could define a ScopedSocketCloser and ScopedAddrInfoReleaser and avoid
gotos
> (and close(sock) above).

Noted.  If I need to write this kind of code again I will.

http://codereview.chromium.org/6508016/diff/23015/ipsec_manager_test.cc
File ipsec_manager_test.cc (right):

http://codereview.chromium.org/6508016/diff/23015/ipsec_manager_test.cc#newco...
ipsec_manager_test.cc:12: #include "vpn-manager/ipsec_manager.h"
On 2011/03/11 19:12:53, petkov wrote:
> add blank line before

Since right now I'm considering these not system libraries, they're all grouped
together.  Consistent with crash-reporter, window manager, and others.

http://codereview.chromium.org/6508016/diff/23015/service_manager.h
File service_manager.h (right):

http://codereview.chromium.org/6508016/diff/23015/service_manager.h#newcode89
service_manager.h:89: // of output.
On 2011/03/11 19:12:53, petkov wrote:
> The amount of data repeat per call is undefined? Document |partial_line|
> 

Done.