Merge lp://qastaging/~jamesodhunt/ubuntu/natty/upstart/proposed-stage2 into lp://qastaging/ubuntu/natty/upstart

On Mon, Mar 07, 2011 at 03:42:04PM -0000, James Hunt wrote:
> === modified file 'debian/changelog'
> --- debian/changelog 2011-02-28 21:05:32 +0000
> +++ debian/changelog 2011-03-07 15:40:32 +0000
> @@ -1,3 +1,39 @@
> +upstart (0.9.1-1ubuntu4) natty; urgency=low
> +
> + * Merge of upstream lp:~upstart-devel/upstart/0.9.
> +
> + -- James Hunt <email address hidden> Mon, 07 Mar 2011 15:08:35 +0000

I assume I should use
https://launchpad.net/~jamesodhunt/+archive/upstart-testing/+files/upstart_0.9.1.orig.tar.gz
for this? It'll be somewhat behind the upstream you merged in this
change, but I assume you don't mind about that so much?

(I'm not going to pretend to have done an exhaustive documentation
review, I'm afraid, but a few things that I happened to spot:)

> +Note that the \'<\' and \'>\' characters in the \fITime\fP column denote
> +that the event in the \fIEvent\fP column occurs respectively before or
> +after the event specified in the \fITime\fP column (for example, the
> +\fBmounting\fP(7) event occurs "at some time" after the \fBstartup\fP(7)
> +event, and the \fBvirtual-filesystems\fP(7) event occurs after the last
> +\fBmounted\fP(7) event relating to a virtual filesystem has been emitted).

You probably ought to use \- rather than - in event names and the like,
to avoid groff marking them up as hyphens in some cases. (It doesn't
make much difference with the default groff configuration in Ubuntu,
though.)

> +These are specific examples. \fBupstart-udev-bridge\fP(8) will emit
> +events which match the pattern, "\fIS\fP-device-\fIA\fP" where \'S\' is
> +the udev \fIsubsystem\fP and \'A\' is the udev \fIaction\fP. See
> +\fBudev\fP(7) and for further details. If you have
> +.BR sysfs (2)
> +mounted, you can look in \fI/sys/class/\fP for possible values for subsystem.

sysfs(2) is an obsolete system call, and not a helpful cross-reference
here. If there were a manual page for /sys, I'd expect to find it in
section 5 alongside proc(5), but it doesn't seem to exist.

> +It then ascertains the \fIfinal\fP PID for the job which may be a
> +descendent of the immediate child process if \fBexpect fork\fP or

Typo: "descendant".

> +.SH REPORTING BUGS
> +Report bugs at
> +.RB < https://launchpad.net/ubuntu/+source/upstart/+bugs >

Do you actually want this to be the upstream bug reporting URL?
Perhaps https://launchpad.net/upstart/+bugs instead?

> === added file 'extra/conf/upstart-socket-bridge.conf'
> --- extra/conf/upstart-socket-bridge.conf 1970-01-01 00:00:00 +0000
> +++ extra/conf/upstart-socket-bridge.conf 2011-03-07 15:40:32 +0000
> @@ -0,0 +1,14 @@
> +# upstart-socket-bridge - Bridge socket events into upstart
> +#
> +# This helper daemon receives socket(7) events and
> +# emits equivalent Upstart events.
> +
> +description "Bridge socket events into upstart"
> +
> +start on runlevel [2345]
> +stop on runlevel [!2345]

Do we actually want to stop this in single-user mode? It kind of seems
like a core facility of upstart that just happens to be in a single
process because it's more convenient that way. Plus I can imagine that
only starting this once we're in a full runlevel might be a bit late,
especially for Unix sockets.

...

Hi Colin,

Thanks very much for an exceedingly comprehensive review. I've made all
the changes you've specified with 2 exceptions:

* debian/manpages/upstart-events.7: This man page isn't available
  upstream since it documents the "Ubuntu way", so I think the URL is
  correct, unless we decide to push this upstream?

* init/main/init.5:
  > +Note that a user job configuration file cannot have the same name as a
  > +system job configuration file.
  Having looked at the session support code, I'm not sure this is a namespace leak but I
  agree that I think it is a bug: Upstart matches against system jobs first, but it does stop
  non-priv users from manipulating jobs they don't own. Trouble is, previous versions of
  Upstart allowed non-priv to run "initctl list" so we maintain compatibility, but by imposing a
  seemingly artificial restriction on user job names. As discussed, there is no D-Bus method that
  initctl can call to query session details. This seems like an oversight, so if you're happy to
  allow this branch to land, I'll investigate this later (I've raised bug 732656 to cover it).

On Thu, Mar 10, 2011 at 03:43:34PM -0000, James Hunt wrote:
> * debian/manpages/upstart-events.7: This man page isn't available
> upstream since it documents the "Ubuntu way", so I think the URL is
> correct, unless we decide to push this upstream?

Sorry, I meant to delete this part of my review having spotted the same
thing, but apparently didn't (perhaps an undo accident). You're quite
right, of course.

> * init/main/init.5:
> > +Note that a user job configuration file cannot have the same name as a
> > +system job configuration file.
> Having looked at the session support code, I'm not sure this is a namespace leak but I
> agree that I think it is a bug: Upstart matches against system jobs first, but it does stop
> non-priv users from manipulating jobs they don't own. Trouble is, previous versions of
> Upstart allowed non-priv to run "initctl list" so we maintain compatibility, but by imposing a
> seemingly artificial restriction on user job names. As discussed, there is no D-Bus method that
> initctl can call to query session details. This seems like an oversight, so if you're happy to
> allow this branch to land, I'll investigate this later (I've raised bug 732656 to cover it).

Understood, and I agree with your compatibility point. I don't think
this needs to block landing of this branch.