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

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

Right now we add the -o encryption|preallocation or -e option args after
the positional arguments in every case except for "create" with
"-o backing_fmt", so it would be the first command line both with and
without my patch.

But yes, it would be nicer with options in the front and a test for it.

Jan


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