2014-03-12 0:54 GMT+08:00 Eric Blake <ebl...@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 <wdon...@linux.vnet.ibm.com>
> > Signed-off-by: Chunyan Liu <cy...@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
>
>
