On Tue, Feb 05, 2013 at 12:56:12PM +0100, Ján Tomko wrote:
Branch by imgformat first, since we only add new features if -o backing_fmt is supported.
Switch to virBuffer for generating -o options to make adding new options easier.
The only change in the generated command line is movement of the -o/-F options to the end when creating images with backing stores. --- src/storage/storage_backend.c | 77 +++++++++++++++++-------------------------- 1 file changed, 31 insertions(+), 46 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index cab72c6..f9e604a 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -669,6 +669,8 @@ virStorageBackendCreateQemuImg(virConnectPtr conn, bool do_encryption = (vol->target.encryption != NULL); unsigned long long int size_arg; bool preallocate = false; + char *options = NULL; + virBuffer buf = VIR_BUFFER_INITIALIZER;
virCheckFlags(VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA, -1);
@@ -810,61 +812,44 @@ virStorageBackendCreateQemuImg(virConnectPtr conn, if (inputvol) { virCommandAddArgList(cmd, "convert", "-f", inputType, "-O", type, inputPath, vol->target.path, NULL); - - if (imgformat == QEMU_IMG_BACKING_FORMAT_OPTIONS && - (do_encryption || preallocate)) { - virCommandAddArg(cmd, "-o"); - virCommandAddArgFormat(cmd, "%s%s%s", do_encryption ? "encryption=on" : "", - (do_encryption && preallocate) ? "," : "", - preallocate ? "preallocation=metadata" : ""); - } else if (do_encryption) { - virCommandAddArg(cmd, "-e"); - } } else if (vol->backingStore.path) { + virCommandAddArgList(cmd, "create", "-f", type, "-b", + vol->backingStore.path, vol->target.path, NULL); + virCommandAddArgFormat(cmd, "%lluK", size_arg); + } else { virCommandAddArgList(cmd, "create", "-f", type, - "-b", vol->backingStore.path, NULL); + vol->target.path, NULL); + virCommandAddArgFormat(cmd, "%lluK", size_arg); + }
- 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.
- 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);
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.
- virCommandAddArg(cmd, vol->target.path); - virCommandAddArgFormat(cmd, "%lluK", size_arg); - if (do_encryption) - virCommandAddArg(cmd, "-e"); - } + VIR_FREE(options); } else { - virCommandAddArgList(cmd, "create", "-f", type, - vol->target.path, NULL); - virCommandAddArgFormat(cmd, "%lluK", size_arg); - - if (imgformat == QEMU_IMG_BACKING_FORMAT_OPTIONS && - (do_encryption || preallocate)) { - virCommandAddArg(cmd, "-o"); - virCommandAddArgFormat(cmd, "%s%s%s", do_encryption ? "encryption=on" : "", - (do_encryption && preallocate) ? "," : "", - preallocate ? "preallocation=metadata" : ""); - } else if (do_encryption) { - virCommandAddArg(cmd, "-e"); + if (!inputvol && vol->backingStore.path) { + if (imgformat == QEMU_IMG_BACKING_FORMAT_FLAG) + virCommandAddArgList(cmd, "-F", backingType, NULL); + else + VIR_INFO("Unable to set backing store format for %s with %s", + vol->target.path, create_tool);
s/INFO/DEBUG/
} + if (do_encryption) + virCommandAddArg(cmd, "-e"); }
ret = virStorageBackendCreateExecCommand(pool, vol, cmd);
In general, I'd be alot happier if we had a test case to cover the generation of the qemu-img command line args. ie separate out the creation of the 'virCommandPtr' instance, from the execution of it, and then have a test case which validates the args we have constructed. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|