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

[Markus Armbruster]

Dong Xu Wang <address@hidden> writes:

> On Thu, Mar 21, 2013 at 11:31 PM, Markus Armbruster <address@hidden> wrote:
>> Dong Xu Wang <address@hidden> writes:
[...]
>>> diff --git a/block.c b/block.c
>>> index 4582961..975c3d8 100644
>>> --- a/block.c
>>> +++ b/block.c
[...]
>>> @@ -4521,28 +4522,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_opts = qemu_opts_append(drv->bdrv_create_opts,
>>> +                                   proto_drv->bdrv_create_opts);
>>
>> qemu_opts_append() dereferences its arguments to compute
>>
>>     dest->merge_lists = first->merge_lists || second->merge_lists;
>>
>> However, either of drv->create_opts and proto_drv->create_opts may be
>> null, as far as I can see.  If I'm correct, you need a few more test
>> cases :)
>>
> Okay, will check first and second.
>> 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?  Beyond the
>> scope of this patch.
>>
>>>      /* Create parameter list with default values */
>>> -    param = parse_option_parameters("", create_options, param);
>>> +    opts = qemu_opts_create_nofail(create_opts);
>>
>> Before: param empty.
>>
>> After: opts empty.
>>
>> Good.
>>
>>>
>>> -    set_option_parameter_int(param, BLOCK_OPT_SIZE, img_size);
>>> +    qemu_opt_set_number(opts, BLOCK_OPT_SIZE, img_size);
>>
>> Before: param contains exactly BLOCK_OPT_SIZE = img_size.
>>
>> After: opts contains exactly BLOCK_OPT_SIZE = img_size.
>>
>> Both old and new code seem to assume BLOCK_OPT_SIZE is always a valid
>> option.  Beyond the scope of this patch.
>>
>> Good.
>>
>>>
>>>      /* Parse -o options */
>>>      if (options) {
>>> -        param = parse_option_parameters(options, create_options, param);
>>> -        if (param == NULL) {
>>> +        if (qemu_opts_do_parse_replace(opts, options, NULL) != 0) {
>>>              error_setg(errp, "Invalid options for file format '%s'.", fmt);
>>>              goto out;
>>>          }
>>>      }
>>
>> Before: values from -o replace whatever is in param already.
>>
>> After: values from -o replace whatever is in opts already.
>>
>> In particular, size from -o replaces img_size before and after.
>>
>> Did you test it does?
>>
>>     $ qemu-img create -f raw -o size=1024 t.img 512
>>     Formatting 't.img', fmt=raw size=1024
>>     $ ls -l t.img
>>     -rw-r--r--. 1 armbru armbru 1024 Mar 21 15:12 t.img
>>
>>>
>>>      if (base_filename) {
>>> -        if (set_option_parameter(param, BLOCK_OPT_BACKING_FILE,
>>> +        if (qemu_opt_replace_set(opts, BLOCK_OPT_BACKING_FILE,
>>>                                   base_filename)) {
>>>              error_setg(errp, "Backing file not supported for file format 
>>> '%s'",
>>>                         fmt);
>>
>> Before: base_filename replaces any backing_file from -o in param.
>>
>> After: base_filename replaces any backing_file from -o in opts.
>>
>> Did you test it does?
>>
> Oh, I did not test command line like this, my patch will fail here.
> Will fix in next version.
>
>>     $ qemu-img create -f qcow2 -b t.img -o backing_file=/dev/null t.qcow2
>>     Formatting 't.qcow2', fmt=qcow2 size=1024 backing_file='t.img' 
>> encryption=off cluster_size=65536 lazy_refcounts=off
>>     $ qemu-img info t.qcow2image: t.qcow2
>>     file format: qcow2
>>     virtual size: 1.0K (1024 bytes)
>>     disk size: 196K
>>     cluster_size: 65536
>>     backing file: t.img
>>
>
> I run this command using upstream QEMU code, but failed.
> address@hidden@VM:~]$ qemu-img info t.qcow2image: t.qcow2
> qemu-img: Could not open 't.qcow2image:': No such file or directory
>
> Or did I type something wrong?

Looks like I pastoed.  Please try "qemu-img info t.qcow2".

[...]
>>> diff --git a/block/cow.c b/block/cow.c
>>> index 4baf904..135fa33 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,11 @@ 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 */
>>> +    image_sectors = qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0) / 512;
>>> +    image_filename = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FILE);
>>
>> Why _del?
>>
>
> Before: after getting one parameter, then option++, so in my patches, I want 
> to
> delete the opt after using it. If I did not delete it, some parameters
> will be passed
> to "protocol", it will go wrong. Do you think it is acceptable?

I missed the fact that the old code changes its options parameter before
it passes it to bdrv_create_file().  Let's examine how it works.

options is either null an array of QEMUOptionParameter terminated by an
sentinel element with a null name.

The loop steps through options until it hits the sentinel.
Postcondition: !options || !options->name.  In words: either no options,
or an empty array of options.

Then:

>>>
>>> -    ret = bdrv_create_file(filename, options);
>>> +    ret = bdrv_create_file(filename, opts);
>>>      if (ret < 0) {
>>>          return ret;
>>>      }

We pass either no options or an empty array of options to
bdrv_create_file().  I suspect we could just as well pass NULL.

If I'm right, you should pass NULL instead of opts.  No reason to change
opts then.

>>> @@ -318,18 +312,22 @@ exit:
>>>      return ret;
>>>  }

[...]
>
> Really thanks for your reviewing, Markus.

You're welcome!