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

[John Ferlan]

On 04/18/2018 10:09 AM, Ján Tomko wrote:
> 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 at redhat.com>
>>> ---
>>>  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.
> 

Insufficient coffee, bad fingers... "1.1"

So, based on Dan's comment in my series for encryption - should we
"punt" and drop compat completely allowing qemu-img to decide or just
use "1.1" unconditionally?  Effects patch2 in this series.

John

>> 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);
>>>