On 02/05/13 18:10, Daniel P. Berrange wrote:
> On Tue, Feb 05, 2013 at 12:56:12PM +0100, Ján Tomko wrote:
>>
>> - switch (imgformat) {
>> - case QEMU_IMG_BACKING_FORMAT_FLAG:
>> - virCommandAddArgList(cmd, "-F", backingType, vol->target.path,
>> - NULL);
>> - virCommandAddArgFormat(cmd, "%lluK", size_arg);
>> + if (imgformat == QEMU_IMG_BACKING_FORMAT_OPTIONS) {
>> + if (do_encryption)
>> + virBufferAddLit(&buf, ",encryption=on");
>>
>> - if (do_encryption)
>> - virCommandAddArg(cmd, "-e");
>> - break;
>> + if (!inputvol && vol->backingStore.path)
>> + virBufferAsprintf(&buf, ",backing_fmt=%s", backingType);
>> + else if (preallocate)
>> + virBufferAddLit(&buf, ",preallocation=metadata");
>
> This looks like it would result in "-o ,preallocation=metadata" if
> the do_encryption arg is false. I don't believe that will work.
Yes, all the options are added with a leading comma...
>
>>
>> - case QEMU_IMG_BACKING_FORMAT_OPTIONS:
>> - virCommandAddArg(cmd, "-o");
>> - virCommandAddArgFormat(cmd, "backing_fmt=%s%s", backingType,
>> - do_encryption ? ",encryption=on" : "");
>> - virCommandAddArg(cmd, vol->target.path);
>> - virCommandAddArgFormat(cmd, "%lluK", size_arg);
>> - break;
>> + if (virBufferError(&buf) > 0) {
>> + virReportOOMError();
>> + goto cleanup;
>> + }
>>
>> - default:
>> - VIR_INFO("Unable to set backing store format for %s with %s",
>> - vol->target.path, create_tool);
>> + if ((options = virBufferContentAndReset(&buf)))
>> + virCommandAddArgList(cmd, "-o", &(options[1]), NULL);
...which is ignored when the options are added to the argument list.
>
> IIUC the flow correctly this also means that instead of
>
> # qemu-img create -f qcow2 foo 10G -o preallocation=metadata
>
> the code now does
>
> # qemu-img create -f qcow2 -o preallocation=metadata foo 10G
>
> while it appears qemu-img currently allows for option args to
> come after positional args, this isn't a nice thing to rely
> on in general. It is the kind of silent behaviour that someting
> is likely to break.
>
Right now we add the -o encryption|preallocation or -e option args after
the positional arguments in every case except for "create" with
"-o backing_fmt", so it would be the first command line both with and
without my patch.
But yes, it would be nicer with options in the front and a test for it.
Jan
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list