On 04/10/2014 11:54 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> > ---
> @@ -419,8 +420,27 @@ static void coroutine_fn bdrv_create_co_entry(void > *opaque) > > CreateCo *cco = opaque; > + if (cco->drv->bdrv_create2) { > + QemuOptsList *opts_list = NULL; > + QemuOpts *opts = NULL; > + if (!cco->opts) { Isn't your conversion pair-wise per driver, in that you always pair bdrv_create2 with options, and bdrv_create with opts? That is, won't cco->opts always be false if cco->drv->bdrv_create2 is non-NULL, since we already guaranteed that there is at most one of the two creation function callbacks defined? Maybe you have a missing assertion at the point where the callbacks are registered? [1] > + opts_list = params_to_opts(cco->options); > + cco->opts = opts = > + qemu_opts_create(opts_list, NULL, 0, &error_abort); Ouch. This assigns to cco->opts,... > + } > + ret = cco->drv->bdrv_create2(cco->filename, cco->opts, &local_err); > + qemu_opts_del(opts); ...but this frees the memory. You have modified the caller's opaque pointer to point to what is now stale memory. Is this intentional? [2] > + qemu_opts_free(opts_list); > + } else { > + QEMUOptionParameter *options = NULL; > + if (!cco->options) { > + cco->options = options = opts_to_params(cco->opts); > + } > + ret = cco->drv->bdrv_create(cco->filename, cco->options, &local_err); > + free_option_parameters(options); Same question [2] in the opposite direction for cco->options. > @@ -5296,28 +5328,25 @@ void bdrv_img_create(const char *filename, const char > *fmt, > /* Parse -o options */ > if (options) { > - param = parse_option_parameters(options, create_options, param); > - if (param == NULL) { > + if (qemu_opts_do_parse(opts, options, NULL) != 0) { > error_setg(errp, "Invalid options for file format '%s'.", fmt); Pre-existing (and probably worth an independent patch to qemu-trivial), but error messages typically should not end in '.' > +int bdrv_amend_options(BlockDriverState *bs, QEMUOptionParameter *options, > + QemuOpts *opts) > { > - if (bs->drv->bdrv_amend_options == NULL) { > + int ret; > + assert(!(options && opts)); > + > + if (!bs->drv->bdrv_amend_options && !bs->drv->bdrv_amend_options2) { Hmm, should we also guarantee that at most one of these callback functions exist? [1] > return -ENOTSUP; > } > - return bs->drv->bdrv_amend_options(bs, options); > + if (bs->drv->bdrv_amend_options2) { > + QemuOptsList *tmp_opts_list = NULL; > + QemuOpts *tmp_opts = NULL; > + if (options) { > + tmp_opts_list = params_to_opts(options); > + opts = tmp_opts = > + qemu_opts_create(tmp_opts_list, NULL, 0, &error_abort); > + } > + ret = bs->drv->bdrv_amend_options2(bs, opts); > + qemu_opts_del(tmp_opts); Why do you need tmp_opts? Isn't manipulation of 'opts' sufficient here? > +++ 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); > int (*bdrv_set_key)(BlockDriverState *bs, const char *key); > int (*bdrv_make_empty)(BlockDriverState *bs); > /* aio */ > @@ -217,7 +219,10 @@ struct BlockDriver { > > /* List of options for creating images, terminated by name == NULL */ > QEMUOptionParameter *create_options; > - > + /* FIXME: will replace create_options. > + * These two fields are mutually exclusive. At most one is non-NULL. > + */ > + QemuOptsList *create_opts; Do you also want to mention that create_options may only be set with bdrv_create, and that create_opts may only be set with bdrv_create2? [1] > @@ -1340,40 +1340,36 @@ static int img_convert(int argc, char **argv) > > - if (options) { > - param = parse_option_parameters(options, create_options, param); > - if (param == NULL) { > - error_report("Invalid options for file format '%s'.", out_fmt); > - ret = -1; > - goto out; > - } > - } else { > - param = parse_option_parameters("", create_options, param); > + 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'.", out_fmt); Pre-existing, but another instance of trailing '.' in the error message. Back to [1], rather than asserting in individual functions at use time, why not do all the sanity checking up front in bdrv_register()? That is, I think it is easier to add: if (bdrv->bdrv_create) { assert(!bdrv->bdrv_create2); assert(!bdrv->opts && !bdrv->bdrv_amend_options2); } else if (bdrv_{ assert(!bdrv->options && !bdrv->bdrv_amend_options); } at the point where drivers are registered, rather than having to duplicate checks across all points that might use a driver. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature