Re: [Xen-devel] [PATCH] libxl: libxl__spawn_qdisk_backend closes fds

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Roger Pau Monné writes ("Re: [PATCH] libxl: libxl__spawn_qdisk_backend closes 
fds"):
> On 17/03/14 17:32, Ian Jackson wrote:
> > This function needs to close both null and logfile_w on both error and
> > normal exits.  (The child gets its own copy during the fork, and the
> > parent doesn't need them any more.)
> > 
> > Use the standard initialise-to-unallocated, always-free style; the
> > label "out" only makes the callback if rc is nonzero.
> > 
> > Signed-off-by: Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>
> > Coverity-ID: 1130517 and 1130518
> 
> There's a similar patch in
> <21134.22006.356685.408733@xxxxxxxxxxxxxxxxxxxxxxxx>.

Yes.  This posting is an updated version of it.

> > +    /* rc is nonzero iff we had an error; if we had no error then
> > +     * spawn succeeded and we will continue in a further callback */
> > +    if (rc)
> > +        dmss->callback(egc, dmss, rc);
> 
> Using the device_model_spawn_outcome helper instead of directly calling
> the callback will print a nice error message in case spawn has failed.

Good idea.  I will do that as a separate patch.

> >      return;
> >  
> >  error:
> >      assert(rc);
> > -    dmss->callback(egc, dmss, rc);
> > -    return;
> > +    goto out;
> 
> This jump backwards is kind of strange IMHO, why not just rename the
> error label to out and use it instead for both the error and non-error
> exit paths?

That would be possible, but would lose the assert(rc).  An error
handling bug elsewhere in the function, which failed to set rc, would
result in it losing the thread of control.  If we don't think the
assert is important I don't mind losing it.

Ian.

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