Dong Xu Wang <wdon...@linux.vnet.ibm.com> writes:
> Markus Armbruster <arm...@redhat.com> writes:
>> Dong Xu Wang <wdon...@vnet.linux.ibm.com> writes:
>>
>>> This patch will use QemuOpts related functions in block layer, add
>>> a member bdrv_create_options to BlockDriver struct, it will return
>>> a QemuOptsList pointer, which includes the image format's create
>>> options.
>>>
>>> And create options's primary consumer is block creating related functions,
>>> so modify them together.
>>>
>>>
>>> Signed-off-by: Dong Xu Wang <wdon...@linux.vnet.ibm.com>
>>> ---
>>> v10->v11)
>>> 1) qed.h move QED_DEFAULT_CLUSTER_SIZE from enum to macro, or
>>> qemu_opts_print produce un-expanded cluster_size.
>>> 2) In qcow2.c and qcow.c, bdrv_create_file(filename, NULL), NULL -> opts,
>>> or while using protocol, there will be an error.
>>>
>>> v8->v9)
>>> 1) add qemu_ prefix to gluster_create_opts.
>>> 2) fix bug: bdrv_gluster_unix and bdrv_gluster_rdma should also be
>>> converted.
>>>
>>> v7->v8)
>>> 1) rebase to upstream source tree.
>>> 2) add gluster.c, raw-win32.c, and rbd.c.
>>>
>>> v6->v7:
>>> 1) use osdep.h:stringify(), not redefining new macro.
>>> 2) preserve TODO comment.
>>> 3) fix typo. BLOCK_OPT_ENCRYPT->BLOCK_OPT_STATIC.
>>> 4) initialize disk_type even when opts is NULL.
>>>
>>> v5->v6:
>>> 1) judge if opts == NULL in block layer create functions.
>>> 2) use bdrv_create_file(filename, NULL) in qcow_create and cow_create
>>> funtion.
>>> 3) made more readable while using qemu_opt_get_number.
>>>
>>>
>>> block.c | 91 ++++++++++++------------
>>> block/cow.c | 46 ++++++-------
>>> block/gluster.c | 37 +++++-----
>>> block/qcow.c | 60 ++++++++--------
>>> block/qcow2.c | 171
>>> +++++++++++++++++++++++-----------------------
>>> block/qed.c | 86 +++++++++++------------
>>> block/qed.h | 2 +-
>>> block/raw-posix.c | 59 ++++++++--------
>>> block/raw-win32.c | 30 ++++----
>>> block/raw.c | 30 ++++----
>>> block/rbd.c | 62 ++++++++---------
>>> block/sheepdog.c | 75 ++++++++++----------
>>> block/vdi.c | 69 +++++++++----------
>>> block/vmdk.c | 74 ++++++++++----------
>>> block/vpc.c | 67 +++++++++---------
>>> block/vvfat.c | 11 +--
>>> include/block/block.h | 4 +-
>>> include/block/block_int.h | 6 +-
>>> qemu-img.c | 61 ++++++++---------
>>> 19 files changed, 519 insertions(+), 522 deletions(-)
>>>
>>> diff --git a/block.c b/block.c
>>> index 6fa7c90..56e4613 100644
>>> --- a/block.c
>>> +++ b/block.c
>>> @@ -357,7 +357,7 @@ BlockDriver *bdrv_find_whitelisted_format(const char
>>> *format_name)
>>> typedef struct CreateCo {
>>> BlockDriver *drv;
>>> char *filename;
>>> - QEMUOptionParameter *options;
>>> + QemuOpts *opts;
>>> int ret;
>>> } CreateCo;
>>>
>>> @@ -366,11 +366,11 @@ static void coroutine_fn bdrv_create_co_entry(void
>>> *opaque)
>>> CreateCo *cco = opaque;
>>> assert(cco->drv);
>>>
>>> - cco->ret = cco->drv->bdrv_create(cco->filename, cco->options);
>>> + cco->ret = cco->drv->bdrv_create(cco->filename, cco->opts);
>>> }
>>>
>>> int bdrv_create(BlockDriver *drv, const char* filename,
>>> - QEMUOptionParameter *options)
>>> + QemuOpts *opts)
>>> {
>>> int ret;
>>>
>>> @@ -378,7 +378,7 @@ int bdrv_create(BlockDriver *drv, const char* filename,
>>> CreateCo cco = {
>>> .drv = drv,
>>> .filename = g_strdup(filename),
>>> - .options = options,
>>> + .opts = opts,
>>> .ret = NOT_DONE,
>>> };
>>>
>>> @@ -405,7 +405,7 @@ out:
>>> return ret;
>>> }
>>>
>>> -int bdrv_create_file(const char* filename, QEMUOptionParameter *options)
>>> +int bdrv_create_file(const char *filename, QemuOpts *opts)
>>> {
>>> BlockDriver *drv;
>>>
>>> @@ -414,7 +414,7 @@ int bdrv_create_file(const char* filename,
>>> QEMUOptionParameter *options)
>>> return -ENOENT;
>>> }
>>>
>>> - return bdrv_create(drv, filename, options);
>>> + return bdrv_create(drv, filename, opts);
>>> }
>>>
>>> /*
>>> @@ -794,7 +794,7 @@ int bdrv_open(BlockDriverState *bs, const char
>>> *filename, int flags,
>>> int64_t total_size;
>>> int is_protocol = 0;
>>> BlockDriver *bdrv_qcow2;
>>> - QEMUOptionParameter *options;
>>> + QemuOpts *opts;
>>> char backing_filename[PATH_MAX];
>>>
>>> /* if snapshot, we create a temporary backing file and open it
>>> @@ -827,17 +827,16 @@ int bdrv_open(BlockDriverState *bs, const char
>>> *filename, int flags,
>>> return -errno;
>>>
>>> bdrv_qcow2 = bdrv_find_format("qcow2");
>>> - options = parse_option_parameters("", bdrv_qcow2->create_options,
>>> NULL);
>>> + opts = qemu_opts_create_nofail(bdrv_qcow2->bdrv_create_options);
>>>
>>> - set_option_parameter_int(options, BLOCK_OPT_SIZE, total_size);
>>> - set_option_parameter(options, BLOCK_OPT_BACKING_FILE,
>>> backing_filename);
>>> + qemu_opt_set_number(opts, BLOCK_OPT_SIZE, total_size);
>>> + qemu_opt_set(opts, BLOCK_OPT_BACKING_FILE, backing_filename);
>>> if (drv) {
>>> - set_option_parameter(options, BLOCK_OPT_BACKING_FMT,
>>> - drv->format_name);
>>> + qemu_opt_set(opts, BLOCK_OPT_BACKING_FMT, drv->format_name);
>>> }
>>>
>>> - ret = bdrv_create(bdrv_qcow2, tmp_filename, options);
>>> - free_option_parameters(options);
>>> + ret = bdrv_create(bdrv_qcow2, tmp_filename, opts);
>>> + qemu_opts_del(opts);
>>> if (ret < 0) {
>>> return ret;
>>> }
>>> @@ -4493,8 +4492,10 @@ void bdrv_img_create(const char *filename, const
>>> char *fmt,
>>> const char *base_filename, const char *base_fmt,
>>> char *options, uint64_t img_size, int flags, Error
>>> **errp)
>>> {
>>> - QEMUOptionParameter *param = NULL, *create_options = NULL;
>>> - QEMUOptionParameter *backing_fmt, *backing_file, *size;
>>> + QemuOpts *opts = NULL;
>>> + QemuOptsList *create_options = NULL;
>>> + const char *backing_fmt, *backing_file;
>>> + int64_t size;
>>> BlockDriverState *bs = NULL;
>>> BlockDriver *drv, *proto_drv;
>>> BlockDriver *backing_drv = NULL;
>>> @@ -4512,28 +4513,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_options = append_opts_list(drv->bdrv_create_options,
>>> + proto_drv->bdrv_create_options);
>>
>> 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? Outside the
>> scope of this patch.
> Sorry, I do not understand this line, clash? Can you explain some more?
> Do you mean bdrv_create_options should be renamed such as bdrv_create_opts?
No. I was talking about what happens when drv->create_options and
proto->create_options both have an entry with the same name.
First, I reasoned why your patch doesn't change the way such a clash is
handled.
Then I wondered whether we should avoid such clashes, but hastened to
add that I'm not asking you to do that now.
>>> /* Create parameter list with default values */
>>> - param = parse_option_parameters("", create_options, param);
>>> + opts = qemu_opts_create_nofail(create_options);
>>>
>>> - set_option_parameter_int(param, BLOCK_OPT_SIZE, img_size);
>>> + qemu_opt_set_number(opts, BLOCK_OPT_SIZE, img_size);
>>>
>>> /* Parse -o options */
>>> if (options) {
>>> - param = parse_option_parameters(options, create_options, param);
>>> - if (param == NULL) {
>>> + if (qemu_opts_do_parse(opts, options, NULL) != 0) {
>>> error_setg(errp, "Invalid options for file format '%s'.",
>>> fmt);
>>> goto out;
>>> }
>>> }
>>
>> Before: size from -o replaces img_size in param.
>>
>> After: size from -o gets appended to opts, is therefore shadowed by
>> img_size. I think.
>>
>> User-visible change, if my reading is correct. Should be avoided.
>>
> Okay. will also "replace", not "append".
Prepending could also work.
Doing things in a different order could make appending work.
Whatever is easiest.
>>>
>>> if (base_filename) {
>>> - if (set_option_parameter(param, BLOCK_OPT_BACKING_FILE,
>>> + if (qemu_opt_set(opts, BLOCK_OPT_BACKING_FILE,
>>> base_filename)) {
>>> error_setg(errp, "Backing file not supported for file format
>>> '%s'",
>>> fmt);
>>
>> Before: base_filename replaces the backing_file from -o.
>>
>> After: base_filename gets appended to opts, shadowed by backing_file
>> from -o.
>>
>>> @@ -4542,39 +4538,37 @@ void bdrv_img_create(const char *filename, const
>>> char *fmt,
>>> }
>>>
>>> if (base_fmt) {
>>> - if (set_option_parameter(param, BLOCK_OPT_BACKING_FMT, base_fmt)) {
>>> + if (qemu_opt_set(opts, BLOCK_OPT_BACKING_FMT, base_fmt)) {
>>> error_setg(errp, "Backing file format not supported for file "
>>> "format '%s'", fmt);
>>> goto out;
>>> }
>>> }
>>
>> Likewise.
>>
>>>
>>> - backing_file = get_option_parameter(param, BLOCK_OPT_BACKING_FILE);
>>> - if (backing_file && backing_file->value.s) {
>>> - if (!strcmp(filename, backing_file->value.s)) {
>>> + backing_file = qemu_opt_get(opts, BLOCK_OPT_BACKING_FILE);
>>> + if (backing_file) {
>>> + if (!strcmp(filename, backing_file)) {
>>> error_setg(errp, "Error: Trying to create an image with the "
>>> "same filename as the backing file");
>>> goto out;
>>> }
>>> }
>>>
>>> - backing_fmt = get_option_parameter(param, BLOCK_OPT_BACKING_FMT);
>>> - if (backing_fmt && backing_fmt->value.s) {
>>> - backing_drv = bdrv_find_format(backing_fmt->value.s);
>>> + backing_fmt = qemu_opt_get(opts, BLOCK_OPT_BACKING_FMT);
>>> + if (backing_fmt) {
>>> + backing_drv = bdrv_find_format(backing_fmt);
>>> if (!backing_drv) {
>>> - error_setg(errp, "Unknown backing file format '%s'",
>>> - backing_fmt->value.s);
>>> + error_setg(errp, "Unknown backing file format '%s'",
>>> backing_fmt);
>>> goto out;
>>> }
>>> }
>>>
>>> // The size for the image must always be specified, with one
>>> exception:
>>> // If we are using a backing file, we can obtain the size from there
>>> - size = get_option_parameter(param, BLOCK_OPT_SIZE);
>>> - if (size && size->value.n == -1) {
>>> - if (backing_file && backing_file->value.s) {
>>> + size = qemu_opt_get_number(opts, BLOCK_OPT_SIZE, -1);
>>> + if (size == -1) {
>>> + if (backing_file) {
>>
>> Code has the same before your patch, so it's not your fault, but here
>> goes anyway:
>>
>> QemuOpts support only *unsigned* numbers. Argument -1 is for an
>> uint64_t parameter, and gets converted to UINT64_MAX. If the option
>> isn't set, it's returned. The assignment to int64_t size converts it
>> back to -1.
>>
>> Using the an honest UINT64_MAX instead of -1 would be clearer. Should
>> make size uint64_t then.
>>
>> However, all other places do
>>
>> qemu_opt_get_number(opts, BLOCK_OPT_SIZE, 0)
>>
>> Why is this one different?
>>
> Because previous use -1, so, I passed default value -1.
I have no idea what you're talking about, and I'm not 100% sure you do
:)
The last argument to qemu_opt_get_number() is *not* used here. That's
because a few lines above, you do
qemu_opt_set_number(opts, BLOCK_OPT_SIZE, img_size);
So, opts always has the BLOCK_OPT_SIZE option, and the default value
never applies.
Let's take a step back and figure out how the current code works before
we further review your patch.
bdrv_img_create() merges parameters img_size, base_filename, base_fmt
with options as follows:
1. Get format and protocol driver from parameters @fmt and @filename.
2. Combine the two drivers' option lists.
3. Copy the resulting option list for no discernible reason.
4. If %BLOCK_OPT_SIZE is a valid option, set its value to parameter
@img_size.
5. For all KEY=VAL in parameter @options:
KEY must be a valid option (error out if not)
set its value to VAL
6. If parameter @base_filename isn't null:
%BLOCK_OPT_BACKING_FILE must be a valid option (error out if not)
set its value to @base_filename
7. If parameter @base_fmt isn't null:
%BLOCK_OPT_BACKING_FMT must be a valid option (error out if not)
set its value to @base_fmt
In short, @base_filename and @base_fmt take precedence over @options
(unless they're null, of course). @options takes precedence over
@img_size. What a mess :) Not your fault.
Now let's look at the code retrieving BLOCK_OPT_SIZE:
// The size for the image must always be specified, with one exception:
// If we are using a backing file, we can obtain the size from there
size = get_option_parameter(param, BLOCK_OPT_SIZE);
if (size && size->value.n == -1) {
if (backing_file && backing_file->value.s) {
// compute size, put it into buf
set_option_parameter(param, BLOCK_OPT_SIZE, buf);
} else {
// error out
}
If %BLOCK_OPT_SIZE is a valid option, and has the special value
(uint64_t)-1, then we must have a backing file (error out if not), and
we use its size instead. In other words, the special value means "use
the backing file's size".
The value can come either from @img_size or from @options.
Some callers indeed pass (uint64_t)-1 for @img_size. For instance,
qemu-img create passes it when run without the optional size argument.
Works.
qemu-img is the only caller passing @options. With -o size=-1, it
actually puts (uint64_t)-1 into @param (parse_option_size() sucks).
Weird.
The whole bloody mess should be cleaned up. But I'm not asking you to
do that now.
I do think, however, that you should stick to
qemu_opt_get_number(opts, BLOCK_OPT_SIZE, 0)
instead of your voodoo
qemu_opt_get_number(opts, BLOCK_OPT_SIZE, -1)
>>> uint64_t size;
>>> - char buf[32];
>>> int back_flags;
>>>
>>> /* backing files always opened read-only */
>>> @@ -4583,17 +4577,16 @@ void bdrv_img_create(const char *filename, const
>>> char *fmt,
>>>
>>> bs = bdrv_new("");
>>>
>>> - ret = bdrv_open(bs, backing_file->value.s, back_flags,
>>> backing_drv);
>>> + ret = bdrv_open(bs, backing_file, back_flags, backing_drv);
>>> if (ret < 0) {
>>> error_setg_errno(errp, -ret, "Could not open '%s'",
>>> - backing_file->value.s);
>>> + backing_file);
>>> goto out;
>>> }
>>> bdrv_get_geometry(bs, &size);
>>> size *= 512;
>>>
>>> - snprintf(buf, sizeof(buf), "%" PRId64, size);
>>> - set_option_parameter(param, BLOCK_OPT_SIZE, buf);
>>> + qemu_opt_set_number(opts, BLOCK_OPT_SIZE, size);
>>> } else {
>>> error_setg(errp, "Image creation needs a size parameter");
>>> goto out;
>>> @@ -4601,10 +4594,10 @@ void bdrv_img_create(const char *filename, const
>>> char *fmt,
>>> }
>>>
>>> printf("Formatting '%s', fmt=%s ", filename, fmt);
>>> - print_option_parameters(param);
>>> + qemu_opts_print(opts, NULL);
>>> puts("");
>>>
>>> - ret = bdrv_create(drv, filename, param);
>>> + ret = bdrv_create(drv, filename, opts);
>>> if (ret < 0) {
>>> if (ret == -ENOTSUP) {
>>> error_setg(errp,"Formatting or formatting option not
>>> supported for "
>>> @@ -4619,8 +4612,10 @@ void bdrv_img_create(const char *filename, const
>>> char *fmt,
>>> }
>>>
>>> out:
>>> - free_option_parameters(create_options);
>>> - free_option_parameters(param);
>>> + free_opts_list(create_options);
>>> + if (opts) {
>>> + qemu_opts_del(opts);
>>> + }
>>>
>>> if (bs) {
>>> bdrv_delete(bs);
>>> diff --git a/block/cow.c b/block/cow.c
>>> index a33ce95..5442c9c 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,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?
>>
>>> + image_sectors = qemu_opt_get_number(opts, BLOCK_OPT_SIZE, 0) / 512;
>>> + image_filename = qemu_opt_get(opts, BLOCK_OPT_BACKING_FILE);
>>> }
>>>
>>> - ret = bdrv_create_file(filename, options);
>>> + ret = bdrv_create_file(filename, opts);
>>> if (ret < 0) {
>>> return ret;
>>> }
>>> @@ -318,18 +314,22 @@ exit:
>>> return ret;
>>> }
>>>
>>> -static QEMUOptionParameter cow_create_options[] = {
>>> - {
>>> - .name = BLOCK_OPT_SIZE,
>>> - .type = OPT_SIZE,
>>> - .help = "Virtual disk size"
>>> - },
>>> - {
>>> - .name = BLOCK_OPT_BACKING_FILE,
>>> - .type = OPT_STRING,
>>> - .help = "File name of a base image"
>>> - },
>>> - { NULL }
>>> +static QemuOptsList cow_create_opts = {
>>> + .name = "cow-create-opts",
>>> + .head = QTAILQ_HEAD_INITIALIZER(cow_create_opts.head),
>>> + .desc = {
>>> + {
>>> + .name = BLOCK_OPT_SIZE,
>>> + .type = QEMU_OPT_NUMBER,
>>
>> QEMU_OPT_SIZE, or else we lose support for suffixes, don't we?
>>
> In qemu-img.c, strtosz_suffix convert suffixes to regular digitals.
That's correct for the optional size argument, but not for -o size=...
> Okay. I will change to QEMU_OPT_SIZE.
Please do, and test that both the size argument and -o size=... works
just like before, with and without suffixes.
>>> + .help = "Virtual disk size"
>>> + },
>>> + {
>>> + .name = BLOCK_OPT_BACKING_FILE,
>>> + .type = QEMU_OPT_STRING,
>>> + .help = "File name of a base image"
>>> + },
>>> + { /* end of list */ }
>>> + }
>>> };
>>>
>>> static BlockDriver bdrv_cow = {
>>> @@ -345,7 +345,7 @@ static BlockDriver bdrv_cow = {
>>> .bdrv_write = cow_co_write,
>>> .bdrv_co_is_allocated = cow_co_is_allocated,
>>>
>>> - .create_options = cow_create_options,
>>> + .bdrv_create_options = &cow_create_opts,
>>> };
>>>
>>> static void bdrv_cow_init(void)
>>> diff --git a/block/gluster.c b/block/gluster.c
>>> index 0f2c32a..a41c684 100644
>>> --- a/block/gluster.c
>>> +++ b/block/gluster.c
>>> @@ -335,8 +335,7 @@ out:
>>> return ret;
>>> }
>>>
>>> -static int qemu_gluster_create(const char *filename,
>>> - QEMUOptionParameter *options)
>>> +static int qemu_gluster_create(const char *filename, QemuOpts* opts)
>>
>> Space before the *, not after: QemuOpts *opts
>>
> Okay.
[...]
