[PATCH weston 1/3] compositor: add weston_client_start()

[Pekka Paalanen]

On Wed, 27 Aug 2014 11:31:18 -0500
Derek Foreman <derekf at osg.samsung.com> wrote:

> On 27/08/14 05:41 AM, Pekka Paalanen wrote:
> > From: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
> > 
> > weston_client_start() is a new wrapper around weston_client_launch(),
> > that does the process tracking on its own, and logs the process exit
> > status.
> > 
> > When users of weston_client_start() want to know when the process exits,
> > they should hook into the wl_client destroy signal. This works for cases
> > where the client is not expected to disconnect without exiting.
> > 
> > As wl_client destructor and the sigchld handler run in arbitary order,
> > it is usually difficult for users to maintain both struct weston_process
> > and a struct wl_client pointer. You would need to wait for both
> > destructor and handler to have run, before attempting to respawn the
> > client.
> > 
> > This new function relieves the caller from the burden of maintaining the
> > struct weston_process, assuming the caller is only interested in client
> > disconnects.
> > 
> > Cc: Boyan Ding <stu_dby at 126.com>
> > Cc: Derek Foreman <derekf at osg.samsung.com>
> > Signed-off-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
> > ---
> >  src/compositor.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  src/compositor.h |  3 +++
> >  2 files changed, 63 insertions(+)
> > 
> > diff --git a/src/compositor.c b/src/compositor.c
> > index 96e3435..c62077c 100644
> > --- a/src/compositor.c
> > +++ b/src/compositor.c
> > @@ -319,6 +319,66 @@ weston_client_launch(struct weston_compositor *compositor,
> >  	return client;
> >  }
> >  
> > +struct process_info {
> > +	struct weston_process proc;
> > +	char *path;
> > +};
> > +
> > +static void
> > +process_handle_sigchld(struct weston_process *process, int status)
> > +{
> > +	struct process_info *pinfo =
> > +		container_of(process, struct process_info, proc);
> > +
> > +	/*
> > +	 * There are no guarantees whether this runs before or after
> > +	 * the wl_client destructor.
> > +	 */
> > +
> > +	if (WIFEXITED(status)) {
> > +		weston_log("%s exited with status %d\n", pinfo->path,
> > +			   WEXITSTATUS(status));
> 
> If I might be immensely pedantic, this may in fact only indicate that
> the process that was going to exec pinfo->path failed.  So, the log
> message may indicate that lala/foo exited with status 255 when lala/foo
> was actually ENOENT.
> 
> I find that to be a little confusing, but I can get past it if nobody
> else minds.  :)

Oh yeah. Well, can fix it later, it's just cosmetic. We do want to know
what was attemped to be executed.

Need to verify that the exit code due to failing child_client_exec() is
sufficiently unique, and just check for that.

If we only had some way to pass the errno from the _exit()'ing process
back here, we could print the exec error message here instead of from
the fork'd process which races with the log writing.

Hmm, continuing that thought, should we perhaps hook into the launched
client's stderr? oh, or maybe stdlog really? Bah, stdlog is not
standard, so pass an fd via env for logging errors... redirecting all
stderr's of the whole process tree back into Weston's log might be too
much. Ahh, nevermind, the current way works well enough. :-)


Thanks,
pq

> > +	} else if (WIFSIGNALED(status)) {
> > +		weston_log("%s died on signal %d\n", pinfo->path,
> > +			   WTERMSIG(status));
> > +	} else {
> > +		weston_log("%s disappeared\n", pinfo->path);
> > +	}
> > +
> > +	free(pinfo->path);
> > +	free(pinfo);
> > +}
> > +
> > +WL_EXPORT struct wl_client *
> > +weston_client_start(struct weston_compositor *compositor, const char *path)
> > +{
> > +	struct process_info *pinfo;
> > +	struct wl_client *client;
> > +
> > +	pinfo = zalloc(sizeof *pinfo);
> > +	if (!pinfo)
> > +		return NULL;
> > +
> > +	pinfo->path = strdup(path);
> > +	if (!pinfo->path)
> > +		goto out_free;
> > +
> > +	client = weston_client_launch(compositor, &pinfo->proc, path,
> > +				      process_handle_sigchld);
> > +	if (!client)
> > +		goto out_str;
> > +
> > +	return client;
> > +
> > +out_str:
> > +	free(pinfo->path);
> > +
> > +out_free:
> > +	free(pinfo);
> > +
> > +	return NULL;
> > +}
> > +
> >  static void
> >  region_init_infinite(pixman_region32_t *region)
> >  {
> > diff --git a/src/compositor.h b/src/compositor.h
> > index c0fc0a6..24a9768 100644
> > --- a/src/compositor.h
> > +++ b/src/compositor.h
> > @@ -1345,6 +1345,9 @@ weston_client_launch(struct weston_compositor *compositor,
> >  		     const char *path,
> >  		     weston_process_cleanup_func_t cleanup);
> >  
> > +struct wl_client *
> > +weston_client_start(struct weston_compositor *compositor, const char *path);
> > +
> >  void
> >  weston_watch_process(struct weston_process *process);
> >  
> >