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

[Chun Yan Liu]

>>> 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()?

>  
> > +            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);
       } 

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