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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list