Re: [PATCH v3 1/6] xl: Return proper error codes for block-attach and block-detach

[Ian Campbell <ian.campbell@citrix.com>]

On Tue, 2015-12-01 at 11:53 +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.

... and the ocaml bindings.

I can't remember what we decided regarding libxl "getter" functions and
initialisation of the data type (i.e. whose responsibility it is), but it
seems that changing a given calls semantics is rather dangerous API-wise.

Wei, ISTR you stumbling over this once and the formulation of A Plan(tm),
but I can't remember what it was, can you?

> 
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> ---
> CC: Ian Campbell <ian.campbell@citrix.com>
> CC: Ian Jackson <ian.jackson@citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Konrad Wilk <konrad.wilk@oracle.com>
> 
> v3:
>  - Rebased to tip
> 
> v2:
>  - Restructure functions to make sure init and dispose are properly
>  called.
> ---
>  tools/libxl/libxl.c      |  2 --
>  tools/libxl/xl_cmdimpl.c | 29 ++++++++++++++++++++---------
>  2 files changed, 20 insertions(+), 11 deletions(-)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index bd3aac8..814d056 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -2738,8 +2738,6 @@ int libxl_vdev_to_device_disk(libxl_ctx *ctx,
> uint32_t domid,
>      if (devid < 0)
>          return ERROR_INVAL;
>  
> -    libxl_device_disk_init(disk);
> -
>      dompath = libxl__xs_get_dompath(gc, domid);
>      if (!dompath) {
>          goto out;
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 2b6371d..2ba2393 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -570,8 +570,6 @@ static void parse_disk_config_multistring(XLU_Config
> **config,
>  {
>      int e;
>  
> -    libxl_device_disk_init(disk);
> -
>      if (!*config) {
>          *config = xlu_cfg_init(stderr, "command line");
>          if (!*config) { perror("xlu_cfg_init"); exit(-1); }
> @@ -3340,6 +3338,8 @@ static int cd_insert(uint32_t domid, const char
> *virtdev, char *phys)
>      xasprintf(&buf, "vdev=%s,access=r,devtype=cdrom,target=%s",
>                virtdev, phys ? phys : "");
>  
> +    libxl_device_disk_init(&disk);
> +
>      parse_disk_config(&config, buf, &disk);
>  
>      /* ATM the existence of the backing file is not checked for qdisk
> @@ -6649,6 +6649,7 @@ int main_blockattach(int argc, char **argv)
>      uint32_t fe_domid;
>      libxl_device_disk disk;
>      XLU_Config *config = 0;
> +    int rc = 0;
>  
>      SWITCH_FOREACH_OPT(opt, "", NULL, "block-attach", 2) {
>          /* No options */
> @@ -6660,6 +6661,8 @@ int main_blockattach(int argc, char **argv)
>      }
>      optind++;
>  
> +    libxl_device_disk_init(&disk);
> +
>      parse_disk_config_multistring
>          (&config, argc-optind, (const char* const*)argv + optind,
> &disk);
>  
> @@ -6668,14 +6671,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;
> +        goto out;
>      }
>  
>      if (libxl_device_disk_add(ctx, fe_domid, &disk, 0)) {
>          fprintf(stderr, "libxl_device_disk_add failed.\n");
> -        return 1;
> +        rc = 1;
>      }
> -    return 0;
> +out:
> +    libxl_device_disk_dispose(&disk);
> +
> +    return rc;
>  }
>  
>  int main_blocklist(int argc, char **argv)
> @@ -6726,18 +6732,23 @@ int main_blockdetach(int argc, char **argv)
>          /* No options */
>      }
>  
> +    libxl_device_disk_init(&disk);
> +
>      domid = find_domain(argv[optind]);
>  
>      if (libxl_vdev_to_device_disk(ctx, domid, argv[optind+1], &disk)) {
>          fprintf(stderr, "Error: Device %s not connected.\n",
> argv[optind+1]);
> -        return 1;
> +        rc = 1;
> +        goto out;
>      }
> -    rc = libxl_device_disk_remove(ctx, domid, &disk, 0);
> -    if (rc) {
> +    if (libxl_device_disk_remove(ctx, domid, &disk, 0)) {
>          fprintf(stderr, "libxl_device_disk_remove failed.\n");
> -        return 1;
> +        rc = 1;
>      }
> +
> +out:
>      libxl_device_disk_dispose(&disk);
> +
>      return rc;
>  }
>  

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