Re: [Qemu-devel] [PATCH V16 5/7] Use QemuOpts support in block layer

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 13369 bytes --]

On 06/18/2013 03:31 AM, Dong Xu Wang wrote:
> This patch uses QemuOpts related functions in block layer, add
> a member bdrv_create_opts to BlockDriver struct, it returns
> a QemuOptsList pointer, which includes the image format's create
> options.
> 

> +++ b/block/cow.c

Concluding my review...

> @@ -315,21 +309,27 @@ static int cow_create(const char *filename, QEMUOptionParameter *options)
>  
>  exit:
>      bdrv_delete(cow_bs);
> +finish:
> +    g_free((/* !const */ char*)image_filename);

Yuck - the only reason you have to cast away const here is because patch
4/7 returned const in the first place.  It would be a lot easier to just
declare 'char *image_filename', after fixing qemu_opt_get_del to not
force const on something the caller must free.  And since doing that
means spinning a v17, I really would like to see this patch split into
manageable pieces (incremental conversion of one file at a time, rather
than everything in one blow).

> +++ b/block/gluster.c
> @@ -365,8 +365,7 @@ out:
>      return ret;
>  }
>  
> -static int qemu_gluster_create(const char *filename,
> -        QEMUOptionParameter *options)
> +static int qemu_gluster_create(const char *filename, QemuOpts *opts)

Kudos on fixing bad indentation in the process.indentation

> +++ b/block/iscsi.c
> @@ -1213,7 +1213,7 @@ static int iscsi_has_zero_init(BlockDriverState *bs)
>      return 0;
>  }
>  
> -static int iscsi_create(const char *filename, QEMUOptionParameter *options)
> +static int iscsi_create(const char *filename,  QemuOpts *opts)

Spacing is off.

> +++ b/block/qcow.c

