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, 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); > > /* 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
signature.asc
Description: OpenPGP digital signature