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

[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.

[...]