port to use centralized constants files, and add input validation

port to use centralized constants files, and add input validation

[Will Drewry, 2010-01-19 17:06:00]

Looks good - some comments and nits on style and testing, but how much you want
to change/integrate is up to you.  Let me know what you want to adopt and I'll
take one pass through (or give a quick LGTM if the answer is not much :)

http://codereview.chromium.org/553016/diff/1/6
File session_manager_service.cc (right):

http://codereview.chromium.org/553016/diff/1/6#newcode48
session_manager_service.cc:48: CHECK(system);
Instead of CHECK for system, why not make it default to the normal value.  You
can either keep the new argument and if it is NULL instantiate your own of what
is the default (new SystemUtils).

Alternately, you can keep this hidden away from the normal user by defaulting to
SystemUtils on construction but provider an accessor to Reset it prior to the
class being used - which will be the case if there is unexpected extensions or a
test.  E.g.,

SessionManagerService smsrv = new 
SessionManagerService(child);
MockSystemUtils *sysutils =  new MockSystemUtils();
smsrv->set_systemutils(sysutils);

That is completely up to you and is just a personal style thing.  I'm just
against exploding constructor arguments with dependencies (since constructors
don't do any work).

http://codereview.chromium.org/553016/diff/1/6#newcode171
session_manager_service.cc:171: email[kMaxEmailSize] = '\0';  // Just to be
sure.
shouldn't that be email[kMaxEmailSize-1] or +1 it during declaration?

http://codereview.chromium.org/553016/diff/1/6#newcode192
session_manager_service.cc:192: g_idle_add_full(G_PRIORITY_DEFAULT_IDLE,
Should Toggle and g_idle* be protected bu if (*OUT_done) {  ... }?

http://codereview.chromium.org/553016/diff/1/6#newcode197
session_manager_service.cc:197: return TRUE;
Should this complain if stop-user-session complains?

Same with Emit... and StartSession?

http://codereview.chromium.org/553016/diff/1/6#newcode247
session_manager_service.cc:247: size_t at = email_address.find('@');
Could be constantized: kEmailSeparator

http://codereview.chromium.org/553016/diff/1/6#newcode270
session_manager_service.cc:270: usleep(500 * 1000 /* milliseconds */);
Not required but would be nice to constantize this.
(Maybe better is drop a TODO in to you and/or me to constantize this and put in
kFooInBar in common/ so that we can easily reference unit change multipliers
(like kUsecondinMsecond as well as kEmailSeparator).

http://codereview.chromium.org/553016/diff/1/8
File session_manager_unittest.cc (right):

http://codereview.chromium.org/553016/diff/1/8#newcode31
session_manager_unittest.cc:31: : expected_calls_(desired_runs+1),
Could change this over to gmock now if you want (and drop it in a reusable
mock_child_job.h file):

class MockChildJob : public ChildJob {
 public:
   MockChildJob() { }
   ~MockChildJob() { }
   MOCK_METHOD0(ShouldRun, bool());
   MOCK_METHOD0(Toggle, void());
   MOCK_METHOD0(Run, void());
};

http://codereview.chromium.org/553016/diff/1/8#newcode61
session_manager_unittest.cc:61: TEST(SessionManagerTest, NoLoopTest) {
These are fine, but if you switch to gmock you can probably write them something
like:

using ::testing::Return;
using ::testing::Invoke;
using ::testing::_;

TEST(SessionManagerTest, NoLoopTest) {
  MockChildJob *job = new MockChildJob;
  login_manager::SessionManagerService manager(job,
                                               new SystemUtils,
                                               true);
  EXPECT_CALL(*job, ShouldRun())
    .Times(1)
    .WillRepeatedly(Return(false));
  manager.Run();
}

static void BadExit() { exit(1); }  // compatible with void Run()
TEST(SessionManagerTest, BadExitChild) {
  MockChildJob *job = new MockChildJob;
  login_manager::SessionManagerService manager(job,
                                               new SystemUtils,
                                               true);
  EXPECT_CALL(*job, ShouldRun())
    .Times(1)
    .WillOnce(Return(true));
  ON_CALL(*job, Run())
    .Times(1)
    .WillByDefault(Invoke(BadExit));
  manager.Run();
}

static void CleanExit() { exit(0); }
TEST(SessionManagerTest, CleanExitChild) {
  MockChildJob *job = new MockChildJob;
  login_manager::SessionManagerService manager(job,
                                               new SystemUtils,
                                               true);
  EXPECT_CALL(*job, ShouldRun())
    .Times(1)
    .WillRepeatedly(Return(true));
  ON_CALL(*job, Run())
    .WillByDefault(Invoke(CleanExit));
  manager.Run();
}


(untested - showed both Will* options but WillOnce should do the trick for the
non-multi cases)

http://codereview.chromium.org/553016/diff/1/8#newcode98
session_manager_unittest.cc:98: class MockSystem : public SystemUtils {
Another good case for gmock :)

http://codereview.chromium.org/553016/diff/1/8#newcode122
session_manager_unittest.cc:122: manager.set_child_pgid(4);
manager.set_child_pgid(kDummyPid);
EXPECTED_CALL(*mocksys, kill(kDummyPid, SIGKILL))
  .Times(1)
  .WillOnce(Return(0));
EXPECTED_CALL(*mocksys, child_is_gone(kDummyPid))
  .Times(1)
  .WillOnce(Return(true));

If you switch over to gmock, you could also test the case where SIGKILL doesn't
result in child_is_gone() returning true - which could happen if in  diskwait,
etc.

The example I give doesn't work for your case, but your test doesn't explore the
try count either.  With gmock, you can have it expect N calls to kill with
SIGTERM followed by SIGKILL and also set the number of child_is_gone returning
true/false.  Then you can model SIGTERM success as well.

[Chris Masone, 2010-01-19 21:52:02]

so much mocking, you don't even know

http://codereview.chromium.org/553016/diff/1/6
File session_manager_service.cc (right):

http://codereview.chromium.org/553016/diff/1/6#newcode48
session_manager_service.cc:48: CHECK(system);
On 2010/01/19 17:06:00, Will Drewry wrote:
> Instead of CHECK for system, why not make it default to the normal value.  You
> can either keep the new argument and if it is NULL instantiate your own of
what
> is the default (new SystemUtils).
> 
> Alternately, you can keep this hidden away from the normal user by defaulting
to
> SystemUtils on construction but provider an accessor to Reset it prior to the
> class being used - which will be the case if there is unexpected extensions or
a
> test.  E.g.,
> 
> SessionManagerService smsrv = new 
> SessionManagerService(child);
> MockSystemUtils *sysutils =  new MockSystemUtils();
> smsrv->set_systemutils(sysutils);
> 
> That is completely up to you and is just a personal style thing.  I'm just
> against exploding constructor arguments with dependencies (since constructors
> don't do any work).
> 

I've done this all with setters, now, since I don't like default params.

http://codereview.chromium.org/553016/diff/1/6#newcode171
session_manager_service.cc:171: email[kMaxEmailSize] = '\0';  // Just to be
sure.
On 2010/01/19 17:06:00, Will Drewry wrote:
> shouldn't that be email[kMaxEmailSize-1] or +1 it during declaration?

y'know, you're right.

http://codereview.chromium.org/553016/diff/1/6#newcode192
session_manager_service.cc:192: g_idle_add_full(G_PRIORITY_DEFAULT_IDLE,
On 2010/01/19 17:06:00, Will Drewry wrote:
> Should Toggle and g_idle* be protected bu if (*OUT_done) {  ... }?

...I think so?  Certainly above, in StartSession (which I added).  Here...well,
I guess it depends on how aggressive we want to be.  If we're stopping the
session, maybe we want to go ahead and nuke Chrome et al even if we don't get
other stuff to shut down gracefully.

http://codereview.chromium.org/553016/diff/1/6#newcode197
session_manager_service.cc:197: return TRUE;
On 2010/01/19 17:06:00, Will Drewry wrote:
> Should this complain if stop-user-session complains?
> 
> Same with Emit... and StartSession?

I was going to say that I figured that's what OUT_done was for, returning that
kind of status...but it seems like we should mirror that in the return value. 
Or maybe get rid of OUT_done and just return the bool.  For now, I'll just make
them both return the same.

I'm also going to add error messages in a soon-to-come CL

http://codereview.chromium.org/553016/diff/1/6#newcode247
session_manager_service.cc:247: size_t at = email_address.find('@');
On 2010/01/19 17:06:00, Will Drewry wrote:
> Could be constantized: kEmailSeparator

Done.

http://codereview.chromium.org/553016/diff/1/6#newcode270
session_manager_service.cc:270: usleep(500 * 1000 /* milliseconds */);
On 2010/01/19 17:06:00, Will Drewry wrote:
> Not required but would be nice to constantize this.
> (Maybe better is drop a TODO in to you and/or me to constantize this and put
in
> kFooInBar in common/ so that we can easily reference unit change multipliers
> (like kUsecondinMsecond as well as kEmailSeparator).

Done.

http://codereview.chromium.org/553016/diff/1/8
File session_manager_unittest.cc (right):

http://codereview.chromium.org/553016/diff/1/8#newcode31
session_manager_unittest.cc:31: : expected_calls_(desired_runs+1),
On 2010/01/19 17:06:00, Will Drewry wrote:
> Could change this over to gmock now if you want (and drop it in a reusable
> mock_child_job.h file):
> 
> class MockChildJob : public ChildJob {
>  public:
>    MockChildJob() { }
>    ~MockChildJob() { }
>    MOCK_METHOD0(ShouldRun, bool());
>    MOCK_METHOD0(Toggle, void());
>    MOCK_METHOD0(Run, void());
> };

Done.

http://codereview.chromium.org/553016/diff/1/8#newcode61
session_manager_unittest.cc:61: TEST(SessionManagerTest, NoLoopTest) {
On 2010/01/19 17:06:00, Will Drewry wrote:
> These are fine, but if you switch to gmock you can probably write them
something
> like:
> 
> using ::testing::Return;
> using ::testing::Invoke;
> using ::testing::_;
> 
> TEST(SessionManagerTest, NoLoopTest) {
>   MockChildJob *job = new MockChildJob;
>   login_manager::SessionManagerService manager(job,
>                                                new SystemUtils,
>                                                true);
>   EXPECT_CALL(*job, ShouldRun())
>     .Times(1)
>     .WillRepeatedly(Return(false));
>   manager.Run();
> }
> 
> static void BadExit() { exit(1); }  // compatible with void Run()
> TEST(SessionManagerTest, BadExitChild) {
>   MockChildJob *job = new MockChildJob;
>   login_manager::SessionManagerService manager(job,
>                                                new SystemUtils,
>                                                true);
>   EXPECT_CALL(*job, ShouldRun())
>     .Times(1)
>     .WillOnce(Return(true));
>   ON_CALL(*job, Run())
>     .Times(1)
>     .WillByDefault(Invoke(BadExit));
>   manager.Run();
> }
> 
> static void CleanExit() { exit(0); }
> TEST(SessionManagerTest, CleanExitChild) {
>   MockChildJob *job = new MockChildJob;
>   login_manager::SessionManagerService manager(job,
>                                                new SystemUtils,
>                                                true);
>   EXPECT_CALL(*job, ShouldRun())
>     .Times(1)
>     .WillRepeatedly(Return(true));
>   ON_CALL(*job, Run())
>     .WillByDefault(Invoke(CleanExit));
>   manager.Run();
> }
> 
> 
> (untested - showed both Will* options but WillOnce should do the trick for the
> non-multi cases)

I haz tweaked your mocks to work right and used them.

http://codereview.chromium.org/553016/diff/1/8#newcode98
session_manager_unittest.cc:98: class MockSystem : public SystemUtils {
On 2010/01/19 17:06:00, Will Drewry wrote:
> Another good case for gmock :)

Truly.

http://codereview.chromium.org/553016/diff/1/8#newcode122
session_manager_unittest.cc:122: manager.set_child_pgid(4);
On 2010/01/19 17:06:00, Will Drewry wrote:
> manager.set_child_pgid(kDummyPid);
> EXPECTED_CALL(*mocksys, kill(kDummyPid, SIGKILL))
>   .Times(1)
>   .WillOnce(Return(0));
> EXPECTED_CALL(*mocksys, child_is_gone(kDummyPid))
>   .Times(1)
>   .WillOnce(Return(true));
> 
> If you switch over to gmock, you could also test the case where SIGKILL
doesn't
> result in child_is_gone() returning true - which could happen if in  diskwait,
> etc.
> 
> The example I give doesn't work for your case, but your test doesn't explore
the
> try count either.  With gmock, you can have it expect N calls to kill with
> SIGTERM followed by SIGKILL and also set the number of child_is_gone returning
> true/false.  Then you can model SIGTERM success as well.

I have now done all sorts of sweet object mocking.

[Will Drewry, 2010-01-19 22:20:05]

That's some fine gmockery. A couple of nits and a thought about the
CHROMEOS_USER=%s validation.

http://codereview.chromium.org/553016/diff/5002/5007
File mock_system_utils.h (right):

http://codereview.chromium.org/553016/diff/5002/5007#newcode11
mock_system_utils.h:11: #include <unistd.h>
nit - C includes should go before C++ ones IIRC (but after the local include...)

http://codereview.chromium.org/553016/diff/5002/5009
File session_manager_service.cc (right):

http://codereview.chromium.org/553016/diff/5002/5009#newcode167
session_manager_service.cc:167: StringPrintf("/sbin/initctl emit
start-user-session CHROMEOS_USER=%s &",
While I assume that the authentication code should stop this, but if not, it
looks like this is can have some sweet shell injection.

(We really just need [a-z0-9@.] if we're enforcing lower ASCII only.)

http://codereview.chromium.org/553016/diff/5002/5010
File session_manager_service.h (right):

http://codereview.chromium.org/553016/diff/5002/5010#newcode41
session_manager_service.h:41: // Takes ownership of |child|, |system|.
No long a |system| argument to take ownership of.

http://codereview.chromium.org/553016/diff/5002/5010#newcode80
session_manager_service.h:80: void set_exit_on_child_done(bool do_exit) {
exit_on_child_done_ = do_exit; }
Might be worth noting that some of these need to be called _before_ any other
calls after the constructor.

http://codereview.chromium.org/553016/diff/5002/5011
File session_manager_unittest.cc (right):

http://codereview.chromium.org/553016/diff/5002/5011#newcode35
session_manager_unittest.cc:35: class RunEnJob : public ChildJob {
Is this code dead now?

http://codereview.chromium.org/553016/diff/5002/5013
File system_utils.h (right):

http://codereview.chromium.org/553016/diff/5002/5013#newcode20
system_utils.h:20: return kill(pid, signal);
I'm assuming this works as is but if not, I'd think this would need to be
  ::kill(pid, signal)

[Chris Masone, 2010-01-19 23:04:24]

http://codereview.chromium.org/553016/diff/5002/5007
File mock_system_utils.h (right):

http://codereview.chromium.org/553016/diff/5002/5007#newcode11
mock_system_utils.h:11: #include <unistd.h>
On 2010/01/19 22:20:05, Will Drewry wrote:
> nit - C includes should go before C++ ones IIRC (but after the local
include...)

Done.

http://codereview.chromium.org/553016/diff/5002/5009
File session_manager_service.cc (right):

http://codereview.chromium.org/553016/diff/5002/5009#newcode167
session_manager_service.cc:167: StringPrintf("/sbin/initctl emit
start-user-session CHROMEOS_USER=%s &",
On 2010/01/19 22:20:05, Will Drewry wrote:
> While I assume that the authentication code should stop this, but if not, it
> looks like this is can have some sweet shell injection.
> 
> (We really just need [a-z0-9@.] if we're enforcing lower ASCII only.)

The login code should enforce, but this we don't enforce that only the login
code can talk to this service :-D

since we validate before we force to lower case (so that we're sure we can force
lower case), I check for all ascii alphanums and . and @

http://codereview.chromium.org/553016/diff/5002/5010
File session_manager_service.h (right):

http://codereview.chromium.org/553016/diff/5002/5010#newcode41
session_manager_service.h:41: // Takes ownership of |child|, |system|.
On 2010/01/19 22:20:05, Will Drewry wrote:
> No long a |system| argument to take ownership of.

Done.

http://codereview.chromium.org/553016/diff/5002/5010#newcode80
session_manager_service.h:80: void set_exit_on_child_done(bool do_exit) {
exit_on_child_done_ = do_exit; }
On 2010/01/19 22:20:05, Will Drewry wrote:
> Might be worth noting that some of these need to be called _before_ any other
> calls after the constructor.

Done.

http://codereview.chromium.org/553016/diff/5002/5011
File session_manager_unittest.cc (right):

http://codereview.chromium.org/553016/diff/5002/5011#newcode35
session_manager_unittest.cc:35: class RunEnJob : public ChildJob {
On 2010/01/19 22:20:05, Will Drewry wrote:
> Is this code dead now?

hah.  yep.

http://codereview.chromium.org/553016/diff/5002/5013
File system_utils.h (right):

http://codereview.chromium.org/553016/diff/5002/5013#newcode20
system_utils.h:20: return kill(pid, signal);
On 2010/01/19 22:20:05, Will Drewry wrote:
> I'm assuming this works as is but if not, I'd think this would need to be
>   ::kill(pid, signal)
> 

it seems to, yeah, but I've made this change anyway

[Will Drewry, 2010-01-19 23:12:42]

LGTM