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