Re: [Xen-devel] [PATCH 16/20] libxl: event API: new facilities for waiting for subprocesses

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Thanks for the thorough review!

Ian Campbell writes ("Re: [Xen-devel] [PATCH 16/20] libxl: event API: new 
facilities for waiting for subprocesses"):
> On Fri, 2012-03-16 at 16:26 +0000, Ian Jackson wrote:
> > +    /* May make callbacks into the appliction for child processes.
> 
> Typo:                                application

Fixed.

> > +int libxl__self_pipe_eatall(int fd)
> > +{
> > +    char buf[256];
> > +    for (;;) {
> > +        int r = read(fd, buf, sizeof(buf));
> > +        if (r >= 0) return 0;
> 
> Is there some 256 byte limit on the number of bytes pending in the pipe
> somewhere? Wouldn't r==256 require us to (potentially) go round the loop
> again?

If this loop has to go round again, it means we have had 256
wakeup events of some kind.  I don't think this is really an
efficiency concern and it makes the code simpler not to have to worry
about it.

> > +typedef enum {
> > +    /* libxl owns SIGCHLD whenever it has a child. */
> > +    libxl_sigchld_owner_libxl,
> > +
> > +    /* Application promises to call libxl_childproc_exited but NOT
> > +     * from within a signal handler.  libxl will not itself arrange to
> > +     * (un)block or catch SIGCHLD. */
> > +    libxl_sigchld_owner_mainloop,
> > +
> > +    /* libxl owns SIGCHLD all the time, and the application is
> > +     * relying on libxl's event loop for reaping its own children. */
> > +    libxl_sigchld_owner_libxl_always,
> 
> The comment above doesn't seem to cover this third case?

Good point.  I have improved this.

> > +    /* With libxl_sigchld_owner_libxl, called by libxl when it has
> > +     * reaped a pid.  (Not permitted with _owner_mainloop.)
> > +     *
> > +     * Should return 0 if the child was recognised by the application
> > +     * (or if the application does not keep those kind of records),
> > +     * ERROR_NOT_READY if the application knows that the child is not
> 
> ERROR_NOT_READY seems like an odd name. ERROR_UNKNOWN_CHILD perhaps?

I was reusing an existing error code.  Do you think it should have its
own ?

> >  void libxl_postfork_child_noexec(libxl_ctx *ctx);
> >
> >
> > +
> 
> The blank lines appear to be breeding ;-)

OK I think that is one too many :-).

> > +int libxl__sigchld_installhandler(libxl_ctx *ctx) /* non-reentrant */
> > +{
> > +    int r, rc;
> > +
> > +    if (ctx->sigchld_selfpipe[0] < 0) {
> > +        r = pipe(ctx->sigchld_selfpipe);
> > +        if (r) {
> > +            ctx->sigchld_selfpipe[0] = -1;
> > +            LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
> > +                             "failed to create sigchld pipe");
> > +            rc = ERROR_FAIL;
> > +            goto out;
> > +        }
> > +    }
> > +
> > +    atfork_lock();
> > +    if (sigchld_owner != ctx) {
> 
> A non-NULL sigchld_owner which is != ctx is a pretty serious error here,
> isn't it?

Yes.

> > +        struct sigaction ours;
> > +
> > +        assert(!sigchld_owner);

Hence this assertion.

> [...]
> > +void libxl__fork_selfpipe_woken(libxl__egc *egc) {
> > +    /* May make callbacks into the appliction for child processes.
> 
>                                       application

I'm rather afflicted (afflicated?) with these.

> > +     * ctx must be locked EXACTLY ONCE */
> > +    EGC_GC;
> > +
> > +    while (chldmode_ours(CTX) /* in case the app changes the mode */) {
> 
> setmode takes the CTX lock and ctx must be locked here too, so can this
> happen? Ah, we unlock inside the loop, ok then.
> 
> Changing the mode in the callback seems, er, ambitious given you can't
> call it when libxl has any children and we recommend you do it once at
> start of day. Can we not just outlaw it?

The callback might well be what makes the application decide it needs
to change the mode.  It would need an if() anyway since you might
respond to the fd readability after the mode has been changed.

> > +void libxl_childproc_setmode(libxl_ctx *ctx, const libxl_childproc_hooks 
> > *hooks,
> > +                             void *user)
> > +{
> > +    GC_INIT(ctx);
> > +    CTX_LOCK;
> > +
> > +    assert(LIBXL_LIST_EMPTY(&CTX->children));
> > +
> > +    if (!hooks)
> > +        hooks = &libxl__childproc_default_hooks;
>            user = NULL;
> 
> (probably doesn't matter much)

The default "hooks" naturally don't use user; in fact they are both
null function pointers.

> > @@ -263,6 +276,8 @@ struct libxl__ctx {
> >      const libxl_event_hooks *event_hooks;
> >      void *event_hooks_user;
> >
> > +    LIBXL_LIST_ENTRY(struct libxl__ctx);
> 
> Shouldn't there be a field name here? Or is it deliberately anon? (what
> if we get a second list)

This is a leftover hunk.  It can't actually be used because the
LIBXL_LIST_* macros require an actual member name.  I will remove it.

> > @@ -554,6 +573,33 @@ static inline int libxl__ev_xswatch_isregistered(const 
> > libxl__ev_xswatch *xw)
> >
> >
> >  /*
> > + * For making subprocesses.  This is the only permitted mechanism for
> > + * code in libxl to do so.
> > + *
> > + * In the parent, returns the pid, filling in childw_out.
> > + * In the child, returns 0.
> > + * If it fails, returns a libxl error (all of which are -ve).
> > + *
> > + * The child should go on to exec (or exit) soon, and should not make
> > + * any further libxl event calls in the meantime.
> > + *
> > + * The parent may signal the child but it must not reap it.  That will
> > + * be done by the event machinery.  Either death or cldstop may be
> 
> "cldstop" did you rename that childw_out and miss this comment or is it
> something else?

I was going to provide a mechanism analogous to waitpid's WUNTRACED
but thought better of it.  I have fixed the comment.

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel