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