Re: [Xen-devel] [PATCH v2 for 4.5] xl: Return proper error codes for block-attach and block-detach

[George Dunlap <george.dunlap@xxxxxxxxxxxxx>]

On 11/14/2014 11:12 AM, Ian Campbell wrote:
> On Thu, 2014-11-13 at 19:04 +0000, George Dunlap wrote:
> > Return proper error codes on failure so that scripts can tell whether
> > the command completed properly or not.
> >
> > Also while we're at it, normalize init and dispose of
> > libxl_device_disk structures.  This means calling init and dispose in
> > the actual functions where they are declared.
> >
> > This in turn means changing parse_disk_config_multistring() to not
> > call libxl_device_disk_init.  And that in turn means changes to
> > callers of parse_disk_config().
> >
> > It also means removing libxl_device_disk_init() from
> > libxl.c:libxl_vdev_to_device_disk().  This is only called from
> > xl_cmdimpl.c.
> >
> > Signed-off-by: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
> > ---
> > CC: Ian Campbell <ian.campbell@xxxxxxxxxx>
> > CC: Ian Jackson <ian.jackson@xxxxxxxxxx>
> > CC: Wei Liu <wei.liu2@xxxxxxxxxx>
> > CC: Konrad Wilk <konrad.wilk@xxxxxxxxxx>
> >
> > Release justification: This is a bug fix.  It's a fairly minor one,
> > but it's also a very simple one.
> >
> > v2:
> >   - Restructure functions to make sure init and dispose are properly
> >   called.
> Sadly this bit has somewhat reduced the truth of the second half of your
> release justification, since the patch is a fair bit more subtle though.
> Although IMHO it hasn't invalidated the case for taking the patch for
> 4.5 (modulo comments below).
Well, I think we can take the hacky short-term fix I posted before, 
which is simple, and do a proper fix after the 4.6 dev window opens up; 
or we can do a more complete fix now.
Or, if the valgrind thing is really important, I could use the change 
you suggested, and do "return rc ? 1 : 0;" but I really think that's 
going in the wrong direction...
 -George

> > ---
> >   tools/libxl/libxl.c      |  2 --
> >   tools/libxl/xl_cmdimpl.c | 28 +++++++++++++++++++++-------
> >   2 files changed, 21 insertions(+), 9 deletions(-)
> >
> > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> > index f7961f6..d9c4ce7 100644
> > --- a/tools/libxl/libxl.c
> > +++ b/tools/libxl/libxl.c
> > @@ -2611,8 +2611,6 @@ int libxl_vdev_to_device_disk(libxl_ctx *ctx, uint32_t 
> > domid,
> >       if (devid < 0)
> >           return ERROR_INVAL;
> >   
> > -    libxl_device_disk_init(disk);
> _init functions are idempotent, so this is harmless, I think. Existing
> callers (including things which aren't xl) might be relying on it.
> Outside of a freeze period that might be acceptable (those callers are
> buggy) but since you are asking for a freeze exception I think this may
> be going a step too far in cleaning things up.
>
> In terms of other callers you've missed
> tools/ocaml/libs/xl/xenlight_stubs.c (which appears buggy to me) in
> tree, plus whatever use e.g. libvirt makes of it.
>
> > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> > index 3c9f146..25af715 100644
> > --- a/tools/libxl/xl_cmdimpl.c
> > +++ b/tools/libxl/xl_cmdimpl.c
> > @@ -485,8 +485,6 @@ static void parse_disk_config_multistring(XLU_Config 
> > **config,
> >   {
> >       int e;
> >   
> > -    libxl_device_disk_init(disk);
> Likewise here, although to a lesser extent since this is xl not libxl.
> >   
> > @@ -6378,13 +6382,17 @@ int main_blockattach(int argc, char **argv)
> >           printf("disk: %s\n", json);
> >           free(json);
> >           if (ferror(stdout) || fflush(stdout)) { perror("stdout"); exit(-1); }
> > -        return 0;
> You should set rc = 0 here rather than initing it at declaration to
> catch error paths which don't set the result. (we aren't very consistent
> about this in the code today so I'm only mentioning it because other
> changes seem to be needed, if that turns out not to be the case and
> there's no need for v3 then this shouldn't block acceptance)
>
> > +        goto out;
> I'm not 100% convinced by the use of the goto out error handling style
> for a success path, but it's probably better than duplicating the exit
> path or adding !dryrun checks to all the following operations. Ian,
> since you recently posted updated coding style for things around this,
> what do you think?
>
> Ian.

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