|
From: | Chunyan Liu |
Subject: | Re: [Qemu-devel] [PATCH v22 07/25] change block layer to support both QemuOpts and QEMUOptionParamter |
Date: | Wed, 12 Mar 2014 14:26:25 +0800 |
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 <address@hidden>
> Signed-off-by: Chunyan Liu <address@hidden>
> ---
> int bdrv_create(BlockDriver *drv, const char* filename,In addition to my remarks last night:
> - QEMUOptionParameter *options, Error **errp)
> + QEMUOptionParameter *options, QemuOpts *opts, Error **errp)
> {
> int ret;
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.
In addition to the memory leak I pointed out earlier,
> @@ -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);
I see another
potential problem: What if drv has been converted to QemuOpts, but
proto_drv is still on QEMUOptionParameter?
Or the converse, if drv is still on QEMUOptionParameter, but proto_drv
> + } else {
> + create_options = append_option_parameters(create_options,
> + drv->create_options);
> + create_options = append_option_parameters(create_options,
> + proto_drv->create_options);
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.
I think a better path to conversion would be doing everything possible
> + create_opts = params_to_opts(create_options);
> + }
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);
>This part worked nicely - once you convert both incoming styles to
> /* 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);
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,...and finally convert back to QEMUOptionParameters at the last moment.
>
> 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);
> + }
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.
Similar to the create/create2 situation, _this_ function is the last
> -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);
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) {if (bs->drv->bdrv_amend_options2) {
return -ENOTSUP;
}
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;
I like the FIXME here; maybe also mention that bdrv_create and
> +++ 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);
bdrv_create2 are mutually exclusive (at most one can be non-NULL).
A similar FIXME comment here might be nice, likewise mentioning that at
> 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;
most one of these two fields should be non-NULL.
Memory leak.
> @@ -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);
Another case where if you add a new helper function that absorbs either
> + } 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);
> + }
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) {More memory leaks.
> + create_opts = qemu_opts_append(create_opts, drv->create_opts);
> + create_opts = qemu_opts_append(create_opts, proto_drv->create_opts);
Another case where you should just always pass opts to bdrv_create(),
> @@ -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);
> + }
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.
And again, you should not be making decisions on the contents of
> @@ -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) {
bs->drv, but just blindly setting up QemuOpts here...
...and blindly letting bdrv_amend_options() be the place that converts
> + 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);
to QEMUOptionParameters as needed.
Here's hoping v23 is nicer; I'm looking forward to ditching
QEMUOptionParameters.
[Prev in Thread] | Current Thread | [Next in Thread] |