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:
>> 
>>> 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 <address@hidden>
>>> ---
>>> 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.

[...]