[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH V12 3/4] Use QemuOpts support in block layer
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH V12 3/4] Use QemuOpts support in block layer |
Date: |
Tue, 02 Apr 2013 18:22:48 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.1 (gnu/linux) |
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!
[Prev in Thread] |
Current Thread |
[Next in Thread] |
- Re: [Qemu-devel] [PATCH V12 3/4] Use QemuOpts support in block layer,
Markus Armbruster <=