Re: [Qemu-devel] [PATCH v25 11/31] change block layer to support both QemuOpts and QEMUOptionParamter

[Eric Blake]

On 04/28/2014 12:20 AM, Chun Yan Liu wrote:

>>> Isn't your conversion pair-wise per driver, in that you always pair  
>>> bdrv_create2 with options, and bdrv_create with opts? That is, won't  
>>> cco->opts always be false if cco->drv->bdrv_create2 is non-NULL, since  
>>> we already guaranteed that there is at most one of the two creation  
>>> function callbacks defined? Maybe you have a missing assertion at the  
>>> point where the callbacks are registered? [1]  
>>  
>> To handle both QEMUOptionParameter and QemuOpts, we convert 
>> QEMUOptionParameter to QemuOpts first for unified processing. 
>> And according to v22 discussion, it's better to convert back to  
>> QEMUOptionParameter at the last moment. 
>> https://lists.gnu.org/archive/html/qemu-devel/2014-03/msg02181.html. 
>> For bdrv_create, either do this in bdrv_create(), or here in 
>> bdrv_create_co_entry(). bdrv_create() just calls bdrv_create_co_entry 
>> to do the work. Do you think we should finish conversion in bdrv_create() 
>> before calling bdrv_create_co_entry()? 
>>
> 
> Eric, how do you think of this? Do we need a change?

At this point, it's up to you.  Stefan made the valid point that as long
as each patch works in isolation, it's not worth any additional churn to
add robustness to code that will be ripped out later in the series.

>>  
>> The work could also  be done as: 
>>  
>>        if (cco->options) { 
>>            opts_list = params_to_opts(cco->options); 
>>            cco->opts =  
>>                qemu_opts_create(opts_list, NULL, 0, &error_abort); 
>>        } 
>>        ret = cco->drv->bdrv_create2(cco->filename, cco->opts, &local_err); 
>>        if (cco->options) { 
>>            qemu_opts_del(cco->opts); 
>>            qemu_opts_free(opts_list); 
>>        }  
>>  
> 
> And here. Should we change it to  this way?

That way is actually fairly legible, and at this point, anything that
makes the patches easy to understand as being correct will help us get
the series applied sooner rather than later.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature