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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2012-03-20 at 17:24 +0000, Ian Jackson wrote:
> 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.

My concern is that we would exit the loop _without_ going round again
because if r == 256 then also r>= 0 and we return i.e. we don't actually
"eatall" the buffer, or does that not matter?

> > > +    /* 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 ?

You once told me we shouldn't be shy about adding error codes for
specific things..

> > > +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.

Right.

> > > +     * 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.

OK
Ian.



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