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

[George Dunlap <George.Dunlap@eu.citrix.com>]

On Tue, Dec 1, 2015 at 11:53 AM, George Dunlap
<george.dunlap@eu.citrix.com> 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@eu.citrix.com>

Ping?
 -George

> ---
> 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;
>  }
>
> --
> 2.1.4
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel