| Message ID | 1394436721-21812-8-git-send-email-cyliu@suse.com |
|---|---|
| State | New |
| Headers | show |
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 <wdongxu@linux.vnet.ibm.com> > Signed-off-by: Chunyan Liu <cyliu@suse.com> > --- > @@ -420,7 +421,11 @@ 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); > + if (cco->drv->bdrv_create2) { > + ret = cco->drv->bdrv_create2(cco->filename, cco->opts, &local_err); Is it worth assert(!cco->options) here, > + } else { > + ret = cco->drv->bdrv_create(cco->filename, cco->options, &local_err); and assert(!cco->opts) here, to ensure that we aren't ignoring options from the other style of create? Then again, it gets cleaned up in later patches. > @@ -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); Memory leak. As implemented earlier in the series, qemu_opts_append() creates a new object, but as you are overwriting create_opts with the second result without hanging onto the pointer returned by the first call, you have just leaked the first instance. I ran out of time to review further today, but this series still needs more work, and I am starting to doubt that it will make 2.0.
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 <wdongxu@linux.vnet.ibm.com> > Signed-off-by: Chunyan Liu <cyliu@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.
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 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> > > --- > > > 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 > >
diff --git a/block.c b/block.c index f1ef4b0..d9b1da3 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; @@ -420,7 +421,11 @@ 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); + if (cco->drv->bdrv_create2) { + ret = cco->drv->bdrv_create2(cco->filename, cco->opts, &local_err); + } else { + ret = cco->drv->bdrv_create(cco->filename, cco->options, &local_err); + } if (local_err) { error_propagate(&cco->err, local_err); } @@ -428,7 +433,7 @@ 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 +442,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 +479,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 +491,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); } @@ -1248,7 +1254,8 @@ int bdrv_open(BlockDriverState **pbs, const char *filename, BlockDriverState *bs1; int64_t total_size; BlockDriver *bdrv_qcow2; - QEMUOptionParameter *create_options; + QEMUOptionParameter *create_options = NULL; + QemuOpts *opts = NULL; QDict *snapshot_options; /* if snapshot, we create a temporary backing file and open it @@ -1274,13 +1281,21 @@ int bdrv_open(BlockDriverState **pbs, const char *filename, } 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); + if (bdrv_qcow2->bdrv_create2) { + opts = qemu_opts_create(bdrv_qcow2->create_opts, NULL, 0, + &error_abort); + qemu_opt_set_number(opts, BLOCK_OPT_SIZE, total_size); + } else { + 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); + ret = bdrv_create(bdrv_qcow2, tmp_filename, create_options, opts, + &local_err); free_option_parameters(create_options); + qemu_opts_del(opts); if (ret < 0) { error_setg_errno(errp, -ret, "Could not create temporary overlay " "'%s': %s", tmp_filename, @@ -5229,7 +5244,10 @@ void bdrv_img_create(const char *filename, const char *fmt, 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; @@ -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); + } else { + create_options = append_option_parameters(create_options, + drv->create_options); + create_options = append_option_parameters(create_options, + proto_drv->create_options); + create_opts = params_to_opts(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; @@ -5277,37 +5298,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]; @@ -5318,11 +5339,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; @@ -5332,7 +5353,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 { @@ -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); + } + 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'" @@ -5365,6 +5393,8 @@ 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); } @@ -5382,12 +5412,17 @@ 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) { + 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); + } } /* Used to recurse on single child block filters. 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 1e128be..153b9bd 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 cfe80be..dcd43bc 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -1487,7 +1487,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 8802ad3..db27f36 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 5390ba6..d8afb42 100644 --- a/block/vhdx.c +++ b/block/vhdx.c @@ -1788,7 +1788,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 f966ea5..ee32b3c 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 780f48b..c482204 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); @@ -283,7 +283,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 */ diff --git a/include/block/block_int.h b/include/block/block_int.h index 0bcf1c9..e4e832f 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,7 @@ struct BlockDriver { /* List of options for creating images, terminated by name == NULL */ QEMUOptionParameter *create_options; - + QemuOptsList *create_opts; /* * Returns 0 for completed check, -errno for internal errors. @@ -228,6 +230,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 2e40cc1..c548741 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -236,6 +236,7 @@ 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 +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); + } 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); + } free_option_parameters(create_options); + qemu_opts_free(create_opts); return 0; } @@ -311,19 +325,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; @@ -1154,7 +1168,9 @@ static int img_convert(int argc, char **argv) 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 +1356,42 @@ 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); - - 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; - } + 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); } else { - param = parse_option_parameters("", create_options, param); + create_options = append_option_parameters(create_options, + drv->create_options); + create_options = append_option_parameters(create_options, + proto_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'.", 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 +1399,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 +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); + } if (ret < 0) { error_report("%s: error while converting %s: %s", out_filename, out_fmt, error_get_pretty(local_err)); @@ -1666,6 +1689,8 @@ out: qemu_progress_end(); free_option_parameters(create_options); free_option_parameters(param); + qemu_opts_free(create_opts); + qemu_opts_del(opts); qemu_vfree(buf); if (sn_opts) { qemu_opts_del(sn_opts); @@ -2652,6 +2677,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; @@ -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) { + 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); + } if (ret < 0) { error_report("Error while amending options: %s", strerror(-ret)); goto out; @@ -2743,6 +2779,8 @@ out: } free_option_parameters(create_options); free_option_parameters(options_param); + qemu_opts_del(opts); + qemu_opts_free(create_opts); g_free(options); if (ret) {