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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Mon, 2014-11-17 at 12:36 +0000, George Dunlap wrote:
> 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.

Specifically the "hacky short-term fix" is
<1415813493-25330-1-git-send-email-george.dunlap@xxxxxxxxxxxxx> ?

I could live with that, perhaps with the commit log explaining that a
little.

Konrad?

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