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

[Daniel P. Berrange]

On Wed, Feb 06, 2013 at 12:24:50PM +0100, Ján Tomko wrote:
> 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.

Ewww, this is gross & really obscure. Add the comma at the end, and
then use 'virBufferTrim' after the last one, along with a comment
indicating why you are doing this.

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