Re: [Qemu-devel] [PATCH v20 18/26] sheepdog.c: replace QEMUOptionParameter with QemuOpts

[Chunyan Liu]

2014-02-18 3:01 GMT+08:00 Eric Blake <address@hidden>:

On 02/11/2014 11:33 PM, Chunyan Liu wrote:
> sheepdog.c: replace QEMUOptionParameter with QemuOpts
>
> Signed-off-by: Dong Xu Wang <address@hidden>
> Signed-off-by: Chunyan Liu <address@hidden>
> ---
>  block/sheepdog.c |  101 +++++++++++++++++++++++++-----------------------------
>  1 files changed, 47 insertions(+), 54 deletions(-)
>

> +
> +    buf = NULL;
> +    buf = qemu_opt_get_del(opts, BLOCK_OPT_REDUNDANCY);

Dead NULL assignment.

> +    if (buf) {
> +        ret = parse_redundancy(s, buf);
> +        if (ret < 0) {
> +            goto out;
>          }

Do you need to store into ret here, or is it sufficient to use 'if
(parse_redundancy(s, buf) < 0) {'

It followed the logic in existing code. If function ret < 0, return
with that return value. Same logic in earlier codes of the function.
    if (strstr(filename, "://")) {
        ret = sd_parse_uri(s, filename, s->name, &snapid, tag);
    } else {
        ret = parse_vdiname(s, filename, s->name, &snapid, tag);
    }
    if (ret < 0) {
        goto out;
    }

 

> -    {
> -        .name = BLOCK_OPT_REDUNDANCY,
> -        .type = OPT_STRING,
> -        .help = "Redundancy of the image"
> -    },
> -    { NULL }
> +static QemuOptsList sd_create_opts = {
> +    .name = "sheepdog-create-opts",
> +    .head = QTAILQ_HEAD_INITIALIZER(sd_create_opts.head),
> +    .desc = {
> +        {

> +            .name = BLOCK_OPT_PREALLOC,
> +            .type = QEMU_OPT_STRING,
> +            .help = "Preallocation mode (allowed values: off, full)"
> +        },
> +        { /* end of list */ }

Missing redundancy in the new list.

> @@ -2538,7 +2533,7 @@ static BlockDriver bdrv_sheepdog = {
>      .bdrv_save_vmstate  = sd_save_vmstate,
>      .bdrv_load_vmstate  = sd_load_vmstate,
>
> -    .create_options = sd_create_options,
> +    .create_opts   = &sd_create_opts,

Another example of the inconsistent use of &.

>  };
>
>  static BlockDriver bdrv_sheepdog_tcp = {
> @@ -2548,7 +2543,7 @@ static BlockDriver bdrv_sheepdog_tcp = {
>      .bdrv_needs_filename = true,
>      .bdrv_file_open = sd_open,
>      .bdrv_close     = sd_close,
> -    .bdrv_create    = sd_create,
> +    .bdrv_create2   = sd_create,
>      .bdrv_has_zero_init = bdrv_has_zero_init_1,
>      .bdrv_getlength = sd_getlength,
>      .bdrv_get_allocated_file_size = sd_get_allocated_file_size,
> @@ -2568,7 +2563,6 @@ static BlockDriver bdrv_sheepdog_tcp = {
>      .bdrv_save_vmstate  = sd_save_vmstate,
>      .bdrv_load_vmstate  = sd_load_vmstate,
>
> -    .create_options = sd_create_options,
>  };

Why no .create_opts replacement?

--
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org