Re: [Qemu-devel] [PATCH v22 07/25] change block layer to support both QemuOpts and QEMUOptionParamter

[Chunyan Liu <cyliu@suse.com>]

[-- Attachment #1: Type: text/plain, Size: 12498 bytes --]

2014-03-12 0:54 GMT+08:00 Eric Blake <eblake@redhat.com>:

> On 03/10/2014 01:31 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 <wdongxu@linux.vnet.ibm.com>
> > Signed-off-by: Chunyan Liu <cyliu@suse.com>
> > ---
>
> >  int bdrv_create(BlockDriver *drv, const char* filename,
> > -    QEMUOptionParameter *options, Error **errp)
> > +    QEMUOptionParameter *options, QemuOpts *opts, Error **errp)
> >  {
> >      int ret;
>
> In addition to my remarks last night:
>
> I wonder if you should add:
>
> assert(!(options && opts))
>
> to prove that all callers are passing at most one of the two supported
> option lists, while doing the conversion.
>
> > @@ -5248,28 +5266,31 @@ void bdrv_img_create(const char *filename, const
> char *fmt,
> >          return;
> >      }
> >
> > -    create_options = append_option_parameters(create_options,
> > -                                              drv->create_options);
> > -    create_options = append_option_parameters(create_options,
> > -
>  proto_drv->create_options);
> > +    if (drv->bdrv_create2) {
> > +        create_opts = qemu_opts_append(create_opts, drv->create_opts);
> > +        create_opts = qemu_opts_append(create_opts,
> proto_drv->create_opts);
>
> In addition to the memory leak I pointed out earlier,


Should use g_realloc in qemu_opts_append, realloc create_opts
to the desired space and append 2nd list to it. Saving the effort to free
memory while append many times.

I see another
> potential problem: What if drv has been converted to QemuOpts, but
> proto_drv is still on QEMUOptionParameter?
>

> > +    } else {
> > +        create_options = append_option_parameters(create_options,
> > +                                                  drv->create_options);
> > +        create_options = append_option_parameters(create_options,
> > +
>  proto_drv->create_options);
>
> Or the converse, if drv is still on QEMUOptionParameter, but proto_drv
> has been converted to QEMUOpts?
>
> Either way, you will be appending NULL for the proto_drv case, when what
> you really wanted to be doing was merging both the QemuOpts and
> QEMUOptionParameters into a single list.
>
> > +        create_opts = params_to_opts(create_options);
> > +    }
>
> I think a better path to conversion would be doing everything possible
> in QemuOpts, and only switching to QEMUOptionParameters at the last
> possible moment, rather than having two separate append code paths.
>
> My suggestion: until the conversion is complete, add a new function,
> something like:
>
> void qemu_opts_append_pair(QemuOpts *dst, QemuOpts *opts,
>                            QEMUOptionParameter *options) {
>     assert(!(opts && options));
>     ... modify dst in-place to have it cover the entries from opts or
> options
> }
>
> then in this code, you do:
>     create_options = /* generate empty QemuOpts */;
>     qemu_opts_append_pair(options, drv->create_opts,
>                           drv->create_options);
>     qemu_opts_append_pair(options, proto_drv->create_options,
>                           proto_drv->create_options);
>
>
Good. Together with always using QemuOpts and only converting to
QEMUOptionParameter until it calls .bdrv_create in specific driver,
the drv/proto_drv inconsistent problem could be solved.
Will update.

Thanks very much for all your suggestions!

Chunyan

>
> >      /* Create parameter list with default values */
> > -    param = parse_option_parameters("", create_options, param);
> > -
> > -    set_option_parameter_int(param, BLOCK_OPT_SIZE, img_size);
> > +    opts = qemu_opts_create(create_opts, NULL, 0, &error_abort);
> > +    qemu_opt_set_number(opts, BLOCK_OPT_SIZE, img_size);
>
> This part worked nicely - once you convert both incoming styles to
> QemuOpts, it really is easier to have this part of the code just use
> QemuOpts functions for setting the size....
>
> > ...
> > @@ -5343,16 +5364,23 @@ void bdrv_img_create(const char *filename, const
> char *fmt,
> >
> >      if (!quiet) {
> >          printf("Formatting '%s', fmt=%s ", filename, fmt);
> > -        print_option_parameters(param);
> > +        qemu_opts_print(opts);
> >          puts("");
> >      }
> > -    ret = bdrv_create(drv, filename, param, &local_err);
> > +
> > +    if (drv->bdrv_create2) {
> > +        ret = bdrv_create(drv, filename, NULL, opts, &local_err);
> > +    } else {
> > +        param = opts_to_params(opts);
> > +        ret = bdrv_create(drv, filename, param, NULL, &local_err);
> > +    }
>
> ...and finally convert back to QEMUOptionParameters at the last moment.
>  But wait a minute - why are you checking for drv->bdrv_create2 here?
> Isn't the last possible moment really inside bdrv_create() (that is, the
> actual function that decides whether to call drv->bdrv_create vs.
> drv->bdrv_create2)?  For the code to be as clean as possible, you want
> to use QemuOpts everywhere until the actual point where you are calling
> the unconverted callback.
>
>
> > -int bdrv_amend_options(BlockDriverState *bs, QEMUOptionParameter
> *options)
> > +int bdrv_amend_options(BlockDriverState *bs, QEMUOptionParameter
> *options,
> > +                       QemuOpts *opts)
> >  {
> > -    if (bs->drv->bdrv_amend_options == NULL) {
> > +    if (!bs->drv->bdrv_amend_options && !bs->drv->bdrv_amend_options2) {
> >          return -ENOTSUP;
> >      }
> > -    return bs->drv->bdrv_amend_options(bs, options);
> > +    if (bs->drv->bdrv_amend_options2) {
> > +        return bs->drv->bdrv_amend_options2(bs, opts);
> > +    } else {
> > +        return bs->drv->bdrv_amend_options(bs, options);
>
> Similar to the create/create2 situation, _this_ function is the last
> possible place to make the conversions.  You have a problem in that if
> the user passes you options but the callback expects opts, you are
> passing NULL to the callback.  I think this should look more like:
>
> int bdrv_amend_options(BlockDriverState *bs, QEMUOptionParameter *options,
>                        QemuOpts *opts)
> {
>     int ret;
>     assert(!(opts && options));
>     if (!bs->drv->bdrv_amend_options && !bs->drv->bdrv_amend_options2) {
>         return -ENOTSUP;
>     }
>     if (bs->drv->bdrv_amend_options2) {
>         if (options) {
>             opts = params_to_opts(options);
>         }
>         ret = bs->drv->bdrv_amend_options2(bs, opts);
>         if (options) {
>             g_free(opts); // or whatever correct spelling to avoid leak
>         }
>     } else {
>         if (opts) {
>             options = opts_to_params(opts);
>         }
>         ret = bs->drv->bdrv_amend_options(bs, options);
>         if (opts) {
>             g_free(options); // again, with correct spelling
>         }
>     }
>     return ret;
>
> > +++ 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);
>
> I like the FIXME here; maybe also mention that bdrv_create and
> bdrv_create2 are mutually exclusive (at most one can be non-NULL).
>
> >      int (*bdrv_set_key)(BlockDriverState *bs, const char *key);
> >      int (*bdrv_make_empty)(BlockDriverState *bs);
> >      /* aio */
> > @@ -217,7 +219,7 @@ struct BlockDriver {
> >
> >      /* List of options for creating images, terminated by name == NULL
> */
> >      QEMUOptionParameter *create_options;
> > -
> > +    QemuOptsList *create_opts;
>
> A similar FIXME comment here might be nice, likewise mentioning that at
> most one of these two fields should be non-NULL.
>
> > @@ -244,21 +245,34 @@ static int print_block_option_help(const char
> *filename, const char *fmt)
> >          return 1;
> >      }
> >
> > -    create_options = append_option_parameters(create_options,
> > -                                              drv->create_options);
> > -
> > +    if (drv->create_opts) {
> > +        create_opts = qemu_opts_append(create_opts, drv->create_opts);
> > +    } else {
> > +        create_options = append_option_parameters(create_options,
> > +                                                  drv->create_options);
> > +    }
> >      if (filename) {
> >          proto_drv = bdrv_find_protocol(filename, true);
> >          if (!proto_drv) {
> >              error_report("Unknown protocol '%s'", filename);
> >              return 1;
> >          }
> > -        create_options = append_option_parameters(create_options,
> > -
>  proto_drv->create_options);
> > +        if (drv->create_opts) {
> > +            create_opts = qemu_opts_append(create_opts,
> proto_drv->create_opts);
>
> Memory leak.
>
> > +        } else {
> > +            create_options =
> > +                append_option_parameters(create_options,
> > +                                         proto_drv->create_options);
> > +        }
> >      }
> >
> > -    print_option_help(create_options);
> > +    if (drv->create_opts) {
> > +        qemu_opts_print_help(create_opts);
> > +    } else {
> > +        print_option_help(create_options);
> > +    }
>
> Another case where if you add a new helper function that absorbs either
> style of options into a QemuOpts, then all you have to do here is print
> the QemuOpts, instead of trying to switch between print methods here.
>
>
> > @@ -1340,40 +1356,42 @@ static int img_convert(int argc, char **argv)
> >          goto out;
> >      }
>
> > -        }
> > +    if (drv->bdrv_create2) {
> > +        create_opts = qemu_opts_append(create_opts, drv->create_opts);
> > +        create_opts = qemu_opts_append(create_opts,
> proto_drv->create_opts);
>
> More memory leaks.
>
>
> > @@ -1400,7 +1418,12 @@ static int img_convert(int argc, char **argv)
> >
> >      if (!skip_create) {
> >          /* Create the new image */
> > -        ret = bdrv_create(drv, out_filename, param, &local_err);
> > +        if (drv->bdrv_create2) {
> > +            ret = bdrv_create(drv, out_filename, NULL, opts,
> &local_err);
> > +        } else {
> > +            param = opts_to_params(opts);
> > +            ret = bdrv_create(drv, out_filename, param, NULL,
> &local_err);
> > +        }
>
> Another case where you should just always pass opts to bdrv_create(),
> and let bdrv_create() do the last minute conversion to
> QEMUOptionParameters based on what callbacks drv supports, rather than
> trying to make decisions here based on contents of drv.
>
>
> > @@ -2721,17 +2748,26 @@ static int img_amend(int argc, char **argv)
> >          goto out;
> >      }
> >
> > -    create_options = append_option_parameters(create_options,
> > -            bs->drv->create_options);
> > -    options_param = parse_option_parameters(options, create_options,
> > -            options_param);
> > -    if (options_param == NULL) {
> > +    if (bs->drv->bdrv_amend_options2) {
>
> And again, you should not be making decisions on the contents of
> bs->drv, but just blindly setting up QemuOpts here...
>
> > +        create_opts = qemu_opts_append(create_opts,
> bs->drv->create_opts);
> > +    } else {
> > +        create_options = append_option_parameters(create_options,
> > +
>  bs->drv->create_options);
> > +        create_opts = params_to_opts(create_options);
> > +    }
> > +    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'", fmt);
> >          ret = -1;
> >          goto out;
> >      }
> >
> > -    ret = bdrv_amend_options(bs, options_param);
> > +    if (bs->drv->bdrv_amend_options2) {
> > +        ret = bdrv_amend_options(bs, NULL, opts);
> > +    } else {
> > +        options_param = opts_to_params(opts);
> > +        ret = bdrv_amend_options(bs, options_param, NULL);
>
> ...and blindly letting bdrv_amend_options() be the place that converts
> to QEMUOptionParameters as needed.
>
> Here's hoping v23 is nicer; I'm looking forward to ditching
> QEMUOptionParameters.
>

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

[-- Attachment #2: Type: text/html, Size: 16909 bytes --]