Re: [Xen-devel] [PATCH 11/29] libxl: events: Make libxl__async_exec_* pass caller an rc

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

On Tue, 2015-03-31 at 19:12 +0100, Ian Jackson wrote:
> Ian Campbell writes ("Re: [Xen-devel] [PATCH 11/29] libxl: events: Make 
> libxl__async_exec_* pass caller an rc"):
> > On Tue, 2015-02-10 at 20:09 +0000, Ian Jackson wrote:
> > > diff --git a/tools/libxl/libxl_aoutils.c b/tools/libxl/libxl_aoutils.c
> > > index 754e2d1..891cdb8 100644
> > > --- a/tools/libxl/libxl_aoutils.c
> > > +++ b/tools/libxl/libxl_aoutils.c
> > > @@ -483,11 +483,12 @@ static void async_exec_done(libxl__egc *egc,
> > >      libxl__ev_time_deregister(gc, &aes->time);
> > >  
> > >      if (status) {
> > > -        libxl_report_child_exitstatus(CTX, LIBXL__LOG_ERROR,
> > > -                                      aes->what, pid, status);
> > > +        if (!aes->rc)
> > 
> > Could be one "if (status && !aes->rc)", unless perhaps there is more
> > code to come in this block?
> 
> No, there is no more to come.  I find it clearer this way but I don't
> mind changing it.

No it's fine if you don't want to.

> > > +                        libxl__async_exec_state *aes, int rc, int 
> > > status);
> > > +/*
> > > + * Meaning of status and rc:
> > > + *  rc==0, status==0    all went well
> > > + *  rc==0, status!=0    everything OK except child exited nonzero 
> > > (logged)
> > > + *  rc!=0               something else went wrong (status is real
> > > + *                       exit status, maybe reflecting SIGKILL if aes
> > > + *                       code killed the child).  Logged unless 
> > > CANCELLED.
> > 
> > I'm unclear on whether status is valid in this third case or not. I
> > think you are saying that it is (probably?) valid but if rc!=0 the
> > caller likely doesn't actually care what it is?
> 
> status is definitely valid but maybe uninteresting, as stated in the
> comment.

I think I initially parsed it as "real exit status, maybe", which isn't
really what it says...

> Would it help to add something about status to the third row of the
> little table bit at the left ?

Perhaps, or perhaps:
s/ maybe//; s/child)/child, and therefore likely to be uninteresting)/
?

In any case now that I've read it correctly:
        Acked-by: Ian Campbell <ian.campbell@xxxxxxxxxx>

But if you want to clarify any further please go ahead and retain the
ack.

Ian.


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