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

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(a)redhat.com&gt; > --- > 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",