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

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