[libvirt] [PATCH 4/7] storage: Adjust qemu-img switches check

[Peter Krempa]

On Fri, Jun 03, 2016 at 06:42:09 -0400, John Ferlan wrote:
> Since we support QEMU 0.12 and later, checking for support of specific flags
> added prior to that isn't necessary.
> 
> Thus start with the base of having the "-o options" available for the
> qemu-img create option and then determine whether we have the compat
> option for qcow2 files (which would be necessary up through qemu 2.0
> where the default changes to compat 0.11).
> 
> NOTE: Keeping old tests around since it's still possible to create in
> the old format.
> 
> Signed-off-by: John Ferlan <jferlan at redhat.com>
> ---
>  src/storage/storage_backend.c | 66 ++++++++++++++++---------------------------
>  1 file changed, 24 insertions(+), 42 deletions(-)
> 
> diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
> index 2076155..4c40e43 100644
> --- a/src/storage/storage_backend.c
> +++ b/src/storage/storage_backend.c
> @@ -869,9 +869,19 @@ virStoragePloopResize(virStorageVolDefPtr vol,
>      return ret;
>  }
>  
> +/* Flag values shared w/ storagevolxml2argvtest.c.
> + * Since it's still possible to provide the old format args, just
> + * keep them; however, prefix with an "X_" (similar to qemu_capabilities.c)
> + * to indicate the are older.

We use that approach as we can't delete the already existing flags since
we wouldn't be able to parse the status XML. For qemu-img we don't store
the flags anywhere so the old ones can be deleted.

> + *
> + * QEMU_IMG_BACKING_FORMAT_OPTIONS (added in qemu 0.11)
> + * QEMU_IMG_BACKING_FORMAT_OPTIONS_COMPAT
> + *    was made necessary due to 2.0 change to change the default
> + *    qcow2 file format from 0.10 to 1.1.
> + */
>  enum {
> -    QEMU_IMG_BACKING_FORMAT_NONE = 0,
> -    QEMU_IMG_BACKING_FORMAT_FLAG,
> +    X_QEMU_IMG_BACKING_FORMAT_NONE = 0,

So this can be dropped entirely after the hunk below.

> +    X_QEMU_IMG_BACKING_FORMAT_FLAG,

This will be never set either after the hunk below.

>      QEMU_IMG_BACKING_FORMAT_OPTIONS,
>      QEMU_IMG_BACKING_FORMAT_OPTIONS_COMPAT,
>  };
> @@ -904,46 +914,18 @@ virStorageBackendQemuImgSupportsCompat(const char *qemuimg)
>  static int
>  virStorageBackendQEMUImgBackingFormat(const char *qemuimg)
>  {
> -    char *help = NULL;
> -    char *start;
> -    char *end;
> -    char *tmp;
> -    int ret = -1;
> -    int exitstatus;
> -    virCommandPtr cmd = virCommandNewArgList(qemuimg, "-h", NULL);
> -
> -    virCommandAddEnvString(cmd, "LC_ALL=C");
> -    virCommandSetOutputBuffer(cmd, &help);
> -    virCommandClearCaps(cmd);
> -
> -    /* qemuimg doesn't return zero exit status on -h,
> -     * therefore we need to provide pointer for storing
> -     * exit status, although we don't parse it any later */
> -    if (virCommandRun(cmd, &exitstatus) < 0)
> -        goto cleanup;
> +    /* As of QEMU 0.11 the [-o options] support was added via qemu
> +     * commit id '9ea2ea71', so we start with that base and figure
> +     * out what else we have */
> +    int ret = QEMU_IMG_BACKING_FORMAT_OPTIONS;

So this flag can basically be completely removed as it should be always
present in qemu-img 0.12

> +
> +    /* QEMU 2.0 changed to using a format that only QEMU 1.1 and newer
> +     * understands. Since we still support QEMU 0.12 and newer, we need
> +     * to be able to handle the previous format as can be set via a
> +     * compat=0.10 option. */
> +    if (virStorageBackendQemuImgSupportsCompat(qemuimg))
> +        ret = QEMU_IMG_BACKING_FORMAT_OPTIONS_COMPAT;

Yup, we need this.

>  
> -    if ((start = strstr(help, " create ")) == NULL ||
> -        (end = strstr(start, "\n")) == NULL) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR,
> -                       _("unable to parse qemu-img output '%s'"),
> -                       help);
> -        goto cleanup;
> -    }
> -    if (((tmp = strstr(start, "-F fmt")) && tmp < end) ||
> -        ((tmp = strstr(start, "-F backing_fmt")) && tmp < end)) {
> -        ret = QEMU_IMG_BACKING_FORMAT_FLAG;
> -    } else if ((tmp = strstr(start, "[-o options]")) && tmp < end) {
> -        if (virStorageBackendQemuImgSupportsCompat(qemuimg))
> -            ret = QEMU_IMG_BACKING_FORMAT_OPTIONS_COMPAT;
> -        else
> -            ret = QEMU_IMG_BACKING_FORMAT_OPTIONS;
> -    } else {
> -        ret = QEMU_IMG_BACKING_FORMAT_NONE;
> -    }
> -
> - cleanup:
> -    virCommandFree(cmd);
> -    VIR_FREE(help);
>      return ret;
>  }
>  
> @@ -1219,7 +1201,7 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn,
>          VIR_FREE(opts);
>      } else {
>          if (info.backingPath) {
> -            if (imgformat == QEMU_IMG_BACKING_FORMAT_FLAG)
> +            if (imgformat == X_QEMU_IMG_BACKING_FORMAT_FLAG)


So ... if the above is never set, why aren't you dropping this entire
case?

>                  virCommandAddArgList(cmd, "-F", backingType, NULL);
>              else
>                  VIR_DEBUG("Unable to set backing store format for %s with %s",