δΊ 2013-1-29 1:41, Markus Armbruster ει: > Dong Xu Wang <wdon...@linux.vnet.ibm.com> writes: > >> Markus Armbruster <arm...@redhat.com> writes: >>> Dong Xu Wang <wdon...@vnet.linux.ibm.com> writes: >>> >>>> This patch will use QemuOpts related functions in block layer, add >>>> a member bdrv_create_options to BlockDriver struct, it will return >>>> a QemuOptsList pointer, which includes the image format's create >>>> options. >>>> >>>> And create options's primary consumer is block creating related functions, >>>> so modify them together. >>>> >>>> >>>> Signed-off-by: Dong Xu Wang <wdon...@linux.vnet.ibm.com> >>>> --- >>>> v10->v11) >>>> 1) qed.h move QED_DEFAULT_CLUSTER_SIZE from enum to macro, or >>>> qemu_opts_print produce un-expanded cluster_size. >>>> 2) In qcow2.c and qcow.c, bdrv_create_file(filename, NULL), NULL -> opts, >>>> or while using protocol, there will be an error. >>>> >>>> v8->v9) >>>> 1) add qemu_ prefix to gluster_create_opts. >>>> 2) fix bug: bdrv_gluster_unix and bdrv_gluster_rdma should also be >>>> converted. >>>> >>>> v7->v8) >>>> 1) rebase to upstream source tree. >>>> 2) add gluster.c, raw-win32.c, and rbd.c. >>>> >>>> v6->v7: >>>> 1) use osdep.h:stringify(), not redefining new macro. >>>> 2) preserve TODO comment. >>>> 3) fix typo. BLOCK_OPT_ENCRYPT->BLOCK_OPT_STATIC. >>>> 4) initialize disk_type even when opts is NULL. >>>> >>>> v5->v6: >>>> 1) judge if opts == NULL in block layer create functions. >>>> 2) use bdrv_create_file(filename, NULL) in qcow_create and cow_create >>>> funtion. >>>> 3) made more readable while using qemu_opt_get_number. >>>> >>>> >>>> block.c | 91 ++++++++++++------------ >>>> block/cow.c | 46 ++++++------- >>>> block/gluster.c | 37 +++++----- >>>> block/qcow.c | 60 ++++++++-------- >>>> block/qcow2.c | 171 >>>> +++++++++++++++++++++++----------------------- >>>> block/qed.c | 86 +++++++++++------------ >>>> block/qed.h | 2 +- >>>> block/raw-posix.c | 59 ++++++++-------- >>>> block/raw-win32.c | 30 ++++---- >>>> block/raw.c | 30 ++++---- >>>> block/rbd.c | 62 ++++++++--------- >>>> block/sheepdog.c | 75 ++++++++++---------- >>>> block/vdi.c | 69 +++++++++---------- >>>> block/vmdk.c | 74 ++++++++++---------- >>>> block/vpc.c | 67 +++++++++--------- >>>> block/vvfat.c | 11 +-- >>>> include/block/block.h | 4 +- >>>> include/block/block_int.h | 6 +- >>>> qemu-img.c | 61 ++++++++--------- >>>> 19 files changed, 519 insertions(+), 522 deletions(-) >>>> >>>> diff --git a/block.c b/block.c >>>> index 6fa7c90..56e4613 100644 >>>> --- a/block.c >>>> +++ b/block.c >>>> @@ -357,7 +357,7 @@ BlockDriver *bdrv_find_whitelisted_format(const char >>>> *format_name) >>>> typedef struct CreateCo { >>>> BlockDriver *drv; >>>> char *filename; >>>> - QEMUOptionParameter *options; >>>> + QemuOpts *opts; >>>> int ret; >>>> } CreateCo; >>>> >>>> @@ -366,11 +366,11 @@ static void coroutine_fn bdrv_create_co_entry(void >>>> *opaque) >>>> CreateCo *cco = opaque; >>>> assert(cco->drv); >>>> >>>> - cco->ret = cco->drv->bdrv_create(cco->filename, cco->options); >>>> + cco->ret = cco->drv->bdrv_create(cco->filename, cco->opts); >>>> } >>>> >>>> int bdrv_create(BlockDriver *drv, const char* filename, >>>> - QEMUOptionParameter *options) >>>> + QemuOpts *opts) >>>> { >>>> int ret; >>>> >>>> @@ -378,7 +378,7 @@ int bdrv_create(BlockDriver *drv, const char* filename, >>>> CreateCo cco = { >>>> .drv = drv, >>>> .filename = g_strdup(filename), >>>> - .options = options, >>>> + .opts = opts, >>>> .ret = NOT_DONE, >>>> }; >>>> >>>> @@ -405,7 +405,7 @@ out: >>>> return ret; >>>> } >>>> >>>> -int bdrv_create_file(const char* filename, QEMUOptionParameter *options) >>>> +int bdrv_create_file(const char *filename, QemuOpts *opts) >>>> { >>>> BlockDriver *drv; >>>> >>>> @@ -414,7 +414,7 @@ int bdrv_create_file(const char* filename, >>>> QEMUOptionParameter *options) >>>> return -ENOENT; >>>> } >>>> >>>> - return bdrv_create(drv, filename, options); >>>> + return bdrv_create(drv, filename, opts); >>>> } >>>> >>>> /* >>>> @@ -794,7 +794,7 @@ int bdrv_open(BlockDriverState *bs, const char >>>> *filename, int flags, >>>> int64_t total_size; >>>> int is_protocol = 0; >>>> BlockDriver *bdrv_qcow2; >>>> - QEMUOptionParameter *options; >>>> + QemuOpts *opts; >>>> char backing_filename[PATH_MAX]; >>>> >>>> /* if snapshot, we create a temporary backing file and open it >>>> @@ -827,17 +827,16 @@ int bdrv_open(BlockDriverState *bs, const char >>>> *filename, int flags, >>>> return -errno; >>>> >>>> bdrv_qcow2 = bdrv_find_format("qcow2"); >>>> - options = parse_option_parameters("", bdrv_qcow2->create_options, >>>> NULL); >>>> + opts = qemu_opts_create_nofail(bdrv_qcow2->bdrv_create_options); >>>> >>>> - set_option_parameter_int(options, BLOCK_OPT_SIZE, total_size); >>>> - set_option_parameter(options, BLOCK_OPT_BACKING_FILE, >>>> backing_filename); >>>> + qemu_opt_set_number(opts, BLOCK_OPT_SIZE, total_size); >>>> + qemu_opt_set(opts, BLOCK_OPT_BACKING_FILE, backing_filename); >>>> if (drv) { >>>> - set_option_parameter(options, BLOCK_OPT_BACKING_FMT, >>>> - drv->format_name); >>>> + qemu_opt_set(opts, BLOCK_OPT_BACKING_FMT, drv->format_name); >>>> } >>>> >>>> - ret = bdrv_create(bdrv_qcow2, tmp_filename, options); >>>> - free_option_parameters(options); >>>> + ret = bdrv_create(bdrv_qcow2, tmp_filename, opts); >>>> + qemu_opts_del(opts); >>>> if (ret < 0) { >>>> return ret; >>>> } >>>> @@ -4493,8 +4492,10 @@ void bdrv_img_create(const char *filename, const >>>> char *fmt, >>>> const char *base_filename, const char *base_fmt, >>>> char *options, uint64_t img_size, int flags, Error >>>> **errp) >>>> { >>>> - QEMUOptionParameter *param = NULL, *create_options = NULL; >>>> - QEMUOptionParameter *backing_fmt, *backing_file, *size; >>>> + QemuOpts *opts = NULL; >>>> + QemuOptsList *create_options = NULL; >>>> + const char *backing_fmt, *backing_file; >>>> + int64_t size; >>>> BlockDriverState *bs = NULL; >>>> BlockDriver *drv, *proto_drv; >>>> BlockDriver *backing_drv = NULL; >>>> @@ -4512,28 +4513,23 @@ void bdrv_img_create(const char *filename, const >>>> char *fmt, >>>> error_setg(errp, "Unknown protocol '%s'", filename); >>>> return; >>>> } >>>> - >>>> - create_options = append_option_parameters(create_options, >>>> - drv->create_options); >>>> - create_options = append_option_parameters(create_options, >>>> - proto_drv->create_options); >>>> - >>>> + create_options = append_opts_list(drv->bdrv_create_options, >>>> + proto_drv->bdrv_create_options); >>> >>> Before: format's options get appended first, then protocol's options. >>> Because get_option_parameter() searches in order, format options shadow >>> protocol options. >>> >>> After: append_opts_list() gives first argument's options precedence. >>> >>> Thus, no change. Good. >>> >>> Should bdrv_create_options option name clashes be avoided? Outside the >>> scope of this patch. >> Sorry, I do not understand this line, clash? Can you explain some more? >> Do you mean bdrv_create_options should be renamed such as bdrv_create_opts? > > No. I was talking about what happens when drv->create_options and > proto->create_options both have an entry with the same name. > > First, I reasoned why your patch doesn't change the way such a clash is > handled. > > Then I wondered whether we should avoid such clashes, but hastened to > add that I'm not asking you to do that now. > >>>> /* Create parameter list with default values */ >>>> - param = parse_option_parameters("", create_options, param); >>>> + opts = qemu_opts_create_nofail(create_options); >>>> >>>> - set_option_parameter_int(param, BLOCK_OPT_SIZE, img_size); >>>> + 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; >>>> } >>>> } >>> >>> Before: size from -o replaces img_size in param. >>> >>> After: size from -o gets appended to opts, is therefore shadowed by >>> img_size. I think. >>> >>> User-visible change, if my reading is correct. Should be avoided. >>> >> Okay. will also "replace", not "append". > > Prepending could also work. > > Doing things in a different order could make appending work. > > Whatever is easiest. > >>>> >>>> if (base_filename) { >>>> - if (set_option_parameter(param, BLOCK_OPT_BACKING_FILE, >>>> + if (qemu_opt_set(opts, BLOCK_OPT_BACKING_FILE, >>>> base_filename)) { >>>> error_setg(errp, "Backing file not supported for file >>>> format '%s'", >>>> fmt); >>> >>> Before: base_filename replaces the backing_file from -o. >>> >>> After: base_filename gets appended to opts, shadowed by backing_file >>> from -o. >>> >>>> @@ -4542,39 +4538,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; >>>> } >>>> } >>> >>> Likewise. >>> >>>> >>>> - 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); >>>> + error_setg(errp, "Unknown backing file format '%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_number(opts, BLOCK_OPT_SIZE, -1); >>>> + if (size == -1) { >>>> + if (backing_file) { >>> >>> Code has the same before your patch, so it's not your fault, but here >>> goes anyway: >>> >>> QemuOpts support only *unsigned* numbers. Argument -1 is for an >>> uint64_t parameter, and gets converted to UINT64_MAX. If the option >>> isn't set, it's returned. The assignment to int64_t size converts it >>> back to -1. >>> >>> Using the an honest UINT64_MAX instead of -1 would be clearer. Should >>> make size uint64_t then. >>> >>> However, all other places do >>> >>> qemu_opt_get_number(opts, BLOCK_OPT_SIZE, 0) >>> >>> Why is this one different? >>> >> Because previous use -1, so, I passed default value -1. > > I have no idea what you're talking about, and I'm not 100% sure you do > :) > > The last argument to qemu_opt_get_number() is *not* used here. That's > because a few lines above, you do > > qemu_opt_set_number(opts, BLOCK_OPT_SIZE, img_size); > > So, opts always has the BLOCK_OPT_SIZE option, and the default value > never applies. > > Let's take a step back and figure out how the current code works before > we further review your patch. > > bdrv_img_create() merges parameters img_size, base_filename, base_fmt > with options as follows: > > 1. Get format and protocol driver from parameters @fmt and @filename. > > 2. Combine the two drivers' option lists. > > 3. Copy the resulting option list for no discernible reason. > > 4. If %BLOCK_OPT_SIZE is a valid option, set its value to parameter > @img_size. > > 5. For all KEY=VAL in parameter @options: > KEY must be a valid option (error out if not) > set its value to VAL > > 6. If parameter @base_filename isn't null: > %BLOCK_OPT_BACKING_FILE must be a valid option (error out if not) > set its value to @base_filename > > 7. If parameter @base_fmt isn't null: > %BLOCK_OPT_BACKING_FMT must be a valid option (error out if not) > set its value to @base_fmt > > In short, @base_filename and @base_fmt take precedence over @options > (unless they're null, of course). @options takes precedence over > @img_size. What a mess :) Not your fault. > > Now let's look at the code retrieving BLOCK_OPT_SIZE: > > // 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) { > // compute size, put it into buf > set_option_parameter(param, BLOCK_OPT_SIZE, buf); > } else { > // error out > } > > If %BLOCK_OPT_SIZE is a valid option, and has the special value > (uint64_t)-1, then we must have a backing file (error out if not), and > we use its size instead. In other words, the special value means "use > the backing file's size". > > The value can come either from @img_size or from @options. > > Some callers indeed pass (uint64_t)-1 for @img_size. For instance, > qemu-img create passes it when run without the optional size argument. > Works. > > qemu-img is the only caller passing @options. With -o size=-1, it > actually puts (uint64_t)-1 into @param (parse_option_size() sucks). > Weird. > > The whole bloody mess should be cleaned up. But I'm not asking you to > do that now. > > I do think, however, that you should stick to > > qemu_opt_get_number(opts, BLOCK_OPT_SIZE, 0) > > instead of your voodoo > > qemu_opt_get_number(opts, BLOCK_OPT_SIZE, -1) > >>>> uint64_t size; >>>> - char buf[32]; >>>> int back_flags; >>>> >>>> /* backing files always opened read-only */ >>>> @@ -4583,17 +4577,16 @@ void bdrv_img_create(const char *filename, const >>>> char *fmt, >>>> >>>> bs = bdrv_new(""); >>>> >>>> - ret = bdrv_open(bs, backing_file->value.s, back_flags, >>>> backing_drv); >>>> + ret = bdrv_open(bs, backing_file, back_flags, backing_drv); >>>> if (ret < 0) { >>>> error_setg_errno(errp, -ret, "Could not open '%s'", >>>> - backing_file->value.s); >>>> + backing_file); >>>> goto out; >>>> } >>>> bdrv_get_geometry(bs, &size); >>>> 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); >>>> } else { >>>> error_setg(errp, "Image creation needs a size parameter"); >>>> goto out; >>>> @@ -4601,10 +4594,10 @@ void bdrv_img_create(const char *filename, const >>>> char *fmt, >>>> } >>>> >>>> printf("Formatting '%s', fmt=%s ", filename, fmt); >>>> - print_option_parameters(param); >>>> + qemu_opts_print(opts, NULL); >>>> puts(""); >>>> >>>> - ret = bdrv_create(drv, filename, param); >>>> + ret = bdrv_create(drv, filename, opts); >>>> if (ret < 0) { >>>> if (ret == -ENOTSUP) { >>>> error_setg(errp,"Formatting or formatting option not >>>> supported for " >>>> @@ -4619,8 +4612,10 @@ void bdrv_img_create(const char *filename, const >>>> char *fmt, >>>> } >>>> >>>> out: >>>> - free_option_parameters(create_options); >>>> - free_option_parameters(param); >>>> + free_opts_list(create_options); >>>> + if (opts) { >>>> + qemu_opts_del(opts); >>>> + } >>>> >>>> if (bs) { >>>> bdrv_delete(bs); >>>> diff --git a/block/cow.c b/block/cow.c >>>> index a33ce95..5442c9c 100644 >>>> --- a/block/cow.c >>>> +++ b/block/cow.c >>>> @@ -255,7 +255,7 @@ static void cow_close(BlockDriverState *bs) >>>> { >>>> } >>>> >>>> -static int cow_create(const char *filename, QEMUOptionParameter *options) >>>> +static int cow_create(const char *filename, QemuOpts *opts) >>>> { >>>> struct cow_header_v2 cow_header; >>>> struct stat st; >>>> @@ -264,17 +264,13 @@ static int cow_create(const char *filename, >>>> QEMUOptionParameter *options) >>>> int ret; >>>> BlockDriverState *cow_bs; >>>> >>>> - /* Read out options */ >>>> - while (options && options->name) { >>>> - if (!strcmp(options->name, BLOCK_OPT_SIZE)) { >>>> - image_sectors = options->value.n / 512; >>>> - } else if (!strcmp(options->name, BLOCK_OPT_BACKING_FILE)) { >>>> - image_filename = options->value.s; >>>> - } >>>> - options++; >>>> + /* Read out opts */ >>>> + if (opts) { >>> >>> I suspect you need the if (opts) here and in many other places only >>> because you special-cased "both lists empty" in append_opts_list(). I >>> suspect things become easier if you drop that. >> >> No, in this version, if(opt) is needed in "protocol", not needed in >> "format", I want to have the same code style, so I also judged if opts >> is NULL in "format" _create functions. Do you think is it acceptable? > > I still don't get it. Can you explain how opts can be null here even > when append_opts_list() is changed not to return null? > I mean: When I use protocol, such as gluster: /usr/bin/qemu-img create -f qed -o cluster_size=65536 gluster://kvm11/single-volume/12.qed 10M
Then opts will be null: qemu_gluster_create (filename=0x555555c11930 "gluster://kvm11/single-volume/12.qed", opts=0x0) at block/gluster.c:339 So it checked if opts is NULL now: 352 if (opts) { 353 total_size = 354 qemu_opt_get_number(opts, BLOCK_OPT_SIZE, 0) / BDRV_SECTOR_SIZE; 355 } I want to make these code in only one kind of style, so I judged if opts is NULL in block format code. >>> >>>> + image_sectors = qemu_opt_get_number(opts, BLOCK_OPT_SIZE, 0) / >>>> 512; >>>> + image_filename = qemu_opt_get(opts, BLOCK_OPT_BACKING_FILE); >>>> } >>>> >>>> - ret = bdrv_create_file(filename, options); >>>> + ret = bdrv_create_file(filename, opts); >>>> if (ret < 0) { >>>> return ret; >>>> } >>>> @@ -318,18 +314,22 @@ exit: >>>> return ret; >>>> } >>>> >>>> -static QEMUOptionParameter cow_create_options[] = { >>>> - { >>>> - .name = BLOCK_OPT_SIZE, >>>> - .type = OPT_SIZE, >>>> - .help = "Virtual disk size" >>>> - }, >>>> - { >>>> - .name = BLOCK_OPT_BACKING_FILE, >>>> - .type = OPT_STRING, >>>> - .help = "File name of a base image" >>>> - }, >>>> - { NULL } >>>> +static QemuOptsList cow_create_opts = { >>>> + .name = "cow-create-opts", >>>> + .head = QTAILQ_HEAD_INITIALIZER(cow_create_opts.head), >>>> + .desc = { >>>> + { >>>> + .name = BLOCK_OPT_SIZE, >>>> + .type = QEMU_OPT_NUMBER, >>> >>> QEMU_OPT_SIZE, or else we lose support for suffixes, don't we? >>> >> In qemu-img.c, strtosz_suffix convert suffixes to regular digitals. > > That's correct for the optional size argument, but not for -o size=... > >> Okay. I will change to QEMU_OPT_SIZE. > > Please do, and test that both the size argument and -o size=... works > just like before, with and without suffixes. > >>>> + .help = "Virtual disk size" >>>> + }, >>>> + { >>>> + .name = BLOCK_OPT_BACKING_FILE, >>>> + .type = QEMU_OPT_STRING, >>>> + .help = "File name of a base image" >>>> + }, >>>> + { /* end of list */ } >>>> + } >>>> }; >>>> >>>> static BlockDriver bdrv_cow = { >>>> @@ -345,7 +345,7 @@ static BlockDriver bdrv_cow = { >>>> .bdrv_write = cow_co_write, >>>> .bdrv_co_is_allocated = cow_co_is_allocated, >>>> >>>> - .create_options = cow_create_options, >>>> + .bdrv_create_options = &cow_create_opts, >>>> }; >>>> >>>> static void bdrv_cow_init(void) >>>> diff --git a/block/gluster.c b/block/gluster.c >>>> index 0f2c32a..a41c684 100644 >>>> --- a/block/gluster.c >>>> +++ b/block/gluster.c >>>> @@ -335,8 +335,7 @@ out: >>>> return ret; >>>> } >>>> >>>> -static int qemu_gluster_create(const char *filename, >>>> - QEMUOptionParameter *options) >>>> +static int qemu_gluster_create(const char *filename, QemuOpts* opts) >>> >>> Space before the *, not after: QemuOpts *opts >>> >> Okay. > > [...] > > >