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

[Chun Yan Liu]

>>> On 4/23/2014 at 02:44 PM, in message <53576153.6CE : 102 : 21807>, Chun Yan 
>>> Liu
wrote: 

>  
>>>> On 4/22/2014 at 07:17 AM, in message <address@hidden>, Eric Blake 
> <address@hidden> wrote:  
> > On 04/10/2014 11:54 AM, Chunyan Liu wrote:  
>> > Change block layer to support both QemuOpts and QEMUOptionParameter.  
>> > After this patch, it will change backend drivers one by one. At the end,  
>> > QEMUOptionParameter will be removed and only QemuOpts is kept.  
>> >   
>> > Signed-off-by: Dong Xu Wang <address@hidden>  
>> > Signed-off-by: Chunyan Liu <address@hidden>  
>> > ---  
>>   
>> > @@ -419,8 +420,27 @@ static void coroutine_fn bdrv_create_co_entry(void   
>> *opaque)  
>> >    
>> >      CreateCo *cco = opaque;  
>>   
>> > +    if (cco->drv->bdrv_create2) {  
>> > +        QemuOptsList *opts_list = NULL;  
>> > +        QemuOpts *opts = NULL;  
>> > +        if (!cco->opts) {  
>>   
>> 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?

>>   
>> > +            opts_list = params_to_opts(cco->options);  
>> > +            cco->opts = opts =  
>> > +                qemu_opts_create(opts_list, NULL, 0, &error_abort);  
>>   
>> Ouch.  This assigns to cco->opts,...  
>>   
>> > +        }  
>> > +        ret = cco->drv->bdrv_create2(cco->filename, cco->opts, 
>> > &local_err);  
>> > +        qemu_opts_del(opts);  
>>   
>> ...but this frees the memory.  You have modified the caller's opaque  
>> pointer to point to what is now stale memory.  Is this intentional? [2] 
>   
> Yes. My intension  is  to avoid deleting cco->opts errorly, only the  
> converted 
> one from cco->options should be deleted,  so I use a temp variable to  
> retrieve 
> the new allocated opts, just new and delete the temp variable is OK. 
>  
> 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?
I'll update and prepare a new version.

Regards,
Chunyan

>>   
>> > +        qemu_opts_free(opts_list);  
>> > +    } else {  
>> > +        QEMUOptionParameter *options = NULL;  
>> > +        if (!cco->options) {  
>> > +            cco->options = options = opts_to_params(cco->opts);  
>> > +        }  
>> > +        ret = cco->drv->bdrv_create(cco->filename, cco->options,  
> &local_err);  
>> > +        free_option_parameters(options);  
>>   
>> Same question [2] in the opposite direction for cco->options.  
>>   
>>   
>>   
>> > @@ -5296,28 +5328,25 @@ void bdrv_img_create(const char *filename, const   
>> char *fmt,  
>>   
>> >      /* Parse -o options */  
>> >      if (options) {  
>> > -        param = parse_option_parameters(options, create_options, param);  
>> > -        if (param == NULL) {  
>> > +        if (qemu_opts_do_parse(opts, options, NULL) != 0) {  
>> >              error_setg(errp, "Invalid options for file format '%s'.",   
>> fmt);  
>>   
>> Pre-existing (and probably worth an independent patch to qemu-trivial),  
>> but error messages typically should not end in '.'  
>  
> Will add new patch for that. 
>  
>>   
>> > +int bdrv_amend_options(BlockDriverState *bs, QEMUOptionParameter 
>> > *options,  
>> > +                       QemuOpts *opts)  
>> >  {  
>> > -    if (bs->drv->bdrv_amend_options == NULL) {  
>> > +    int ret;  
>> > +    assert(!(options && opts));  
>> > +  
>> > +    if (!bs->drv->bdrv_amend_options && !bs->drv->bdrv_amend_options2) {  
>>   
>> Hmm, should we also guarantee that at most one of these callback  
>> functions exist?  [1]  
>>   
>> >          return -ENOTSUP;  
>> >      }  
>> > -    return bs->drv->bdrv_amend_options(bs, options);  
>> > +    if (bs->drv->bdrv_amend_options2) {  
>> > +        QemuOptsList *tmp_opts_list = NULL;  
>> > +        QemuOpts *tmp_opts = NULL;  
>> > +        if (options) {  
>> > +            tmp_opts_list = params_to_opts(options);  
>> > +            opts = tmp_opts =  
>> > +                qemu_opts_create(tmp_opts_list, NULL, 0, &error_abort);  
>> > +        }  
>> > +        ret = bs->drv->bdrv_amend_options2(bs, opts);  
>> > +        qemu_opts_del(tmp_opts);  
>>   
>> Why do you need tmp_opts?  Isn't manipulation of 'opts' sufficient here?  
>  
> Same as answer to [2]. The intension is to avoid  deleting opts errorly. 
>  
>>   
>> > +++ b/include/block/block_int.h  
>> > @@ -118,6 +118,8 @@ struct BlockDriver {  
>> >      void (*bdrv_rebind)(BlockDriverState *bs);  
>> >      int (*bdrv_create)(const char *filename, QEMUOptionParameter 
>> > *options,  
>> >                         Error **errp);  
>> > +    /* FIXME: will remove the duplicate and rename back to bdrv_create   
>> later */  
>> > +    int (*bdrv_create2)(const char *filename, QemuOpts *opts, Error   
>> **errp);  
>> >      int (*bdrv_set_key)(BlockDriverState *bs, const char *key);  
>> >      int (*bdrv_make_empty)(BlockDriverState *bs);  
>> >      /* aio */  
>> > @@ -217,7 +219,10 @@ struct BlockDriver {  
>> >    
>> >      /* List of options for creating images, terminated by name == NULL */ 
>> >  
>> >      QEMUOptionParameter *create_options;  
>> > -  
>> > +    /* FIXME: will replace create_options.  
>> > +     * These two fields are mutually exclusive. At most one is non-NULL.  
>> > +     */  
>> > +    QemuOptsList *create_opts;  
>>   
>> Do you also want to mention that create_options may only be set with  
>> bdrv_create, and that create_opts may only be set with bdrv_create2? [1]  
>  
> Will add description. 
>  
>>   
>>   
>> > @@ -1340,40 +1340,36 @@ static int img_convert(int argc, char **argv)  
>>   
>> >    
>> > -    if (options) {  
>> > -        param = parse_option_parameters(options, create_options, param);  
>> > -        if (param == NULL) {  
>> > -            error_report("Invalid options for file format '%s'.",   
>> out_fmt);  
>> > -            ret = -1;  
>> > -            goto out;  
>> > -        }  
>> > -    } else {  
>> > -        param = parse_option_parameters("", create_options, param);  
>> > +    opts = qemu_opts_create(create_opts, NULL, 0, &error_abort);  
>> > +    if (options && qemu_opts_do_parse(opts, options, NULL)) {  
>> > +        error_report("Invalid options for file format '%s'.", out_fmt);  
>>   
>> Pre-existing, but another instance of trailing '.' in the error message.  
>>   
>> Back to [1], rather than asserting in individual functions at use time,  
>> why not do all the sanity checking up front in bdrv_register()?  That  
>> is, I think it is easier to add:  
>>   
>> if (bdrv->bdrv_create) {  
>>     assert(!bdrv->bdrv_create2);  
>>     assert(!bdrv->opts && !bdrv->bdrv_amend_options2);  
>> } else if (bdrv_{  
>>     assert(!bdrv->options && !bdrv->bdrv_amend_options);  
>> }  
>>   
>> at the point where drivers are registered, rather than having to  
>> duplicate checks across all points that might use a driver.  
>  
> That makes sense. But [1] still needs the asserting individually, since 
> it doesn't use bdrv->create_opts or bdrv->create_options, but the opts or 
> options passed in.  
>  
>>   
>> --   
>> Eric Blake   eblake redhat com    +1-919-301-3266  
>> Libvirt virtualization library http://libvirt.org  
>>   
>>   
>