δΊ 2013-1-29 17:45, Markus Armbruster ει:
> 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.
>
> [...]
Got it, thank you Markus, I will consider your comments in next version.
>
>
>
