On 06/06/2016 09:37 AM, Peter Krempa wrote:
> 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@xxxxxxxxxx>
>> ---
>> 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?
>
It wasn't clear if tests/storagevolxml2argvtest.c should then be
removed, adjusted, or otherwise. It replicated the flags I put the X_ in
front of as:
enum {
FMT_NONE = 0,
FMT_FLAG,
FMT_OPTIONS,
FMT_COMPAT,
};
Whether we care or not to support/test all those old options was unclear
to me. I would think at some point in time qemu-img would drop those in
preference for the new arguments.
I you think essentially doing away with all those old options is fine, I
can certain do/add that.
John
>> virCommandAddArgList(cmd, "-F", backingType, NULL);
>> else
>> VIR_DEBUG("Unable to set backing store format for %s with %s",
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list