[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH V16 5/7] Use QemuOpts support in block layer
|
From: |
Eric Blake |
|
Subject: |
Re: [Qemu-devel] [PATCH V16 5/7] Use QemuOpts support in block layer |
|
Date: |
Wed, 10 Jul 2013 13:47:49 -0600 |
|
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130625 Thunderbird/17.0.7 |
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
signature.asc
Description: OpenPGP digital signature