Re: [Qemu-devel] [PATCH V11 3/4] Use QemuOpts support in block layer

[Dong Xu Wang]

于 2013-1-29 17:45, Markus Armbruster 写道:
> Dong Xu Wang <address@hidden> writes:
> 
>> Markus Armbruster <address@hidden> writes:
>>> Dong Xu Wang <address@hidden> writes:
>>>
>>>> Markus Armbruster <address@hidden> writes:
>>>>> Dong Xu Wang <address@hidden> 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.
> 
> 
>