> @@ -662,26 +662,21 @@ static int qcow_create(const char *filename, QEMUOptionParameter *options)
>      int ret;
>      BlockDriverState *qcow_bs;
>  
> -    /* Read out options */
> -    while (options && options->name) {
> -        if (!strcmp(options->name, BLOCK_OPT_SIZE)) {
> -            total_size = options->value.n / 512;
> -        } else if (!strcmp(options->name, BLOCK_OPT_BACKING_FILE)) {
> -            backing_file = options->value.s;
> -        } else if (!strcmp(options->name, BLOCK_OPT_ENCRYPT)) {
> -            flags |= options->value.n ? BLOCK_FLAG_ENCRYPT : 0;
> -        }
> -        options++;
> +    /* Read out opts */
> +    total_size = qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0) / 512;
> +    backing_file = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FILE);
> +    if (qemu_opt_get_bool_del(opts, BLOCK_OPT_ENCRYPT, 0)) {

s/0/false/ - you're passing a bool parameter

> @@ -752,6 +747,8 @@ static int qcow_create(const char *filename, QEMUOptionParameter *options)
>      ret = 0;
>  exit:
>      bdrv_delete(qcow_bs);
> +finish:
> +    g_free((/* !const */ char*)backing_file);

Again, this just feels like patch 4/7 declared the wrong signature.
(I'll quit pointing it out, but there are other instances)

> +++ b/block/qcow2.c

> @@ -1243,7 +1244,8 @@ static int qcow2_create2(const char *filename, int64_t total_size,
>          error_report(
>              "Cluster size must be a power of two between %d and %dk",
>              1 << MIN_CLUSTER_BITS, 1 << (MAX_CLUSTER_BITS - 10));
> -        return -EINVAL;
> +        ret =  -EINVAL;

Spacing looks odd.

> @@ -1375,61 +1379,67 @@ static int qcow2_create(const char *filename, QEMUOptionParameter *options)
>      size_t cluster_size = DEFAULT_CLUSTER_SIZE;
>      int prealloc = 0;
>      int version = 2;
> +    const char *buf;
> +    int ret = 0;
>  

> +    sectors = qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0) / 512;
> +    backing_file = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FILE);
> +    backing_fmt = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FMT);
> +    if (qemu_opt_get_bool_del(opts, BLOCK_OPT_ENCRYPT, 0)) {

s/0/false/ (I'll quit pointing it out, but probably other instances)

> @@ -1704,49 +1714,55 @@ static int qcow2_load_vmstate(BlockDriverState *bs, uint8_t *buf,
>  
>      .bdrv_invalidate_cache      = qcow2_invalidate_cache,
>  
> -    .create_options = qcow2_create_options,
>      .bdrv_check = qcow2_check,
> +
> +    .bdrv_create_opts     = &qcow2_create_opts,

In other files, you were aligning the '='.  Here, it looks especially
odd that you are using more than one ' ' but still didn't align things -
it looks more consistent if you either go for full alignment or use
exactly one ' ' everywhere (I don't care which, so long as the results
don't look odd).

> +++ b/block/qed.c
> @@ -555,12 +555,12 @@ static int qed_create(const char *filename, uint32_t cluster_size,
>  
>      ret = bdrv_create_file(filename, NULL);
>      if (ret < 0) {
> -        return ret;
> +        goto finish;
>      }
>  
>      ret = bdrv_file_open(&bs, filename, NULL, BDRV_O_RDWR | BDRV_O_CACHE_WB);
>      if (ret < 0) {
> -        return ret;
> +        goto finish;
>      }
>  
>      /* File must start empty and grow, check truncate is supported */
> @@ -600,55 +600,57 @@ static int qed_create(const char *filename, uint32_t cluster_size,
>  out:
>      g_free(l1_table);
>      bdrv_delete(bs);
> +finish:
> +    g_free((/* !const */ char*)backing_file);
> +    g_free((/* !const */ char*)backing_fmt);

Eww - you are freeing something in the client that was allocated in the
parent, but passed via 'const char *'.  That makes it hard to trace
allocation ownership.  I'd rather see the frees moved into the function
that gets the data allocated (and _this_ function keeps 'const char *'
arguments), rather than passing the buck to the helper function...

>      return ret;
>  }
>  
> -static int bdrv_qed_create(const char *filename, QEMUOptionParameter *options)
> +static int bdrv_qed_create(const char *filename, QemuOpts *opts)
>  {
>      uint64_t image_size = 0;
>      uint32_t cluster_size = QED_DEFAULT_CLUSTER_SIZE;
>      uint32_t table_size = QED_DEFAULT_TABLE_SIZE;
>      const char *backing_file = NULL;
>      const char *backing_fmt = NULL;

> +    image_size = qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0);
> +    backing_file = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FILE);
> +    backing_fmt = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FMT);

>  
>      return qed_create(filename, cluster_size, image_size, table_size,
>                        backing_file, backing_fmt);

...better would have been 'ret = qed_create(...);' with fallthrough...

> +
> +finish:
> +    g_free((/* !const */ char*)backing_file);
> +    g_free((/* !const */ char*)backing_fmt);
> +    return ret;

...and let finish do the cleanup on both success and error.

> +++ b/block/qed.h
> @@ -43,6 +43,7 @@
>   *
>   * All fields are little-endian on disk.
>   */
> +#define  QED_DEFAULT_CLUSTER_SIZE  65536
>  
>  enum {
>      QED_MAGIC = 'Q' | 'E' << 8 | 'D' << 16 | '\0' << 24,
> @@ -69,7 +70,6 @@ enum {
>       */
>      QED_MIN_CLUSTER_SIZE = 4 * 1024, /* in bytes */
>      QED_MAX_CLUSTER_SIZE = 64 * 1024 * 1024,
> -    QED_DEFAULT_CLUSTER_SIZE = 64 * 1024,

Why are you converting an enum into a define?  I personally find enums
slightly easier to debug while under gdb.  I don't think this hunk is
necessary.

> +++ b/block/raw-posix.c
> @@ -123,6 +123,19 @@
>  
>  #define MAX_BLOCKSIZE	4096
>  
> +static QemuOptsList file_proto_create_opts = {
> +    .name = "file-proto-create-opts",
> +    .head = QTAILQ_HEAD_INITIALIZER(file_proto_create_opts.head),
> +    .desc = {
> +        {
> +            .name = BLOCK_OPT_SIZE,
> +            .type = QEMU_OPT_SIZE,
> +            .help = "Virtual disk size"
> +        },
> +        { /* end of list */ }
> +    }
> +};
> +

In other files, you did the conversion...

> @@ -1179,15 +1187,6 @@ static coroutine_fn BlockDriverAIOCB *raw_aio_discard(BlockDriverState *bs,
>                         cb, opaque, QEMU_AIO_DISCARD);
>  }
>  
> -static QEMUOptionParameter raw_create_options[] = {
> -    {
> -        .name = BLOCK_OPT_SIZE,
> -        .type = OPT_SIZE,
> -        .help = "Virtual disk size"
> -    },
> -    { NULL }
> -};

...next to the place you were replacing.  Why the difference in this file?

> +++ b/block/rbd.c

> @@ -997,7 +995,7 @@ static BlockDriver bdrv_rbd = {
>      .bdrv_close         = qemu_rbd_close,
>      .bdrv_create        = qemu_rbd_create,
>      .bdrv_get_info      = qemu_rbd_getinfo,
> -    .create_options     = qemu_rbd_create_options,
> +    .bdrv_create_opts   = &rbd_create_opts,

Why the lost qemu_ prefix?

> +++ b/block/sheepdog.c

> @@ -2259,7 +2253,6 @@ static int sd_load_vmstate(BlockDriverState *bs, uint8_t *data,
>      return do_load_save_vmstate(s, data, pos, size, 1);
>  }
>  
> -
>  static coroutine_fn int sd_co_discard(BlockDriverState *bs, int64_t sector_num,

Spurious whitespace change.

> @@ -2364,7 +2361,7 @@ static BlockDriver bdrv_sheepdog = {
>      .bdrv_save_vmstate  = sd_save_vmstate,
>      .bdrv_load_vmstate  = sd_load_vmstate,
>  
> -    .create_options = sd_create_options,
> +    .bdrv_create_opts   = &sd_create_opts,
>  };
>  
>  static BlockDriver bdrv_sheepdog_tcp = {
> @@ -2391,7 +2388,7 @@ static BlockDriver bdrv_sheepdog_tcp = {
>      .bdrv_save_vmstate  = sd_save_vmstate,
>      .bdrv_load_vmstate  = sd_load_vmstate,
>  
> -    .create_options = sd_create_options,
> +    .bdrv_create_opts = &sd_create_opts,

Inconsistent alignment.

>  };
>  
>  static BlockDriver bdrv_sheepdog_unix = {
> @@ -2418,7 +2415,7 @@ static BlockDriver bdrv_sheepdog_unix = {
>      .bdrv_save_vmstate  = sd_save_vmstate,
>      .bdrv_load_vmstate  = sd_load_vmstate,
>  
> -    .create_options = sd_create_options,
> +    .bdrv_create_opts = &sd_create_opts,

and again.

> +++ b/block/vdi.c

> @@ -648,25 +648,18 @@ static int vdi_create(const char *filename, QEMUOptionParameter *options)
>      logout("\n");
>  
>      /* Read out options. */
> -    while (options && options->name) {
> -        if (!strcmp(options->name, BLOCK_OPT_SIZE)) {
> -            bytes = options->value.n;
> +    bytes = qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0);
>  #if defined(CONFIG_VDI_BLOCK_SIZE)
> -        } else if (!strcmp(options->name, BLOCK_OPT_CLUSTER_SIZE)) {
> -            if (options->value.n) {
> -                /* TODO: Additional checks (SECTOR_SIZE * 2^n, ...). */
> -                block_size = options->value.n;
> -            }
> +        /* TODO: Additional checks (SECTOR_SIZE * 2^n, ...). */
> +    block_size = qemu_opt_get_size_del(opts,

New comment alignment looks odd.

> +++ b/block/vpc.c
> @@ -827,7 +832,7 @@ static BlockDriver bdrv_vpc = {
>      .bdrv_read              = vpc_co_read,
>      .bdrv_write             = vpc_co_write,
>  
> -    .create_options = vpc_create_options,
> +    .bdrv_create_opts = &vpc_create_opts,

Alignment.

> +++ b/include/block/block.h
> @@ -115,9 +115,8 @@ BlockDriver *bdrv_find_protocol(const char *filename);
>  BlockDriver *bdrv_find_format(const char *format_name);
>  BlockDriver *bdrv_find_whitelisted_format(const char *format_name,
>                                            bool readonly);
> -int bdrv_create(BlockDriver *drv, const char* filename,
> -    QEMUOptionParameter *options);
> -int bdrv_create_file(const char* filename, QEMUOptionParameter *options);
> +int bdrv_create(BlockDriver *drv, const char* filename, QemuOpts *opts);
> +int bdrv_create_file(const char *filename, QemuOpts *opts);

I see why you did things all in one patch - you changed the signature,
so all the callbacks had to pick up the new signature.  But I still
think it will be easier to review if you created the new signature with
a new name, did conversion of one file at a time, then delete the old
signature and rename the new name back to the old name.

>  BlockDriverState *bdrv_new(const char *device_name);
>  void bdrv_make_anon(BlockDriverState *bs);
>  void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old);
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index ba52247..41311e2 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -95,7 +95,7 @@ struct BlockDriver {
>                        const uint8_t *buf, int nb_sectors);
>      void (*bdrv_close)(BlockDriverState *bs);
>      void (*bdrv_rebind)(BlockDriverState *bs);
> -    int (*bdrv_create)(const char *filename, QEMUOptionParameter *options);
> +    int (*bdrv_create)(const char *filename, QemuOpts *opts);

Likewise, by temporarily having two different callback names, where
clients provide one of the two callbacks during the transition over
multiple patches, makes it possible to split into reviewable portions.

> +++ b/qemu-img.c
> @@ -1531,8 +1526,10 @@ static int img_convert(int argc, char **argv)
>      }
>  out:
>      qemu_progress_end();
> -    free_option_parameters(create_options);
> -    free_option_parameters(param);
> +    qemu_opts_free(create_opts);
> +    if (opts) {
> +        qemu_opts_del(opts);

Should qemu_opts_del() be made more free-like, by having it be a no-op
if opts is NULL?  That would be an independent cleanup, though.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]