Re: [libvirt] [PATCH 3/3] storage: remove qemu-img help scraping

On Wed, Apr 18, 2018 at 08:24:49AM -0400, John Ferlan wrote: > > > On 04/17/2018 05:43 PM, Ján Tomko wrote: >> We have been checking whether qemu-img supports the -o compat >> option by scraping the -help output. >> >> Since we require QEMU 1.5.0 now and this option was introduced in 1.1, >> assume we support it and ditch the help parsing code along with the >> extra qemu-img invocation. >> >> Signed-off-by: Ján Tomko <jtomko(a)redhat.com&gt; >> --- >>  src/storage/storage_util.c     | 73 >> +++--------------------------------------- >>  src/storage/storage_util.h     |  1 - >>  tests/storagevolxml2argvtest.c |  5 ++- >>  3 files changed, 6 insertions(+), 73 deletions(-) >> >> diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c >> index 897dfdaae..f7a4231e2 100644 >> --- a/src/storage/storage_util.c >> +++ b/src/storage/storage_util.c >> @@ -787,61 +787,6 @@ storagePloopResize(virStorageVolDefPtr vol, >>      return ret; >>  } >> >> -/* Flag values shared w/ storagevolxml2argvtest.c. >> - * >> - * 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_OPTIONS = 0, >> -    QEMU_IMG_BACKING_FORMAT_OPTIONS_COMPAT, >> -}; >> - >> -static bool >> -virStorageBackendQemuImgSupportsCompat(const char *qemuimg) >> -{ >> -    bool ret = false; >> -    char *output; >> -    virCommandPtr cmd = NULL; >> - >> -    cmd = virCommandNewArgList(qemuimg, "create", "-o", "?", "-f", >> "qcow2", >> -                               "/dev/null", NULL); >> - >> -    virCommandAddEnvString(cmd, "LC_ALL=C"); >> -    virCommandSetOutputBuffer(cmd, &output); >> - >> -    if (virCommandRun(cmd, NULL) < 0) >> -        goto cleanup; >> - >> -    if (strstr(output, "\ncompat ")) >> -        ret = true; >> - >> - cleanup: >> -    virCommandFree(cmd); >> -    VIR_FREE(output); >> -    return ret; >> -} >> - >> - >> -static int >> -virStorageBackendQEMUImgBackingFormat(const char *qemuimg) >> -{ >> -    /* 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; >> - >> -    /* 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; >> - >> -    return ret; >> -} >> >>  /* The _virStorageBackendQemuImgInfo separates the command line >> building from >>   * the volume definition so that >> qemuDomainSnapshotCreateInactiveExternal can >> @@ -1089,14 +1034,12 @@ >> storageBackendCreateQemuImgSetBacking(virStoragePoolObjPtr pool, >> >>  static int >>  storageBackendCreateQemuImgSetOptions(virCommandPtr cmd, >> -                                      int imgformat, >>                                        virStorageEncryptionInfoDefPtr >> enc, >>                                        struct >> _virStorageBackendQemuImgInfo info) >>  { >>      char *opts = NULL; >> >> -    if (info.format == VIR_STORAGE_FILE_QCOW2 && !info.compat && >> -        imgformat >= QEMU_IMG_BACKING_FORMAT_OPTIONS_COMPAT) >> +    if (info.format == VIR_STORAGE_FILE_QCOW2 && !info.compat) >>          info.compat = "0.10"; > > The comments for _COMPAT indicate that 0.11 was supported in QEMU 1.1 > and that QEMU 2.0 started using that as the default. So, since this > series is predicated upon QEMU 1.5 being the new default, why isn't this > "0.11"? > I am not sure what you're asking here. Per the comment above, 0.11 is the version where you could specify the backing format via -o backing_fmt. Before that, we used the -F option, but we dropped that in commit f6a92f8e. 1.1 is the version that introduced qcow2v3, so it's the first one to support the compat option. In 2.0, qemu changed the default compat from 0.10 to 1.1 and libvirt started always specifying -o compat to preserve XML->qcow2 format compatibility (qcow2v3 needs to be requested explicitly via <compat>1.1</compat>). No idea where you got "0.11" from.

> For those installations stuck somewhere between 1.5 and 2.0, they could > provide the "0.10" on the command line still > >> >>      if (storageBackendCreateQemuImgOpts(enc, &opts, info) < 0) >> @@ -154,7 +153,7 @@ testCompareXMLToArgvHelper(const void *data) >>      result = testCompareXMLToArgvFiles(info->shouldFail, poolxml, >> volxml, >>                                         inputpoolxml, inputvolxml, >>                                         cmdline, info->flags, >> -                                       info->imgformat, >> info->parseflags); >> +                                       info->parseflags); > > This effectively removes the need for info->imgformat and thus makes > FMT_OPTIONS and FMT_COMPAT unnecessary.  IOW: Those need to be removed > from testInfo and the DO_TEST_FULL @define. Right. Jano > > John > >> >>   cleanup: >>      VIR_FREE(poolxml); >>