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

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

On Thu, Nov 20, 2014 at 03:47:04PM +0000, Ian Campbell wrote:
> 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?

<hands George the band-aid tape>

Release-Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> 
> > 
> > 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