[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v2 1/6] storage: refactor qemu-img command line generation



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 :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]