From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43933) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WNccY-0001ok-LR for qemu-devel@nongnu.org; Wed, 12 Mar 2014 02:26:33 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WNccV-0007Fu-Pt for qemu-devel@nongnu.org; Wed, 12 Mar 2014 02:26:30 -0400 Received: from mail-wi0-x22e.google.com ([2a00:1450:400c:c05::22e]:38539) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WNccV-0007Fl-8E for qemu-devel@nongnu.org; Wed, 12 Mar 2014 02:26:27 -0400 Received: by mail-wi0-f174.google.com with SMTP id d1so1894267wiv.1 for ; Tue, 11 Mar 2014 23:26:26 -0700 (PDT) MIME-Version: 1.0 Sender: kgrace.liu@gmail.com In-Reply-To: <531F3FAC.3010606@redhat.com> References: <1394436721-21812-1-git-send-email-cyliu@suse.com> <1394436721-21812-8-git-send-email-cyliu@suse.com> <531F3FAC.3010606@redhat.com> Date: Wed, 12 Mar 2014 14:26:25 +0800 Message-ID: From: Chunyan Liu Content-Type: multipart/alternative; boundary=f46d044283b08d291404f462ea6c Subject: Re: [Qemu-devel] [PATCH v22 07/25] change block layer to support both QemuOpts and QEMUOptionParamter List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: Kevin Wolf , Dong Xu Wang , qemu-devel@nongnu.org, stefanha@redhat.com --f46d044283b08d291404f462ea6c Content-Type: text/plain; charset=UTF-8 2014-03-12 0:54 GMT+08:00 Eric Blake : > 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 > > Signed-off-by: Chunyan Liu > > --- > > > 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 > > --f46d044283b08d291404f462ea6c Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable



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 en= d,
> 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>
> ---

> =C2=A0int bdrv_create(BlockDriver *drv, const ch= ar* filename,
> - =C2=A0 =C2=A0QEMUOptionParameter *options, Error **errp)
> + =C2=A0 =C2=A0QEMUOptionParameter *options, QemuOpts *opts, Error **e= rrp)
> =C2=A0{
> =C2=A0 =C2=A0 =C2=A0int 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, con= st char *fmt,
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return;
> =C2=A0 =C2=A0 =C2=A0}
>
> - =C2=A0 =C2=A0create_options =3D append_option_parameters(create_opti= ons,
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0drv->create_options);
> - =C2=A0 =C2=A0create_options =3D append_option_parameters(create_opti= ons,
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0proto_drv->create_options);
> + =C2=A0 =C2=A0if (drv->bdrv_create2) {
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0create_opts =3D qemu_opts_append(create_o= pts, drv->create_opts);
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0create_opts =3D qemu_opts_append(create_o= pts, 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 t= he 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?=C2=A0

> + =C2=A0 =C2=A0} else {
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0create_options =3D append_option_paramete= rs(create_options,
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0drv->create_options);
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0create_options =3D append_option_paramete= rs(create_options,
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0proto_drv->create_options);

Or the converse, if drv is still on QEMUOptionParameter, but proto_dr= v
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.

> + =C2=A0 =C2=A0 =C2=A0 =C2=A0create_opts =3D params_to_opts(create_opt= ions);
> + =C2=A0 =C2=A0}

I think a better path to conversion would be doing everything possibl= e
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,
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0QEMUOptionParameter *options) {
=C2=A0 =C2=A0 assert(!(opts && options));
=C2=A0 =C2=A0 ... modify dst in-place to have it cover the entries from opt= s or
options
}

then in this code, you do:
=C2=A0 =C2=A0 create_options =3D /* generate empty QemuOpts */;
=C2=A0 =C2=A0 qemu_opts_append_pair(options, drv->create_opts,
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 drv->create_options);
=C2=A0 =C2=A0 qemu_opts_append_pair(options, proto_drv->create_options,<= br> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 proto_drv->create_options);


Good. Together w= ith always using QemuOpts and only converting to
QEMUOptionParameter unt= il it calls .bdrv_create in specific driver,
the drv/proto_dr= v inconsistent problem could be solved.
Will update.

Thanks very much for all your suggestions!

Chunyan

>
> =C2=A0 =C2=A0 =C2=A0/* Create parameter list with default values */ > - =C2=A0 =C2=A0param =3D parse_option_parameters("", create_= options, param);
> -
> - =C2=A0 =C2=A0set_option_parameter_int(param, BLOCK_OPT_SIZE, img_siz= e);
> + =C2=A0 =C2=A0opts =3D qemu_opts_create(create_opts, NULL, 0, &er= ror_abort);
> + =C2=A0 =C2=A0qemu_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 cha= r *filename, const char *fmt,
>
> =C2=A0 =C2=A0 =C2=A0if (!quiet) {
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0printf("Formatting '%s'= , fmt=3D%s ", filename, fmt);
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0print_option_parameters(param);
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0qemu_opts_print(opts);
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0puts("");
> =C2=A0 =C2=A0 =C2=A0}
> - =C2=A0 =C2=A0ret =3D bdrv_create(drv, filename, param, &local_er= r);
> +
> + =C2=A0 =C2=A0if (drv->bdrv_create2) {
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0ret =3D bdrv_create(drv, filename, NULL, = opts, &local_err);
> + =C2=A0 =C2=A0} else {
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0param =3D opts_to_params(opts);
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0ret =3D bdrv_create(drv, filename, param,= NULL, &local_err);
> + =C2=A0 =C2=A0}

