[PATCH] os: added the -upstart option for Upstart's signaling method.

[Oliver McFadden]

On Wed, 2010-02-24 at 08:49 +0100, ext Aaron Plattner wrote:
> On Tue, Feb 23, 2010 at 10:15:31PM -0800, Oliver McFadden wrote:
> > This is very similar to the RunFromSmartParent option, except we do not
> > send the signal to our parent process, but our own process instead; also
> > the signal is SIGSTOP, not SIGUSR1.
> > 
> > Upstart will detect that our process has stopped, and interprets this as
> > "process ready" (to accept clients), send us SIGCONT, and move our job
> > from the SPAWNED to RUNNING status.
> > 
> > Signed-off-by: Oliver McFadden <oliver.mcfadden at nokia.com>
> > ---
> > Unfortunately we're stuck with using Upstart, and in the tradition of trying to
> > keep as close to upstream as possible, I'm sending this for review...
> > 
> > According to http://upstart.ubuntu.com/ Upstart is used by a number of
> > distributions (Ubuntu, Fedora, Debian, and of course Maemo) so this is probably
> > a worthwhile patch.
> > 
> >  include/opaque.h |    1 +
> >  os/connection.c  |    5 +++++
> >  os/utils.c       |    5 +++++
> >  3 files changed, 11 insertions(+), 0 deletions(-)
> > 
> > diff --git a/include/opaque.h b/include/opaque.h
> > index b3c7c70..f8c0000 100644
> > --- a/include/opaque.h
> > +++ b/include/opaque.h
> > @@ -55,6 +55,7 @@ extern _X_EXPORT int defaultBackingStore;
> >  extern _X_EXPORT Bool disableBackingStore;
> >  extern _X_EXPORT Bool enableBackingStore;
> >  extern _X_EXPORT Bool PartialNetwork;
> > +extern _X_EXPORT Bool RunFromUpstart;
> >  #ifndef NOLOGOHACK
> >  extern _X_EXPORT int logoScreenSaver;
> >  #endif
> > diff --git a/os/connection.c b/os/connection.c
> > index 3ff93bb..92aec38 100644
> > --- a/os/connection.c
> > +++ b/os/connection.c
> > @@ -146,6 +146,8 @@ Bool NewOutputPending;		/* not yet attempted to write some new output */
> >  Bool AnyClientsWriteBlocked;	/* true if some client blocked on write */
> >  
> >  static Bool RunFromSmartParent;	/* send SIGUSR1 to parent process */
> > +Bool RunFromUpstart;		/* send SIGSTOP to our own process;
> > +				   Upstart will send SIGCONT back. */
> >  Bool PartialNetwork;	/* continue even if unable to bind all addrs */
> >  static Pid_t ParentProcess;
> >  
> > @@ -357,6 +359,9 @@ NotifyParentProcess(void)
> >  	    kill (ParentProcess, SIGUSR1);
> >  	}
> >      }
> > +    if (RunFromUpstart) {
> > +	kill (getpid (), SIGSTOP);
> 
> raise(SIGSTOP) is simpler.

Ok, I'll fix that.

> Is there any way to tell for sure that Upstart is running and is going to
> wake us up, or do we just deadlock if something goes wrong with it?

No, unless we want to do something ugly like checking for a running
process with the name of "upstart". Yes, if we are passed -upstart
without being executed from Upstart, then we'll never get SIGCONT and
therefor just stay stopped forever.

> 
> > +    }
> >  #endif
> >  }
> >  
> > diff --git a/os/utils.c b/os/utils.c
> > index 21e25e0..d19ffb0 100644
> > --- a/os/utils.c
> > +++ b/os/utils.c
> > @@ -538,6 +538,7 @@ void UseMsg(void)
> >  #endif
> >      ErrorF("-dumbSched             Disable smart scheduling, enable old behavior\n");
> >      ErrorF("-schedInterval int     Set scheduler interval in msec\n");
> > +    ErrorF("-upstart               Enable support for Upstart's signaling method\n");
> 
> This almost implies that -upstart is something somebody might actually want
> to pass to the X server.  In cases like this, less documentation is almost
> better; "Do not use this option" conveys more useful information to the
> user.

How about "Only use when running with Upstart installed"? I'll also
write something in the server man page.

> 
> >      ErrorF("+extension name        Enable extension\n");
> >      ErrorF("-extension name        Disable extension\n");
> >  #ifdef XDMCP
> > @@ -936,6 +937,10 @@ ProcessCommandLine(int argc, char *argv[])
> >  		UseMsg ();
> >  	}
> >  #endif
> > +	else if ( strcmp( argv[i], "-upstart") == 0)
> > +	{
> > +	    RunFromUpstart = TRUE;
> 
> Running away from Upstart sounds like a good idea.  ;)

Heh, unfortunately, you can run, but you can't hide.

> 
> > +	}
> >  	else if ( strcmp( argv[i], "+extension") == 0)
> >  	{
> >  	    if (++i < argc)
> > -- 
> > 1.6.1
> >