Merge lp://staging/~stgraber/upstart/upstart-session-socket into lp://staging/upstart

* conf/rc-sysinit.conf: I've got some reservations about changing the
  default jobs we provide. That said, those building systems should
  really be creating their own jobs. I think I'd be happy if we added a
  note to the README stating that the files in 'conf/' are examples only
  and *will* need to be changed by those creating systems using Upstart.
* conf/rc.conf: As above.
* init/control.c: control_init(): Spaces before '(' (4 occurences).
* init/job_process.c:
* init/main.c:
* util/initctl.c:
  - user_mode: Comment should be something like 'if TRUE, talk to
    Upstart over a private D-Bus socket'.
  - upstart_open():
    - Space before '(' of getenv call (2 occurences).
    - I'd be tempted to save the value of the first call to getenv to
      avoid the 2nd call.
    - 'else' should be on same line as closing brace of 'if'.
    - Shouldn't the logic be inverted here to conform to?:
      https://wiki.ubuntu.com/FoundationsTeam/Specs/RaringUpstartUserSessions#Effect_of_UPSTART_SESSION
      So, by default, if UPSTART_SESSION is set, use the private D-Bus
      socket, unless the user specifies '--system'.
  - options: I'd be tempted to change 'start' to 'run' as the former
    implies a long-running operation.
* init/man/init.5: 'Job environment' section needs to be updated for
  'UPSTART_SESSION'.
* util/man/initctl.8: Need to document '--user'.
* util/tests/test_initctl.c: We could do with a test for '--user'. This
  will necessitate something like:
  - Changing _START_UPSTART() into a function and allowing atleast 1 extra
    argument to be specified or specifying the type (meaning '--user' or
    '--session') would work, along with a test
  - Changing WAIT_FOR_UPSTART() into a function to allow the session
    bus or private socket to be connected to.
  - Then, a test to simply start upstart, wait for it, create a conf
    file, check that initctl list shows that job, then stop upstart
    (see test_list()).

> * conf/rc-sysinit.conf: I've got some reservations about changing the
> default jobs we provide. That said, those building systems should
> really be creating their own jobs. I think I'd be happy if we added a
> note to the README stating that the files in 'conf/' are examples only
> and *will* need to be changed by those creating systems using Upstart.
> * conf/rc.conf: As above.

Those two ended up in this branch by mistake, that should be fixed now.

> * init/control.c: control_init(): Spaces before '(' (4 occurences).
> * init/job_process.c:
> * init/main.c:
> * util/initctl.c:
> - user_mode: Comment should be something like 'if TRUE, talk to
> Upstart over a private D-Bus socket'.
> - upstart_open():
> - Space before '(' of getenv call (2 occurences).
> - I'd be tempted to save the value of the first call to getenv to
> avoid the 2nd call.
> - 'else' should be on same line as closing brace of 'if'.
> - Shouldn't the logic be inverted here to conform to?:
> https://wiki.ubuntu.com/FoundationsTeam/Specs/RaringUpstartUserSessions#
> Effect_of_UPSTART_SESSION
> So, by default, if UPSTART_SESSION is set, use the private D-Bus
> socket, unless the user specifies '--system'.
> - options: I'd be tempted to change 'start' to 'run' as the former
> implies a long-running operation.
> * init/man/init.5: 'Job environment' section needs to be updated for
> 'UPSTART_SESSION'.
> * util/man/initctl.8: Need to document '--user'.
> * util/tests/test_initctl.c: We could do with a test for '--user'. This
> will necessitate something like:
> - Changing _START_UPSTART() into a function and allowing atleast 1 extra
> argument to be specified or specifying the type (meaning '--user' or
> '--session') would work, along with a test
> - Changing WAIT_FOR_UPSTART() into a function to allow the session
> bus or private socket to be connected to.
> - Then, a test to simply start upstart, wait for it, create a conf
> file, check that initctl list shows that job, then stop upstart
> (see test_list()).

> > * conf/rc-sysinit.conf: I've got some reservations about changing the
> > default jobs we provide. That said, those building systems should
> > really be creating their own jobs. I think I'd be happy if we added a
> > note to the README stating that the files in 'conf/' are examples only
> > and *will* need to be changed by those creating systems using Upstart.
> > * conf/rc.conf: As above.
>
> Those two ended up in this branch by mistake, that should be fixed now.
>
> > * init/control.c: control_init(): Spaces before '(' (4 occurences).

Fixed.

> > * init/job_process.c:
> > * init/main.c:
> > * util/initctl.c:
> > - user_mode: Comment should be something like 'if TRUE, talk to
> > Upstart over a private D-Bus socket'.

Changed.

> > - upstart_open():
> > - Space before '(' of getenv call (2 occurences).

Just one after the fix below, but fixed :)

> > - I'd be tempted to save the value of the first call to getenv to
> > avoid the 2nd call.

Fixed.

> > - 'else' should be on same line as closing brace of 'if'.

Fixed. Working on 3 different C projects each with different coding guideline is a real source of headache ;)

> > - Shouldn't the logic be inverted here to conform to?:
> >
> https://wiki.ubuntu.com/FoundationsTeam/Specs/RaringUpstartUserSessions#
> > Effect_of_UPSTART_SESSION
> > So, by default, if UPSTART_SESSION is set, use the private D-Bus
> > socket, unless the user specifies '--system'.

Yeah, that part is still a bit blurry in my head, I'll try to catch you tomorrow to discuss this some more.

Does that table mean we won't ever look for upstart on the session bus when passed --user?

Also, I'm at a bit of a loss as to how "initctl --user" as root without UPSTART_SESSION isn't failing according to that table?

> > - options: I'd be tempted to change 'start' to 'run' as the former
> > implies a long-running operation.

Done, was a copy/paste from main.c.

> > * init/man/init.5: 'Job environment' section needs to be updated for
> > 'UPSTART_SESSION'.

Done

> > * util/man/initctl.8: Need to document '--user'.

Done

> > * util/tests/test_initctl.c: We could do with a test for '--user'. This
> > will necessitate something like:
> > - Changing _START_UPSTART() into a function and allowing atleast 1 extra
> > argument to be specified or specifying the type (meaning '--user' or
> > '--session') would work, along with a test
> > - Changing WAIT_FOR_UPSTART() into a function to allow the session
> > bus or private socket to be connected to.
> > - Then, a test to simply start upstart, wait for it, create a conf
> > file, check that initctl list shows that job, then stop upstart
> > (see test_list()).

Will look into this once we agree on what initctl should do :)

Alright, I think the current version of the branch matches the spec.

I also included a change to upstart itself, so that --user doesn't mean that it'll listen to the session bus, if you want to force it to listen to the session bus and do user session stuff, --session --user is what you want.

Hi Stéphane,

* init/control.c:
  - control_init(): Second call to nih_strcat_sprintf() should be
    nih_strcat() or nih_strdup().
* util/tests/test_initctl.c: Still need a test for 'init --user'.

I replaced the nih_strcat_sprintf by a nih_strdup.

I also dropped some changes to main.c that weren't relevant after the clear split between use_session_bus and user_session. That makes "/sbin/init --user --session" now possible.

And added two tests:
 - Check that initctl connects to the user bus when user_mode is set to TRUE and UPSTART_SESSION is set.
 - Check that initctl fails when user_mode is set to TRUE and UPSTART_SESSION isn't set.

I also updated the manpage to reflect the change in behaviour (now matching the spec).

Thanks! LGTM - merged.