| Message ID | 1397152467-17186-12-git-send-email-cyliu@suse.com |
|---|---|
| State | New |
| Headers | show |
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 <wdongxu@linux.vnet.ibm.com> > Signed-off-by: Chunyan Liu <cyliu@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.
>>> On 4/22/2014 at 07:17 AM, in message <5355A71E.2010600@redhat.com>, Eric Blake <eblake@redhat.com> wrote: > 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 <wdongxu@linux.vnet.ibm.com> > > Signed-off-by: Chunyan Liu <cyliu@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] To handle both QEMUOptionParameter and QemuOpts, we convert QEMUOptionParameter to QemuOpts first for unified processing. And according to v22 discussion, it's better to convert back to QEMUOptionParameter at the last moment. https://lists.gnu.org/archive/html/qemu-devel/2014-03/msg02181.html. For bdrv_create, either do this in bdrv_create(), or here in bdrv_create_co_entry(). bdrv_create() just calls bdrv_create_co_entry to do the work. Do you think we should finish conversion in bdrv_create() before calling bdrv_create_co_entry()? > > > + 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] Yes. My intension is to avoid deleting cco->opts errorly, only the converted one from cco->options should be deleted, so I use a temp variable to retrieve the new allocated opts, just new and delete the temp variable is OK. The work could also be done as: if (cco->options) { opts_list = params_to_opts(cco->options); cco->opts = qemu_opts_create(opts_list, NULL, 0, &error_abort); } ret = cco->drv->bdrv_create2(cco->filename, cco->opts, &local_err); if (cco->options) { qemu_opts_del(cco->opts); qemu_opts_free(opts_list); } > > > + 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 '.' Will add new patch for that. > > > +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? Same as answer to [2]. The intension is to avoid deleting opts errorly. > > > +++ 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] Will add description. > > > > @@ -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. That makes sense. But [1] still needs the asserting individually, since it doesn't use bdrv->create_opts or bdrv->create_options, but the opts or options passed in. > > -- > Eric Blake eblake redhat com +1-919-301-3266 > Libvirt virtualization library http://libvirt.org > >
On Fri, Apr 11, 2014 at 01:54:07AM +0800, 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 <wdongxu@linux.vnet.ibm.com> > Signed-off-by: Chunyan Liu <cyliu@suse.com> > --- > block.c | 151 +++++++++++++++++++++++++++++++--------------- > block/cow.c | 2 +- > block/qcow.c | 2 +- > block/qcow2.c | 2 +- > block/qed.c | 2 +- > block/raw_bsd.c | 2 +- > block/vhdx.c | 2 +- > block/vmdk.c | 4 +- > block/vvfat.c | 2 +- > include/block/block.h | 7 ++- > include/block/block_int.h | 11 +++- > qemu-img.c | 94 ++++++++++++++--------------- > 12 files changed, 170 insertions(+), 111 deletions(-) Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
On Mon, Apr 21, 2014 at 05:17:50PM -0600, Eric Blake wrote: > 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 <wdongxu@linux.vnet.ibm.com> > > Signed-off-by: Chunyan Liu <cyliu@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] I think you're right but not sure it's worth changing since this is temporary code that gets removed later in the patch series anyway. Stefan
>>> On 4/23/2014 at 02:44 PM, in message <53576153.6CE : 102 : 21807>, Chun Yan Liu wrote: > >>>> On 4/22/2014 at 07:17 AM, in message <5355A71E.2010600@redhat.com>, Eric Blake > <eblake@redhat.com> wrote: > > 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 <wdongxu@linux.vnet.ibm.com> >> > Signed-off-by: Chunyan Liu <cyliu@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] > > To handle both QEMUOptionParameter and QemuOpts, we convert > QEMUOptionParameter to QemuOpts first for unified processing. > And according to v22 discussion, it's better to convert back to > QEMUOptionParameter at the last moment. > https://lists.gnu.org/archive/html/qemu-devel/2014-03/msg02181.html. > For bdrv_create, either do this in bdrv_create(), or here in > bdrv_create_co_entry(). bdrv_create() just calls bdrv_create_co_entry > to do the work. Do you think we should finish conversion in bdrv_create() > before calling bdrv_create_co_entry()? > Eric, how do you think of this? Do we need a change? >> >> > + 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] > > Yes. My intension is to avoid deleting cco->opts errorly, only the > converted > one from cco->options should be deleted, so I use a temp variable to > retrieve > the new allocated opts, just new and delete the temp variable is OK. > > The work could also be done as: > > if (cco->options) { > opts_list = params_to_opts(cco->options); > cco->opts = > qemu_opts_create(opts_list, NULL, 0, &error_abort); > } > ret = cco->drv->bdrv_create2(cco->filename, cco->opts, &local_err); > if (cco->options) { > qemu_opts_del(cco->opts); > qemu_opts_free(opts_list); > } > And here. Should we change it to this way? I'll update and prepare a new version. Regards, Chunyan >> >> > + 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 '.' > > Will add new patch for that. > >> >> > +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? > > Same as answer to [2]. The intension is to avoid deleting opts errorly. > >> >> > +++ 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] > > Will add description. > >> >> >> > @@ -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. > > That makes sense. But [1] still needs the asserting individually, since > it doesn't use bdrv->create_opts or bdrv->create_options, but the opts or > options passed in. > >> >> -- >> Eric Blake eblake redhat com +1-919-301-3266 >> Libvirt virtualization library http://libvirt.org >> >> >
On 04/28/2014 12:20 AM, Chun Yan Liu wrote: >>> 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] >> >> To handle both QEMUOptionParameter and QemuOpts, we convert >> QEMUOptionParameter to QemuOpts first for unified processing. >> And according to v22 discussion, it's better to convert back to >> QEMUOptionParameter at the last moment. >> https://lists.gnu.org/archive/html/qemu-devel/2014-03/msg02181.html. >> For bdrv_create, either do this in bdrv_create(), or here in >> bdrv_create_co_entry(). bdrv_create() just calls bdrv_create_co_entry >> to do the work. Do you think we should finish conversion in bdrv_create() >> before calling bdrv_create_co_entry()? >> > > Eric, how do you think of this? Do we need a change? At this point, it's up to you. Stefan made the valid point that as long as each patch works in isolation, it's not worth any additional churn to add robustness to code that will be ripped out later in the series. >> >> The work could also be done as: >> >> if (cco->options) { >> opts_list = params_to_opts(cco->options); >> cco->opts = >> qemu_opts_create(opts_list, NULL, 0, &error_abort); >> } >> ret = cco->drv->bdrv_create2(cco->filename, cco->opts, &local_err); >> if (cco->options) { >> qemu_opts_del(cco->opts); >> qemu_opts_free(opts_list); >> } >> > > And here. Should we change it to this way? That way is actually fairly legible, and at this point, anything that makes the patches easy to understand as being correct will help us get the series applied sooner rather than later.
diff --git a/block.c b/block.c index 990a754..560cbd5 100644 --- a/block.c +++ b/block.c @@ -408,6 +408,7 @@ typedef struct CreateCo { BlockDriver *drv; char *filename; QEMUOptionParameter *options; + QemuOpts *opts; int ret; Error *err; } CreateCo; @@ -419,8 +420,27 @@ static void coroutine_fn bdrv_create_co_entry(void *opaque) CreateCo *cco = opaque; assert(cco->drv); - - ret = cco->drv->bdrv_create(cco->filename, cco->options, &local_err); + assert(!(cco->options && cco->opts)); + + if (cco->drv->bdrv_create2) { + QemuOptsList *opts_list = NULL; + QemuOpts *opts = NULL; + if (!cco->opts) { + opts_list = params_to_opts(cco->options); + cco->opts = opts = + qemu_opts_create(opts_list, NULL, 0, &error_abort); + } + ret = cco->drv->bdrv_create2(cco->filename, cco->opts, &local_err); + qemu_opts_del(opts); + 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); + } if (local_err) { error_propagate(&cco->err, local_err); } @@ -428,7 +448,8 @@ static void coroutine_fn bdrv_create_co_entry(void *opaque) } int bdrv_create(BlockDriver *drv, const char* filename, - QEMUOptionParameter *options, Error **errp) + QEMUOptionParameter *options, + QemuOpts *opts, Error **errp) { int ret; @@ -437,11 +458,12 @@ int bdrv_create(BlockDriver *drv, const char* filename, .drv = drv, .filename = g_strdup(filename), .options = options, + .opts = opts, .ret = NOT_DONE, .err = NULL, }; - if (!drv->bdrv_create) { + if (!drv->bdrv_create && !drv->bdrv_create2) { error_setg(errp, "Driver '%s' does not support image creation", drv->format_name); ret = -ENOTSUP; goto out; @@ -473,7 +495,7 @@ out: } int bdrv_create_file(const char* filename, QEMUOptionParameter *options, - Error **errp) + QemuOpts *opts, Error **errp) { BlockDriver *drv; Error *local_err = NULL; @@ -485,7 +507,7 @@ int bdrv_create_file(const char* filename, QEMUOptionParameter *options, return -ENOENT; } - ret = bdrv_create(drv, filename, options, &local_err); + ret = bdrv_create(drv, filename, options, opts, &local_err); if (local_err) { error_propagate(errp, local_err); } @@ -1174,7 +1196,8 @@ void bdrv_append_temp_snapshot(BlockDriverState *bs, Error **errp) int64_t total_size; BlockDriver *bdrv_qcow2; - QEMUOptionParameter *create_options; + QemuOptsList *create_opts = NULL; + QemuOpts *opts = NULL; QDict *snapshot_options; BlockDriverState *bs_snapshot; Error *local_err; @@ -1199,13 +1222,20 @@ void bdrv_append_temp_snapshot(BlockDriverState *bs, Error **errp) } bdrv_qcow2 = bdrv_find_format("qcow2"); - create_options = parse_option_parameters("", bdrv_qcow2->create_options, - NULL); - set_option_parameter_int(create_options, BLOCK_OPT_SIZE, total_size); - - ret = bdrv_create(bdrv_qcow2, tmp_filename, create_options, &local_err); - free_option_parameters(create_options); + assert(!(bdrv_qcow2->create_options && bdrv_qcow2->create_opts)); + if (bdrv_qcow2->create_options) { + create_opts = params_to_opts(bdrv_qcow2->create_options); + } else { + create_opts = bdrv_qcow2->create_opts; + } + opts = qemu_opts_create(create_opts, NULL, 0, &error_abort); + qemu_opt_set_number(opts, BLOCK_OPT_SIZE, total_size); + ret = bdrv_create(bdrv_qcow2, tmp_filename, NULL, opts, &local_err); + qemu_opts_del(opts); + if (bdrv_qcow2->create_options) { + qemu_opts_free(create_opts); + } if (ret < 0) { error_setg_errno(errp, -ret, "Could not create temporary overlay " "'%s': %s", tmp_filename, @@ -5276,8 +5306,10 @@ void bdrv_img_create(const char *filename, const char *fmt, char *options, uint64_t img_size, int flags, Error **errp, bool quiet) { - QEMUOptionParameter *param = NULL, *create_options = NULL; - QEMUOptionParameter *backing_fmt, *backing_file, *size; + QemuOptsList *create_opts = NULL; + QemuOpts *opts = NULL; + const char *backing_fmt, *backing_file; + int64_t size; BlockDriver *drv, *proto_drv; BlockDriver *backing_drv = NULL; Error *local_err = NULL; @@ -5296,28 +5328,25 @@ 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); + create_opts = qemu_opts_append(create_opts, drv->create_opts, + drv->create_options); + create_opts = qemu_opts_append(create_opts, proto_drv->create_opts, + 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); /* 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); goto out; } } if (base_filename) { - if (set_option_parameter(param, BLOCK_OPT_BACKING_FILE, - base_filename)) { + if (qemu_opt_set(opts, BLOCK_OPT_BACKING_FILE, base_filename)) { error_setg(errp, "Backing file not supported for file format '%s'", fmt); goto out; @@ -5325,37 +5354,37 @@ void bdrv_img_create(const char *filename, const char *fmt, } if (base_fmt) { - if (set_option_parameter(param, BLOCK_OPT_BACKING_FMT, base_fmt)) { + if (qemu_opt_set(opts, BLOCK_OPT_BACKING_FMT, base_fmt)) { error_setg(errp, "Backing file format not supported for file " "format '%s'", fmt); goto out; } } - backing_file = get_option_parameter(param, BLOCK_OPT_BACKING_FILE); - if (backing_file && backing_file->value.s) { - if (!strcmp(filename, backing_file->value.s)) { + backing_file = qemu_opt_get(opts, BLOCK_OPT_BACKING_FILE); + if (backing_file) { + if (!strcmp(filename, backing_file)) { error_setg(errp, "Error: Trying to create an image with the " "same filename as the backing file"); goto out; } } - backing_fmt = get_option_parameter(param, BLOCK_OPT_BACKING_FMT); - if (backing_fmt && backing_fmt->value.s) { - backing_drv = bdrv_find_format(backing_fmt->value.s); + backing_fmt = qemu_opt_get(opts, BLOCK_OPT_BACKING_FMT); + if (backing_fmt) { + backing_drv = bdrv_find_format(backing_fmt); if (!backing_drv) { error_setg(errp, "Unknown backing file format '%s'", - backing_fmt->value.s); + backing_fmt); goto out; } } // The size for the image must always be specified, with one exception: // If we are using a backing file, we can obtain the size from there - size = get_option_parameter(param, BLOCK_OPT_SIZE); - if (size && size->value.n == -1) { - if (backing_file && backing_file->value.s) { + size = qemu_opt_get_size(opts, BLOCK_OPT_SIZE, 0); + if (size == -1) { + if (backing_file) { BlockDriverState *bs; uint64_t size; char buf[32]; @@ -5366,11 +5395,11 @@ void bdrv_img_create(const char *filename, const char *fmt, flags & ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING); bs = NULL; - ret = bdrv_open(&bs, backing_file->value.s, NULL, NULL, back_flags, + ret = bdrv_open(&bs, backing_file, NULL, NULL, back_flags, backing_drv, &local_err); if (ret < 0) { error_setg_errno(errp, -ret, "Could not open '%s': %s", - backing_file->value.s, + backing_file, error_get_pretty(local_err)); error_free(local_err); local_err = NULL; @@ -5380,7 +5409,7 @@ void bdrv_img_create(const char *filename, const char *fmt, size *= 512; snprintf(buf, sizeof(buf), "%" PRId64, size); - set_option_parameter(param, BLOCK_OPT_SIZE, buf); + qemu_opt_set_number(opts, BLOCK_OPT_SIZE, size); bdrv_unref(bs); } else { @@ -5391,16 +5420,18 @@ 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); + + ret = bdrv_create(drv, filename, NULL, opts, &local_err); + if (ret == -EFBIG) { /* This is generally a better message than whatever the driver would * deliver (especially because of the cluster_size_hint), since that * is most probably not much different from "image too large". */ const char *cluster_size_hint = ""; - if (get_option_parameter(create_options, BLOCK_OPT_CLUSTER_SIZE)) { + if (qemu_opt_get_size(opts, BLOCK_OPT_CLUSTER_SIZE, 0)) { cluster_size_hint = " (try using a larger cluster size)"; } error_setg(errp, "The image size is too large for file format '%s'" @@ -5410,9 +5441,8 @@ void bdrv_img_create(const char *filename, const char *fmt, } out: - free_option_parameters(create_options); - free_option_parameters(param); - + qemu_opts_del(opts); + qemu_opts_free(create_opts); if (local_err) { error_propagate(errp, local_err); } @@ -5430,12 +5460,35 @@ void bdrv_add_before_write_notifier(BlockDriverState *bs, notifier_with_return_list_add(&bs->before_write_notifiers, notifier); } -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) { + int ret; + assert(!(options && opts)); + + 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) { + 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); + qemu_opts_free(tmp_opts_list); + } else { + QEMUOptionParameter *param = NULL; + if (opts) { + options = param = opts_to_params(opts); + } + ret = bs->drv->bdrv_amend_options(bs, options); + free_option_parameters(param); + } + return ret; } /* This function will be called by the bdrv_recurse_is_first_non_filter method diff --git a/block/cow.c b/block/cow.c index 30deb88..26cf4a5 100644 --- a/block/cow.c +++ b/block/cow.c @@ -345,7 +345,7 @@ static int cow_create(const char *filename, QEMUOptionParameter *options, options++; } - ret = bdrv_create_file(filename, options, &local_err); + ret = bdrv_create_file(filename, options, NULL, &local_err); if (ret < 0) { error_propagate(errp, local_err); return ret; diff --git a/block/qcow.c b/block/qcow.c index d5a7d5f..9411aef 100644 --- a/block/qcow.c +++ b/block/qcow.c @@ -687,7 +687,7 @@ static int qcow_create(const char *filename, QEMUOptionParameter *options, options++; } - ret = bdrv_create_file(filename, options, &local_err); + ret = bdrv_create_file(filename, options, NULL, &local_err); if (ret < 0) { error_propagate(errp, local_err); return ret; diff --git a/block/qcow2.c b/block/qcow2.c index e903d97..49985e3 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -1625,7 +1625,7 @@ static int qcow2_create2(const char *filename, int64_t total_size, Error *local_err = NULL; int ret; - ret = bdrv_create_file(filename, options, &local_err); + ret = bdrv_create_file(filename, options, NULL, &local_err); if (ret < 0) { error_propagate(errp, local_err); return ret; diff --git a/block/qed.c b/block/qed.c index 3bd9db9..ef5c56c 100644 --- a/block/qed.c +++ b/block/qed.c @@ -566,7 +566,7 @@ static int qed_create(const char *filename, uint32_t cluster_size, int ret = 0; BlockDriverState *bs; - ret = bdrv_create_file(filename, NULL, &local_err); + ret = bdrv_create_file(filename, NULL, NULL, &local_err); if (ret < 0) { error_propagate(errp, local_err); return ret; diff --git a/block/raw_bsd.c b/block/raw_bsd.c index 01ea692..9ae5fc2 100644 --- a/block/raw_bsd.c +++ b/block/raw_bsd.c @@ -145,7 +145,7 @@ static int raw_create(const char *filename, QEMUOptionParameter *options, Error *local_err = NULL; int ret; - ret = bdrv_create_file(filename, options, &local_err); + ret = bdrv_create_file(filename, options, NULL, &local_err); if (local_err) { error_propagate(errp, local_err); } diff --git a/block/vhdx.c b/block/vhdx.c index 509baaf..a9fcf6b 100644 --- a/block/vhdx.c +++ b/block/vhdx.c @@ -1796,7 +1796,7 @@ static int vhdx_create(const char *filename, QEMUOptionParameter *options, block_size = block_size > VHDX_BLOCK_SIZE_MAX ? VHDX_BLOCK_SIZE_MAX : block_size; - ret = bdrv_create_file(filename, options, &local_err); + ret = bdrv_create_file(filename, options, NULL, &local_err); if (ret < 0) { error_propagate(errp, local_err); goto exit; diff --git a/block/vmdk.c b/block/vmdk.c index b69988d..d87c8f6 100644 --- a/block/vmdk.c +++ b/block/vmdk.c @@ -1525,7 +1525,7 @@ static int vmdk_create_extent(const char *filename, int64_t filesize, uint32_t *gd_buf = NULL; int gd_buf_size; - ret = bdrv_create_file(filename, NULL, &local_err); + ret = bdrv_create_file(filename, NULL, NULL, &local_err); if (ret < 0) { error_propagate(errp, local_err); goto exit; @@ -1865,7 +1865,7 @@ static int vmdk_create(const char *filename, QEMUOptionParameter *options, if (!split && !flat) { desc_offset = 0x200; } else { - ret = bdrv_create_file(filename, options, &local_err); + ret = bdrv_create_file(filename, options, NULL, &local_err); if (ret < 0) { error_setg_errno(errp, -ret, "Could not create image file"); goto exit; diff --git a/block/vvfat.c b/block/vvfat.c index 1978c9e..4390b12 100644 --- a/block/vvfat.c +++ b/block/vvfat.c @@ -2926,7 +2926,7 @@ static int enable_write_target(BDRVVVFATState *s) set_option_parameter_int(options, BLOCK_OPT_SIZE, s->sector_count * 512); set_option_parameter(options, BLOCK_OPT_BACKING_FILE, "fat:"); - ret = bdrv_create(bdrv_qcow, s->qcow_filename, options, &local_err); + ret = bdrv_create(bdrv_qcow, s->qcow_filename, options, NULL, &local_err); if (ret < 0) { qerror_report_err(local_err); error_free(local_err); diff --git a/include/block/block.h b/include/block/block.h index b3230a2..6c38402 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -177,9 +177,9 @@ BlockDriver *bdrv_find_format(const char *format_name); BlockDriver *bdrv_find_whitelisted_format(const char *format_name, bool readonly); int bdrv_create(BlockDriver *drv, const char* filename, - QEMUOptionParameter *options, Error **errp); + QEMUOptionParameter *options, QemuOpts *opts, Error **errp); int bdrv_create_file(const char* filename, QEMUOptionParameter *options, - Error **errp); + QemuOpts *opts, Error **errp); BlockDriverState *bdrv_new(const char *device_name); void bdrv_make_anon(BlockDriverState *bs); void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old); @@ -284,7 +284,8 @@ typedef enum { int bdrv_check(BlockDriverState *bs, BdrvCheckResult *res, BdrvCheckMode fix); -int bdrv_amend_options(BlockDriverState *bs_new, QEMUOptionParameter *options); +int bdrv_amend_options(BlockDriverState *bs_new, QEMUOptionParameter *options, + QemuOpts *opts); /* external snapshots */ bool bdrv_recurse_is_first_non_filter(BlockDriverState *bs, diff --git a/include/block/block_int.h b/include/block/block_int.h index cd5bc73..f9d87da 100644 --- a/include/block/block_int.h +++ 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; /* * Returns 0 for completed check, -errno for internal errors. @@ -228,6 +233,10 @@ struct BlockDriver { int (*bdrv_amend_options)(BlockDriverState *bs, QEMUOptionParameter *options); + /* FIXME: will remove the duplicate and rename back to + * bdrv_amend_options later + */ + int (*bdrv_amend_options2)(BlockDriverState *bs, QemuOpts *opts); void (*bdrv_debug_event)(BlockDriverState *bs, BlkDebugEvent event); diff --git a/qemu-img.c b/qemu-img.c index 8455994..3e2ff0e 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -235,7 +235,7 @@ static int read_password(char *buf, int buf_size) static int print_block_option_help(const char *filename, const char *fmt) { BlockDriver *drv, *proto_drv; - QEMUOptionParameter *create_options = NULL; + QemuOptsList *create_opts = NULL; /* Find driver and parse its options */ drv = bdrv_find_format(fmt); @@ -244,21 +244,20 @@ static int print_block_option_help(const char *filename, const char *fmt) return 1; } - create_options = append_option_parameters(create_options, - drv->create_options); - + create_opts = qemu_opts_append(create_opts, drv->create_opts, + 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); + create_opts = qemu_opts_append(create_opts, proto_drv->create_opts, + proto_drv->create_options); } - print_option_help(create_options); - free_option_parameters(create_options); + qemu_opts_print_help(create_opts); + qemu_opts_free(create_opts); return 0; } @@ -311,19 +310,19 @@ fail: return NULL; } -static int add_old_style_options(const char *fmt, QEMUOptionParameter *list, +static int add_old_style_options(const char *fmt, QemuOpts *opts, const char *base_filename, const char *base_fmt) { if (base_filename) { - if (set_option_parameter(list, BLOCK_OPT_BACKING_FILE, base_filename)) { + if (qemu_opt_set(opts, BLOCK_OPT_BACKING_FILE, base_filename)) { error_report("Backing file not supported for file format '%s'", fmt); return -1; } } if (base_fmt) { - if (set_option_parameter(list, BLOCK_OPT_BACKING_FMT, base_fmt)) { + if (qemu_opt_set(opts, BLOCK_OPT_BACKING_FMT, base_fmt)) { error_report("Backing file format not supported for file " "format '%s'", fmt); return -1; @@ -1153,8 +1152,9 @@ static int img_convert(int argc, char **argv) size_t bufsectors = IO_BUF_SIZE / BDRV_SECTOR_SIZE; const uint8_t *buf1; BlockDriverInfo bdi; - QEMUOptionParameter *param = NULL, *create_options = NULL; - QEMUOptionParameter *out_baseimg_param; + QemuOpts *opts = NULL; + QemuOptsList *create_opts = NULL; + const char *out_baseimg_param; char *options = NULL; const char *snapshot_name = NULL; int min_sparse = 8; /* Need at least 4k of zeros for sparse detection */ @@ -1340,40 +1340,36 @@ static int img_convert(int argc, char **argv) goto out; } - create_options = append_option_parameters(create_options, - drv->create_options); - create_options = append_option_parameters(create_options, - proto_drv->create_options); + create_opts = qemu_opts_append(create_opts, drv->create_opts, + drv->create_options); + create_opts = qemu_opts_append(create_opts, proto_drv->create_opts, + proto_drv->create_options); - 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); + ret = -1; + goto out; } - set_option_parameter_int(param, BLOCK_OPT_SIZE, total_sectors * 512); - ret = add_old_style_options(out_fmt, param, out_baseimg, NULL); + qemu_opt_set_number(opts, BLOCK_OPT_SIZE, total_sectors * 512); + ret = add_old_style_options(out_fmt, opts, out_baseimg, NULL); if (ret < 0) { goto out; } /* Get backing file name if -o backing_file was used */ - out_baseimg_param = get_option_parameter(param, BLOCK_OPT_BACKING_FILE); + out_baseimg_param = qemu_opt_get(opts, BLOCK_OPT_BACKING_FILE); if (out_baseimg_param) { - out_baseimg = out_baseimg_param->value.s; + out_baseimg = out_baseimg_param; } /* Check if compression is supported */ if (compress) { - QEMUOptionParameter *encryption = - get_option_parameter(param, BLOCK_OPT_ENCRYPT); - QEMUOptionParameter *preallocation = - get_option_parameter(param, BLOCK_OPT_PREALLOC); + bool encryption = + qemu_opt_get_bool(opts, BLOCK_OPT_ENCRYPT, false); + const char *preallocation = + qemu_opt_get(opts, BLOCK_OPT_PREALLOC); if (!drv->bdrv_write_compressed) { error_report("Compression not supported for this file format"); @@ -1381,15 +1377,15 @@ static int img_convert(int argc, char **argv) goto out; } - if (encryption && encryption->value.n) { + if (encryption) { error_report("Compression and encryption not supported at " "the same time"); ret = -1; goto out; } - if (preallocation && preallocation->value.s - && strcmp(preallocation->value.s, "off")) + if (preallocation + && strcmp(preallocation, "off")) { error_report("Compression and preallocation not supported at " "the same time"); @@ -1400,7 +1396,7 @@ static int img_convert(int argc, char **argv) if (!skip_create) { /* Create the new image */ - ret = bdrv_create(drv, out_filename, param, &local_err); + ret = bdrv_create(drv, out_filename, NULL, opts, &local_err); if (ret < 0) { error_report("%s: error while converting %s: %s", out_filename, out_fmt, error_get_pretty(local_err)); @@ -1664,8 +1660,8 @@ out: qemu_progress_print(100, 0); } qemu_progress_end(); - free_option_parameters(create_options); - free_option_parameters(param); + qemu_opts_del(opts); + qemu_opts_free(create_opts); qemu_vfree(buf); if (sn_opts) { qemu_opts_del(sn_opts); @@ -2652,7 +2648,8 @@ static int img_amend(int argc, char **argv) { int c, ret = 0; char *options = NULL; - QEMUOptionParameter *create_options = NULL, *options_param = NULL; + QemuOptsList *create_opts = NULL; + QemuOpts *opts = NULL; const char *fmt = NULL, *filename; bool quiet = false; BlockDriverState *bs = NULL; @@ -2722,17 +2719,16 @@ 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) { + create_opts = qemu_opts_append(create_opts, bs->drv->create_opts, + bs->drv->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); + ret = bdrv_amend_options(bs, NULL, opts); if (ret < 0) { error_report("Error while amending options: %s", strerror(-ret)); goto out; @@ -2742,8 +2738,8 @@ out: if (bs) { bdrv_unref(bs); } - free_option_parameters(create_options); - free_option_parameters(options_param); + qemu_opts_del(opts); + qemu_opts_free(create_opts); g_free(options); if (ret) {