Dong Xu Wang <wdon...@linux.vnet.ibm.com> writes: > Markus Armbruster <arm...@redhat.com> writes: >> Dong Xu Wang <wdon...@linux.vnet.ibm.com> writes: >> >>> Markus Armbruster <arm...@redhat.com> writes: >>>> Dong Xu Wang <wdon...@vnet.linux.ibm.com> writes: [...] >>>>> @@ -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 see. > I want to make these code in only one kind of style, so I judged if opts > is NULL in block format code. As far as I can tell, there are just two sources of NULL QEMUOptionParameter *: 1. parse_option_parameters() returns NULL on error (documented), but some callers use it like this without checking for errors: param = parse_option_parameters("", create_options, param); The only error that can happen then is null create_options. Depending on that is slightly unclean. Your patch replaces these uses by opts = qemu_opts_create_nofail(create_options); which cannot return NULL. I'd call that an improvement. 2. qed_create() calls bdrv_create_file(filename, NULL), which passes on the null options to bdrv_create(). To get rid of this source, one of the two functions needs to create empty options. Your patch adds a third source: 3. bdrv_img_create() merges drv->create_options and proto_drv->create_options (both possibly null) into create_options. Unlike the current code, your patch merges two empty lists into null. I don't think that's a wise change, and asked you to drop this special case in my review of PATCH 2/4. [...]