...and finally convert back to QEMUOptionParameters at the last momen= t.
=C2=A0But wait a minute - why are you checking for drv->bdrv_create2 her= e?
Isn't the last possible moment really inside bdrv_create() (that is, th= e
actual function that decides whether to call drv->bdrv_create vs.
drv->bdrv_create2)? =C2=A0For the code to be as clean as possible, you w= ant
to use QemuOpts everywhere until the actual point where you are calling
the unconverted callback.


> -int bdrv_amend_options(BlockDriverState *bs, QEMUOptionParameter *opt= ions)
> +int bdrv_amend_options(BlockDriverState *bs, QEMUOptionParameter *opt= ions,
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 QemuOpts *opts)
> =C2=A0{
> - =C2=A0 =C2=A0if (bs->drv->bdrv_amend_options =3D=3D NULL) { > + =C2=A0 =C2=A0if (!bs->drv->bdrv_amend_options && !bs-&= gt;drv->bdrv_amend_options2) {
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return -ENOTSUP;
> =C2=A0 =C2=A0 =C2=A0}
> - =C2=A0 =C2=A0return bs->drv->bdrv_amend_options(bs, options);<= br> > + =C2=A0 =C2=A0if (bs->drv->bdrv_amend_options2) {
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0return bs->drv->bdrv_amend_options2= (bs, opts);
> + =C2=A0 =C2=A0} else {
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0return bs->drv->bdrv_amend_options(= bs, options);

Similar to the create/create2 situation, _this_ function is the last<= br> possible place to make the conversions. =C2=A0You have a problem in that if=
the user passes you options but the callback expects opts, you are
passing NULL to the callback. =C2=A0I think this should look more like:

int bdrv_amend_options(BlockDriverState *bs, QEMUOptionParameter *options,<= br> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0QemuOpts *opts)
{
=C2=A0 =C2=A0 int ret;
=C2=A0 =C2=A0 assert(!(opts && options));
=C2=A0 =C2=A0 if (!bs->drv->bdrv_amend_options &&= amp; !bs->drv->bdrv_amend_options2) {
=C2=A0 =C2=A0 =C2=A0 =C2=A0 return -ENOTSUP;
=C2=A0 =C2=A0 }
=C2=A0 =C2=A0 if (bs->drv->bdrv_amend_options2) {
=C2=A0 =C2=A0 =C2=A0 =C2=A0 if (options) {
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 opts =3D params_to_opts(options);=
=C2=A0 =C2=A0 =C2=A0 =C2=A0 }
=C2=A0 =C2=A0 =C2=A0 =C2=A0 ret =3D bs->drv->bdrv_amend_options2(bs, = opts);
=C2=A0 =C2=A0 =C2=A0 =C2=A0 if (options) {
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 g_free(opts); // or whatever corr= ect spelling to avoid leak
=C2=A0 =C2=A0 =C2=A0 =C2=A0 }
=C2=A0 =C2=A0 } else {
=C2=A0 =C2=A0 =C2=A0 =C2=A0 if (opts) {
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 options =3D opts_to_params(opts);=
=C2=A0 =C2=A0 =C2=A0 =C2=A0 }
=C2=A0 =C2=A0 =C2=A0 =C2=A0 ret =3D bs->drv->bdrv_amend_options(bs, o= ptions);
=C2=A0 =C2=A0 =C2=A0 =C2=A0 if (opts) {
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 g_free(options); // again, with c= orrect spelling
=C2=A0 =C2=A0 =C2=A0 =C2=A0 }
=C2=A0 =C2=A0 }
=C2=A0 =C2=A0 return ret;

> +++ b/include/block/block_int.h
> @@ -118,6 +118,8 @@ struct BlockDriver {
> =C2=A0 =C2=A0 =C2=A0void (*bdrv_rebind)(BlockDriverState *bs);
> =C2=A0 =C2=A0 =C2=A0int (*bdrv_create)(const char *filename, QEMUOptio= nParameter *options,
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 Error **errp);
> + =C2=A0 =C2=A0/* FIXME: will remove the duplicate and rename back to = bdrv_create later */
> + =C2=A0 =C2=A0int (*bdrv_create2)(const char *filename, QemuOpts *opt= s, 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).

> =C2=A0 =C2=A0 =C2=A0int (*bdrv_set_key)(BlockDriverState *bs, const ch= ar *key);
> =C2=A0 =C2=A0 =C2=A0int (*bdrv_make_empty)(BlockDriverState *bs);
> =C2=A0 =C2=A0 =C2=A0/* aio */
> @@ -217,7 +219,7 @@ struct BlockDriver {
>
> =C2=A0 =C2=A0 =C2=A0/* List of options for creating images, terminated= by name =3D=3D NULL */
> =C2=A0 =C2=A0 =C2=A0QEMUOptionParameter *create_options;
> -
> + =C2=A0 =C2=A0QemuOptsList *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 *f= ilename, const char *fmt)
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return 1;
> =C2=A0 =C2=A0 =C2=A0}
>
> - =C2=A0 =C2=A0create_options =3D append_option_parameters(create_opti= ons,
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0drv->create_options);
> -
> + =C2=A0 =C2=A0if (drv->create_opts) {
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0create_opts =3D qemu_opts_append(create_o= pts, drv->create_opts);
> + =C2=A0 =C2=A0} else {
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0create_options =3D append_option_paramete= rs(create_options,
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0drv->create_options);
> + =C2=A0 =C2=A0}
> =C2=A0 =C2=A0 =C2=A0if (filename) {
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0proto_drv =3D bdrv_find_protocol(fil= ename, true);
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (!proto_drv) {
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0error_report("Unk= nown protocol '%s'", filename);
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return 1;
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0}
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0create_options =3D append_option_paramete= rs(create_options,
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0proto_drv->create_options);
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0if (drv->create_opts) {
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0create_opts =3D qemu_opts_a= ppend(create_opts, proto_drv->create_opts);

Memory leak.

> + =C2=A0 =C2=A0 =C2=A0 =C2=A0} else {
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0create_options =3D
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0append_option= _parameters(create_options,
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 p= roto_drv->create_options);
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0}
> =C2=A0 =C2=A0 =C2=A0}
>
> - =C2=A0 =C2=A0print_option_help(create_options);
> + =C2=A0 =C2=A0if (drv->create_opts) {
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0qemu_opts_print_help(create_opts);
> + =C2=A0 =C2=A0} else {
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0print_option_help(create_options);
> + =C2=A0 =C2=A0}

Another case where if you add a new helper function that absorbs eith= er
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)<= br> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0goto out;
> =C2=A0 =C2=A0 =C2=A0}

> - =C2=A0 =C2=A0 =C2=A0 =C2=A0}
> + =C2=A0 =C2=A0if (drv->bdrv_create2) {
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0create_opts =3D qemu_opts_append(create_o= pts, drv->create_opts);
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0create_opts =3D qemu_opts_append(create_o= pts, proto_drv->create_opts);

More memory leaks.


> @@ -1400,7 +1418,12 @@ static int img_convert(int argc, char **argv) >
> =C2=A0 =C2=A0 =C2=A0if (!skip_create) {
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0/* Create the new image */
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0ret =3D bdrv_create(drv, out_filename, pa= ram, &local_err);
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0if (drv->bdrv_create2) {
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0ret =3D bdrv_create(drv, ou= t_filename, NULL, opts, &local_err);
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0} else {
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0param =3D opts_to_params(op= ts);
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0ret =3D bdrv_create(drv, ou= t_filename, param, NULL, &local_err);
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0}

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) > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0goto out;
> =C2=A0 =C2=A0 =C2=A0}
>
> - =C2=A0 =C2=A0create_options =3D append_option_parameters(create_opti= ons,
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0bs->drv->create_optio= ns);
> - =C2=A0 =C2=A0options_param =3D parse_option_parameters(options, crea= te_options,
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0options_param);
> - =C2=A0 =C2=A0if (options_param =3D=3D NULL) {
> + =C2=A0 =C2=A0if (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...

> + =C2=A0 =C2=A0 =C2=A0 =C2=A0create_opts =3D qemu_opts_append(create_o= pts, bs->drv->create_opts);
> + =C2=A0 =C2=A0} else {
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0create_options =3D append_option_paramete= rs(create_options,
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0bs->drv->create_options);
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0create_opts =3D params_to_opts(create_opt= ions);
> + =C2=A0 =C2=A0}
> + =C2=A0 =C2=A0opts =3D qemu_opts_create(create_opts, NULL, 0, &er= ror_abort);
> + =C2=A0 =C2=A0if (options && qemu_opts_do_parse(opts, options= , NULL)) {
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0error_report("Invalid options f= or file format '%s'", fmt);
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0ret =3D -1;
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0goto out;
> =C2=A0 =C2=A0 =C2=A0}
>
> - =C2=A0 =C2=A0ret =3D bdrv_amend_options(bs, options_param);
> + =C2=A0 =C2=A0if (bs->drv->bdrv_amend_options2) {
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0ret =3D bdrv_amend_options(bs, NULL, opts= );
> + =C2=A0 =C2=A0} else {
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0options_param =3D opts_to_params(opts); > + =C2=A0 =C2=A0 =C2=A0 =C2=A0ret =3D bdrv_amend_options(bs, options_pa= ram, NULL);

...and blindly letting bdrv_amend_options() be the place that convert= s
to QEMUOptionParameters as needed.

Here's hoping v23 is nicer; I'm looking forward to ditching
QEMUOptionParameters.=C2=A0

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


--f46d044283b08d291404f462ea6c--