[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH RFC] remove QEMUOptionParameter
|
From: |
Luiz Capitulino |
|
Subject: |
Re: [Qemu-devel] [PATCH RFC] remove QEMUOptionParameter |
|
Date: |
Mon, 10 Sep 2012 14:38:21 -0300 |
On Fri, 07 Sep 2012 10:42:28 +0200
Markus Armbruster <address@hidden> wrote:
> Some overlap with what I'm working on, but since you have code to show,
> and I don't, I'll review yours as if mine didn't exist.
>
>
> Dong Xu Wang <address@hidden> writes:
>
> > QEMU now has QEMUOptionParameter and QemuOpts to parse parameters, so we can
> > remove one safely. This RFC removed QEMUOptionParameter and use QemuOpts.
> > But
> > the patch is not completed, I think it is better to send a RFC first. In the
> > RFC, I only allow qcow2 and raw file format and did not test it completly,
> > if
> > you have any concerns and suggestions about the main idea, please let me
> > know,
> > that wil be very grateful.
> >
> > Signed-off-by: Dong Xu Wang <address@hidden>
> > ---
> > block.c | 106 ++++++++-----
> > block.h | 8 +-
> > block/Makefile.objs | 9 +-
> > block/qcow2.c | 123 ++++----------
> > block/raw-posix.c | 45 +----
> > block/raw.c | 12 +--
> > block_int.h | 5 +-
> > qemu-config.c | 85 ++++++++++
> > qemu-img.c | 67 ++++----
> > qemu-option.c | 456
> > +++++++++++++++++++--------------------------------
> > qemu-option.h | 47 +-----
> > 11 files changed, 417 insertions(+), 546 deletions(-)
> >
> > diff --git a/block.c b/block.c
> > index 470bdcc..8e973ff 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -351,7 +351,7 @@ BlockDriver *bdrv_find_whitelisted_format(const char
> > *format_name)
> > typedef struct CreateCo {
> > BlockDriver *drv;
> > char *filename;
> > - QEMUOptionParameter *options;
> > + QemuOpts *options;
>
> Consider renaming to opts, because that's the commonly used name. You
> did it already elsewhere.
>
> > int ret;
> > } CreateCo;
> >
> > @@ -364,7 +364,7 @@ static void coroutine_fn bdrv_create_co_entry(void
> > *opaque)
> > }
> >
> > int bdrv_create(BlockDriver *drv, const char* filename,
> > - QEMUOptionParameter *options)
> > + QemuOpts *opts)
> > {
> > int ret;
> >
> > @@ -372,7 +372,7 @@ int bdrv_create(BlockDriver *drv, const char* filename,
> > CreateCo cco = {
> > .drv = drv,
> > .filename = g_strdup(filename),
> > - .options = options,
> > + .options = opts,
> > .ret = NOT_DONE,
> > };
> >
> > @@ -397,7 +397,7 @@ int bdrv_create(BlockDriver *drv, const char* filename,
> > return ret;
> > }
> >
> > -int bdrv_create_file(const char* filename, QEMUOptionParameter *options)
> > +int bdrv_create_file(const char *filename, QemuOpts *opts)
> > {
> > BlockDriver *drv;
> >
> > @@ -406,7 +406,7 @@ int bdrv_create_file(const char* filename,
> > QEMUOptionParameter *options)
> > return -ENOENT;
> > }
> >
> > - return bdrv_create(drv, filename, options);
> > + return bdrv_create(drv, filename, opts);
> > }
> >
> > /*
> > @@ -742,8 +742,9 @@ int bdrv_open(BlockDriverState *bs, const char
> > *filename, int flags,
> > int64_t total_size;
> > int is_protocol = 0;
> > BlockDriver *bdrv_qcow2;
> > - QEMUOptionParameter *options;
> > + QemuOpts *options;
> > char backing_filename[PATH_MAX];
> > + char buf_total_size[1024];
> >
> > /* if snapshot, we create a temporary backing file and open it
> > instead of opening 'filename' directly */
> > @@ -775,17 +776,19 @@ 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);
> > + options = qemu_opts_create(qemu_find_opts("qcow2_create_options"),
> > + NULL, 0, NULL);
>
> I'm afraid you named them "qcow2_create_opts" in qemu-config.c.
>
> >
> > - set_option_parameter_int(options, BLOCK_OPT_SIZE, total_size);
> > - set_option_parameter(options, BLOCK_OPT_BACKING_FILE,
> > backing_filename);
> > + snprintf(buf_total_size, sizeof(buf_total_size),
> > + "%" PRId64, total_size);
> > + qemu_opt_set(options, BLOCK_OPT_SIZE, buf_total_size);
>
> This is a bit awkward.
>
> We could have qemu_opt_set_number(), like qemu_opt_set_bool(). Except
> qemu_opt_set_bool() has issues. Luiz's fix is discussed here:
> http://lists.nongnu.org/archive/html/qemu-devel/2012-07/msg02716.html
>
> Luiz, do you plan to respin?
I'm not going to respin the whole series, but the patch you mention
and the following two seem worth it to have on master.
Dong, do you want me to respin or can you add them to this series?
>
> > + qemu_opt_set(options, BLOCK_OPT_BACKING_FILE, backing_filename);
> > if (drv) {
> > - set_option_parameter(options, BLOCK_OPT_BACKING_FMT,
> > + qemu_opt_set(options, BLOCK_OPT_BACKING_FMT,
> > drv->format_name);
> > }
> >
> > ret = bdrv_create(bdrv_qcow2, tmp_filename, options);
> > - free_option_parameters(options);
> > if (ret < 0) {
> > return ret;
> > }
>
> This means ownership of options now passes to bdrv_create(). Which
> doesn't free it either. Should it?
>
> > @@ -3901,12 +3904,16 @@ int 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)
> > {
> > - QEMUOptionParameter *param = NULL, *create_options = NULL;
> > - QEMUOptionParameter *backing_fmt, *backing_file, *size;
> > + QemuOpts *param = NULL;
> > + QemuOptsList *create_options = NULL;
> > + const char *backing_fmt, *backing_file, *size;
> > BlockDriverState *bs = NULL;
> > BlockDriver *drv, *proto_drv;
> > BlockDriver *backing_drv = NULL;
> > int ret = 0;
> > + char buf_img_size[1024];
> > + char create_buff[1024];
> > + char proto_buff[1024];
> >
> > /* Find driver and parse its options */
> > drv = bdrv_find_format(fmt);
> > @@ -3922,20 +3929,19 @@ int bdrv_img_create(const char *filename, const
> > char *fmt,
> > ret = -EINVAL;
> > goto out;
> > }
> > -
> > - create_options = append_option_parameters(create_options,
> > - drv->create_options);
> > - create_options = append_option_parameters(create_options,
> > - proto_drv->create_options);
> > -
> > + get_format_create_options(create_buff, sizeof(create_buff), drv);
> > + get_proto_create_options(proto_buff, sizeof(proto_buff), proto_drv);
>
> These two functions don't get options, they get a "group name" to pass
> to qemu_find_opts().
>
> That's the only use of the value. What about making the functions
> return the QemuOptsList instead?
>
> Or even a function that maps BlockDriverState * to QemuOptsList *.
>
> > + create_options = append_opts_list(qemu_find_opts(create_buff),
> > + qemu_find_opts(proto_buff));
> > /* Create parameter list with default values */
> > - param = parse_option_parameters("", create_options, param);
> > + param = qemu_opts_create(create_options, NULL, 0, NULL);
>
> Ignoring errors is fine, because qemu_opts_create() can't fail when id
> is null.
>
> Aside: this is a common pattern. Maybe we should have a
> qemu_opts_create_nofail(QemuOptsList *list) for the purpose.
>
> >
> > - set_option_parameter_int(param, BLOCK_OPT_SIZE, img_size);
> > + snprintf(buf_img_size, sizeof(buf_img_size), "%" PRId64, img_size);
> > + qemu_opt_set(param, BLOCK_OPT_SIZE, buf_img_size);
>
> Another possible user of qemu_opt_set_number().
>
> >
> > /* Parse -o options */
> > if (options) {
> > - param = parse_option_parameters(options, create_options, param);
> > + param = parse_opts_list(options, create_options, param);
> > if (param == NULL) {
> > error_report("Invalid options for file format '%s'.", fmt);
> > ret = -EINVAL;
> > @@ -3944,7 +3950,7 @@ int bdrv_img_create(const char *filename, const char
> > *fmt,
> > }
> >
> > if (base_filename) {
> > - if (set_option_parameter(param, BLOCK_OPT_BACKING_FILE,
> > + if (qemu_opt_set(param, BLOCK_OPT_BACKING_FILE,
> > base_filename)) {
> > error_report("Backing file not supported for file format '%s'",
> > fmt);
> > @@ -3954,7 +3960,7 @@ int 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(param, BLOCK_OPT_BACKING_FMT, base_fmt)) {
> > error_report("Backing file format not supported for file "
> > "format '%s'", fmt);
> > ret = -EINVAL;
> > @@ -3962,9 +3968,9 @@ int bdrv_img_create(const char *filename, const char
> > *fmt,
> > }
> > }
> >
> > - 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(param, BLOCK_OPT_BACKING_FILE);
> > + if (backing_file) {
> > + if (!strcmp(filename, backing_file)) {
> > error_report("Error: Trying to create an image with the "
> > "same filename as the backing file");
> > ret = -EINVAL;
> > @@ -3972,12 +3978,12 @@ int bdrv_img_create(const char *filename, const
> > char *fmt,
> > }
> > }
> >
> > - 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(param, BLOCK_OPT_BACKING_FMT);
> > + if (backing_fmt) {
> > + backing_drv = bdrv_find_format(backing_fmt);
> > if (!backing_drv) {
> > error_report("Unknown backing file format '%s'",
> > - backing_fmt->value.s);
> > + backing_fmt);
> > ret = -EINVAL;
> > goto out;
> > }
> > @@ -3985,9 +3991,9 @@ int bdrv_img_create(const char *filename, const char
> > *fmt,
> >
> > // 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) {
>
> Can you explain how this code works? I don't get it.
>
> > + size = qemu_opt_get(param, BLOCK_OPT_SIZE);
> > + if (size && !strcmp(size, "-1")) {
>
> What about qemu_opt_get_number()?
>
> Beware, QemuOpts support only *unsigned* numbers.
>
> > + if (backing_file) {
> > uint64_t size;
> > char buf[32];
> > int back_flags;
> > @@ -3998,16 +4004,16 @@ int 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_report("Could not open '%s'", backing_file->value.s);
> > + error_report("Could not open '%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(param, BLOCK_OPT_SIZE, buf);
>
> Another one for qemu_opt_set_number().
>
> > } else {
> > error_report("Image creation needs a size parameter");
> > ret = -EINVAL;
> > @@ -4016,7 +4022,7 @@ int bdrv_img_create(const char *filename, const char
> > *fmt,
> > }
> >
> > printf("Formatting '%s', fmt=%s ", filename, fmt);
> > - print_option_parameters(param);
> > + qemu_opts_print(param, NULL);
> > puts("");
> >
> > ret = bdrv_create(drv, filename, param);
> > @@ -4035,8 +4041,7 @@ int bdrv_img_create(const char *filename, const char
> > *fmt,
> > }
> >
> > out:
> > - free_option_parameters(create_options);
> > - free_option_parameters(param);
> > + free_opts_list(create_options);
>
> Why isn't param freed with qemu_opts_del()?
>
> > if (bs) {
> > bdrv_delete(bs);
> > @@ -4171,3 +4176,24 @@ void block_job_sleep_ns(BlockJob *job, QEMUClock
> > *clock, int64_t ns)
> > job->busy = true;
> > }
> > }
> > +
> > +void get_proto_create_options(char *proto_buff,
> > + int buf_len, BlockDriver *proto_drv)
> > +{
> > + if (strcmp(proto_drv->format_name, "host_device") ||
> > + strcmp(proto_drv->format_name, "host_floppy") ||
> > + strcmp(proto_drv->format_name, "host_cdrom")) {
> > + snprintf(proto_buff, buf_len,
> > + "%s", "file_proto_create_opts");
>
> This function knows that certain drivers share options. Shouldn't that
> knowledge be encapsulated in drivers?
>
> I suspect the proper solution is a BlockDriver method returning the
> create option list.
>
> > + } else {
> > + snprintf(proto_buff, buf_len,
> > + "%s""_proto_create_opts", proto_drv->format_name);
> > + }
> > +}
> > +
> > +void get_format_create_options(char *create_buff, int buf_len,
> > BlockDriver* drv)
> > +{
> > + snprintf(create_buff, buf_len,
> > + "%s""_create_opts", drv->format_name);
> > +}
> > +
> > diff --git a/block.h b/block.h
> > index 2e2be11..04ec173 100644
> > --- a/block.h
> > +++ b/block.h
> > @@ -119,8 +119,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);
> > int bdrv_create(BlockDriver *drv, const char* filename,
> > - QEMUOptionParameter *options);
> > -int bdrv_create_file(const char* filename, QEMUOptionParameter *options);
> > + QemuOpts *options);
> > +int bdrv_create_file(const char *filename, QemuOpts *options);
> > BlockDriverState *bdrv_new(const char *device_name);
> > void bdrv_make_anon(BlockDriverState *bs);
> > void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old);
> > @@ -405,4 +405,8 @@ typedef enum {
> > #define BLKDBG_EVENT(bs, evt) bdrv_debug_event(bs, evt)
> > void bdrv_debug_event(BlockDriverState *bs, BlkDebugEvent event);
> >
> > +void get_proto_create_options(char *proto_buff,
> > + int buf_len, BlockDriver *proto_drv);
> > +void get_format_create_options(char *create_buff,
> > + int buf_len, BlockDriver *drv);
> > #endif
> > diff --git a/block/Makefile.objs b/block/Makefile.objs
> > index b5754d3..19de020 100644
> > --- a/block/Makefile.objs
> > +++ b/block/Makefile.objs
> > @@ -1,8 +1,9 @@
> > -block-obj-y += raw.o cow.o qcow.o vdi.o vmdk.o cloop.o dmg.o bochs.o vpc.o
> > vvfat.o
> > +#block-obj-y += raw.o cow.o qcow.o vdi.o vmdk.o cloop.o dmg.o bochs.o
> > vpc.o vvfat.o
> > block-obj-y += qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snapshot.o
> > qcow2-cache.o
> > -block-obj-y += qed.o qed-gencb.o qed-l2-cache.o qed-table.o qed-cluster.o
> > -block-obj-y += qed-check.o
> > -block-obj-y += parallels.o nbd.o blkdebug.o sheepdog.o blkverify.o
> > +#block-obj-y += qed.o qed-gencb.o qed-l2-cache.o qed-table.o qed-cluster.o
> > +#block-obj-y += qed-check.o
> > +#block-obj-y += parallels.o nbd.o blkdebug.o sheepdog.o blkverify.o
> > +block-obj-y += raw.o
> > block-obj-y += stream.o
> > block-obj-$(CONFIG_WIN32) += raw-win32.o
> > block-obj-$(CONFIG_POSIX) += raw-posix.o
>
> Lots of stuff cut to simplify exploring, I guess.
>
> > diff --git a/block/qcow2.c b/block/qcow2.c
> > index 8f183f1..c0f3764 100644
> > --- a/block/qcow2.c
> > +++ b/block/qcow2.c
> > @@ -1170,7 +1170,7 @@ static int preallocate(BlockDriverState *bs)
> > static int qcow2_create2(const char *filename, int64_t total_size,
> > const char *backing_file, const char
> > *backing_format,
> > int flags, size_t cluster_size, int prealloc,
> > - QEMUOptionParameter *options, int version)
> > + QemuOpts *options, int version)
> > {
> > /* Calculate cluster_bits */
> > int cluster_bits;
> > @@ -1201,6 +1201,7 @@ static int qcow2_create2(const char *filename,
> > int64_t total_size,
> > uint8_t* refcount_table;
> > int ret;
> >
> > + qemu_opt_set(options, BLOCK_OPT_SIZE, "0");
>
> Why do you need this?
>
> > ret = bdrv_create_file(filename, options);
> > if (ret < 0) {
> > return ret;
> > @@ -1304,7 +1305,7 @@ out:
> > return ret;
> > }
> >
> > -static int qcow2_create(const char *filename, QEMUOptionParameter *options)
> > +static int qcow2_create(const char *filename, QemuOpts *param)
>
> Rename to opts instead of param, please.
>
> > {
> > const char *backing_file = NULL;
> > const char *backing_fmt = NULL;
> > @@ -1313,45 +1314,41 @@ 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;
> >
> > /* Read out options */
> > - while (options && options->name) {
> > - if (!strcmp(options->name, BLOCK_OPT_SIZE)) {
> > - sectors = 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_BACKING_FMT)) {
> > - backing_fmt = options->value.s;
> > - } else if (!strcmp(options->name, BLOCK_OPT_ENCRYPT)) {
> > - flags |= options->value.n ? BLOCK_FLAG_ENCRYPT : 0;
> > - } else if (!strcmp(options->name, BLOCK_OPT_CLUSTER_SIZE)) {
> > - if (options->value.n) {
> > - cluster_size = options->value.n;
> > - }
> > - } else if (!strcmp(options->name, BLOCK_OPT_PREALLOC)) {
> > - if (!options->value.s || !strcmp(options->value.s, "off")) {
> > - prealloc = 0;
> > - } else if (!strcmp(options->value.s, "metadata")) {
> > - prealloc = 1;
> > - } else {
> > - fprintf(stderr, "Invalid preallocation mode: '%s'\n",
> > - options->value.s);
> > - return -EINVAL;
> > - }
> > - } else if (!strcmp(options->name, BLOCK_OPT_COMPAT_LEVEL)) {
> > - if (!options->value.s || !strcmp(options->value.s, "0.10")) {
> > - version = 2;
> > - } else if (!strcmp(options->value.s, "1.1")) {
> > - version = 3;
> > - } else {
> > - fprintf(stderr, "Invalid compatibility level: '%s'\n",
> > - options->value.s);
> > - return -EINVAL;
> > - }
> > - } else if (!strcmp(options->name, BLOCK_OPT_LAZY_REFCOUNTS)) {
> > - flags |= options->value.n ? BLOCK_FLAG_LAZY_REFCOUNTS : 0;
> > - }
> > - options++;
> > + sectors = qemu_opt_get_size(param, BLOCK_OPT_SIZE, 0) / 512;
> > + backing_file = qemu_opt_get(param, BLOCK_OPT_BACKING_FILE);
> > + backing_fmt = qemu_opt_get(param, BLOCK_OPT_BACKING_FMT);
> > + if (qemu_opt_get_bool(param, BLOCK_OPT_ENCRYPT, 0)) {
> > + flags |= BLOCK_FLAG_ENCRYPT;
> > + }
> > + cluster_size =
> > + qemu_opt_get_size(param, BLOCK_OPT_CLUSTER_SIZE,
> > DEFAULT_CLUSTER_SIZE);
> > + buf = qemu_opt_get(param, BLOCK_OPT_PREALLOC);
> > + if (!buf || !strcmp(buf, "off")) {
> > + prealloc = 0;
> > + } else if (!strcmp(buf, "metadata")) {
> > + prealloc = 1;
> > + } else {
> > + fprintf(stderr, "Invalid preallocation mode: '%s'\n",
> > + buf);
> > + return -EINVAL;
> > + }
> > +
> > + buf = qemu_opt_get(param, BLOCK_OPT_COMPAT_LEVEL);
> > + if (!buf || !strcmp(buf, "0.10")) {
> > + version = 2;
> > + } else if (!strcmp(buf, "1.1")) {
> > + version = 3;
> > + } else {
> > + fprintf(stderr, "Invalid compatibility level: '%s'\n",
> > + buf);
> > + return -EINVAL;
> > + }
> > +
> > + if (qemu_opt_get_bool(param, BLOCK_OPT_LAZY_REFCOUNTS, 0)) {
> > + flags |= BLOCK_FLAG_LAZY_REFCOUNTS;
> > }
> >
> > if (backing_file && prealloc) {
> > @@ -1367,7 +1364,7 @@ static int qcow2_create(const char *filename,
> > QEMUOptionParameter *options)
> > }
> >
> > return qcow2_create2(filename, sectors, backing_file, backing_fmt,
> > flags,
> > - cluster_size, prealloc, options, version);
> > + cluster_size, prealloc, param, version);
> > }
> >
> > static int qcow2_make_empty(BlockDriverState *bs)
> > @@ -1628,51 +1625,6 @@ static int qcow2_load_vmstate(BlockDriverState *bs,
> > uint8_t *buf,
> > return ret;
> > }
> >
> > -static QEMUOptionParameter qcow2_create_options[] = {
> > - {
> > - .name = BLOCK_OPT_SIZE,
> > - .type = OPT_SIZE,
> > - .help = "Virtual disk size"
> > - },
> > - {
> > - .name = BLOCK_OPT_COMPAT_LEVEL,
> > - .type = OPT_STRING,
> > - .help = "Compatibility level (0.10 or 1.1)"
> > - },
> > - {
> > - .name = BLOCK_OPT_BACKING_FILE,
> > - .type = OPT_STRING,
> > - .help = "File name of a base image"
> > - },
> > - {
> > - .name = BLOCK_OPT_BACKING_FMT,
> > - .type = OPT_STRING,
> > - .help = "Image format of the base image"
> > - },
> > - {
> > - .name = BLOCK_OPT_ENCRYPT,
> > - .type = OPT_FLAG,
> > - .help = "Encrypt the image"
> > - },
> > - {
> > - .name = BLOCK_OPT_CLUSTER_SIZE,
> > - .type = OPT_SIZE,
> > - .help = "qcow2 cluster size",
> > - .value = { .n = DEFAULT_CLUSTER_SIZE },
> > - },
> > - {
> > - .name = BLOCK_OPT_PREALLOC,
> > - .type = OPT_STRING,
> > - .help = "Preallocation mode (allowed values: off, metadata)"
> > - },
> > - {
> > - .name = BLOCK_OPT_LAZY_REFCOUNTS,
> > - .type = OPT_FLAG,
> > - .help = "Postpone refcount updates",
> > - },
> > - { NULL }
> > -};
> > -
>
> Replacement moves to qemu-config.c. Not sure that's an improvement, but
> it's how QemuOpts generally work. For an exception, see QemuOptsList
> use in blkdebug.c.
>
> > static BlockDriver bdrv_qcow2 = {
> > .format_name = "qcow2",
> > .instance_size = sizeof(BDRVQcowState),
> > @@ -1707,7 +1659,6 @@ static BlockDriver bdrv_qcow2 = {
> >
> > .bdrv_invalidate_cache = qcow2_invalidate_cache,
> >
> > - .create_options = qcow2_create_options,
> > .bdrv_check = qcow2_check,
> > };
> >
> > diff --git a/block/raw-posix.c b/block/raw-posix.c
> > index 6be20b1..bcce7ed 100644
> > --- a/block/raw-posix.c
> > +++ b/block/raw-posix.c
> > @@ -558,19 +558,13 @@ static int64_t
> > raw_get_allocated_file_size(BlockDriverState *bs)
> > return (int64_t)st.st_blocks * 512;
> > }
> >
> > -static int raw_create(const char *filename, QEMUOptionParameter *options)
> > +static int raw_create(const char *filename, QemuOpts *opts)
> > {
> > int fd;
> > int result = 0;
> > int64_t total_size = 0;
> >
> > - /* Read out options */
> > - while (options && options->name) {
> > - if (!strcmp(options->name, BLOCK_OPT_SIZE)) {
> > - total_size = options->value.n / BDRV_SECTOR_SIZE;
> > - }
> > - options++;
> > - }
> > + total_size = qemu_opt_get_size(opts, BLOCK_OPT_SIZE, 0) /
> > BDRV_SECTOR_SIZE;
> >
> > fd = qemu_open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY,
> > 0644);
> > @@ -720,15 +714,6 @@ static coroutine_fn int
> > raw_co_discard(BlockDriverState *bs,
> > return 0;
> > }
> >
> > -static QEMUOptionParameter raw_create_options[] = {
> > - {
> > - .name = BLOCK_OPT_SIZE,
> > - .type = OPT_SIZE,
> > - .help = "Virtual disk size"
> > - },
> > - { NULL }
> > -};
> > -
> > static BlockDriver bdrv_file = {
> > .format_name = "file",
> > .protocol_name = "file",
> > @@ -748,8 +733,6 @@ static BlockDriver bdrv_file = {
> > .bdrv_getlength = raw_getlength,
> > .bdrv_get_allocated_file_size
> > = raw_get_allocated_file_size,
> > -
> > - .create_options = raw_create_options,
> > };
> >
> > /***********************************************/
> > @@ -962,20 +945,14 @@ static int fd_open(BlockDriverState *bs)
> >
> > #endif /* !linux && !FreeBSD */
> >
> > -static int hdev_create(const char *filename, QEMUOptionParameter *options)
> > +static int hdev_create(const char *filename, QemuOpts *opts)
> > {
> > int fd;
> > int ret = 0;
> > struct stat stat_buf;
> > int64_t total_size = 0;
> >
> > - /* Read out options */
> > - while (options && options->name) {
> > - if (!strcmp(options->name, "size")) {
> > - total_size = options->value.n / BDRV_SECTOR_SIZE;
> > - }
> > - options++;
> > - }
> > + total_size = qemu_opt_get_size(opts, BLOCK_OPT_SIZE, 0) /
> > BDRV_SECTOR_SIZE;
> >
> > fd = qemu_open(filename, O_WRONLY | O_BINARY);
> > if (fd < 0)
> > @@ -999,21 +976,20 @@ static int hdev_has_zero_init(BlockDriverState *bs)
> >
> > static BlockDriver bdrv_host_device = {
> > .format_name = "host_device",
> > - .protocol_name = "host_device",
> > + .protocol_name = "host_device",
> > .instance_size = sizeof(BDRVRawState),
> > .bdrv_probe_device = hdev_probe_device,
> > .bdrv_file_open = hdev_open,
> > .bdrv_close = raw_close,
> > .bdrv_create = hdev_create,
> > - .create_options = raw_create_options,
> > .bdrv_has_zero_init = hdev_has_zero_init,
> >
> > - .bdrv_aio_readv = raw_aio_readv,
> > - .bdrv_aio_writev = raw_aio_writev,
> > - .bdrv_aio_flush = raw_aio_flush,
> > + .bdrv_aio_readv = raw_aio_readv,
> > + .bdrv_aio_writev = raw_aio_writev,
> > + .bdrv_aio_flush = raw_aio_flush,
> >
> > .bdrv_truncate = raw_truncate,
> > - .bdrv_getlength = raw_getlength,
> > + .bdrv_getlength = raw_getlength,
> > .bdrv_get_allocated_file_size
> > = raw_get_allocated_file_size,
> >
> > @@ -1126,7 +1102,6 @@ static BlockDriver bdrv_host_floppy = {
> > .bdrv_file_open = floppy_open,
> > .bdrv_close = raw_close,
> > .bdrv_create = hdev_create,
> > - .create_options = raw_create_options,
> > .bdrv_has_zero_init = hdev_has_zero_init,
> >
> > .bdrv_aio_readv = raw_aio_readv,
> > @@ -1225,7 +1200,6 @@ static BlockDriver bdrv_host_cdrom = {
> > .bdrv_file_open = cdrom_open,
> > .bdrv_close = raw_close,
> > .bdrv_create = hdev_create,
> > - .create_options = raw_create_options,
> > .bdrv_has_zero_init = hdev_has_zero_init,
> >
> > .bdrv_aio_readv = raw_aio_readv,
> > @@ -1344,7 +1318,6 @@ static BlockDriver bdrv_host_cdrom = {
> > .bdrv_file_open = cdrom_open,
> > .bdrv_close = raw_close,
> > .bdrv_create = hdev_create,
> > - .create_options = raw_create_options,
> > .bdrv_has_zero_init = hdev_has_zero_init,
> >
> > .bdrv_aio_readv = raw_aio_readv,
> > diff --git a/block/raw.c b/block/raw.c
> > index ff34ea4..a6a6758 100644
> > --- a/block/raw.c
> > +++ b/block/raw.c
> > @@ -87,20 +87,11 @@ static BlockDriverAIOCB *raw_aio_ioctl(BlockDriverState
> > *bs,
> > return bdrv_aio_ioctl(bs->file, req, buf, cb, opaque);
> > }
> >
> > -static int raw_create(const char *filename, QEMUOptionParameter *options)
> > +static int raw_create(const char *filename, QemuOpts *options)
> > {
> > return bdrv_create_file(filename, options);
> > }
> >
> > -static QEMUOptionParameter raw_create_options[] = {
> > - {
> > - .name = BLOCK_OPT_SIZE,
> > - .type = OPT_SIZE,
> > - .help = "Virtual disk size"
> > - },
> > - { NULL }
> > -};
> > -
> > static int raw_has_zero_init(BlockDriverState *bs)
> > {
> > return bdrv_has_zero_init(bs->file);
> > @@ -133,7 +124,6 @@ static BlockDriver bdrv_raw = {
> > .bdrv_aio_ioctl = raw_aio_ioctl,
> >
> > .bdrv_create = raw_create,
> > - .create_options = raw_create_options,
> > .bdrv_has_zero_init = raw_has_zero_init,
> > };
> >
> > diff --git a/block_int.h b/block_int.h
> > index 4452f6f..5eea367 100644
> > --- a/block_int.h
> > +++ b/block_int.h
> > @@ -147,7 +147,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 *options);
> > int (*bdrv_set_key)(BlockDriverState *bs, const char *key);
> > int (*bdrv_make_empty)(BlockDriverState *bs);
> > /* aio */
> > @@ -236,9 +236,6 @@ struct BlockDriver {
> > unsigned long int req, void *buf,
> > BlockDriverCompletionFunc *cb, void *opaque);
> >
> > - /* List of options for creating images, terminated by name == NULL */
> > - QEMUOptionParameter *create_options;
> > -
> >
> > /*
> > * Returns 0 for completed check, -errno for internal errors.
> > diff --git a/qemu-config.c b/qemu-config.c
> > index c05ffbc..0376c92 100644
> > --- a/qemu-config.c
> > +++ b/qemu-config.c
> > @@ -5,6 +5,87 @@
> > #include "hw/qdev.h"
> > #include "error.h"
> >
> > +#include "block_int.h"
> > +#define QCOW2_DEFAULT_CLUSTER_SIZE 65536
> > +
> > +static QemuOptsList file_proto_create_opts = {
> > + .name = "file_proto_create_opts",
>
> Please use '-' instead of '_' in name strings. More of the same below.
>
> > + .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 */ }
> > + }
> > +};
> > +
> > +/************************************************************/
> > +static QemuOptsList raw_create_opts = {
> > + .name = "raw_create_opts",
> > + .head = QTAILQ_HEAD_INITIALIZER(raw_create_opts.head),
> > + .desc = {
> > + {
> > + .name = BLOCK_OPT_SIZE,
> > + .type = QEMU_OPT_SIZE,
> > + .help = "Virtual disk size"
> > + },
> > + { /* end of list */ }
> > + }
> > +};
> > +
> > +static QemuOptsList qcow2_create_opts = {
> > + .name = "qcow2_create_opts",
> > + .head = QTAILQ_HEAD_INITIALIZER(qcow2_create_opts.head),
> > + .desc = {
> > + {
> > + .name = BLOCK_OPT_SIZE,
> > + .type = QEMU_OPT_SIZE,
> > + .help = "Virtual disk size"
> > + },
> > + {
> > + .name = BLOCK_OPT_COMPAT_LEVEL,
> > + .type = QEMU_OPT_STRING,
> > + .help = "Compatibility level (0.10 or 1.1)"
> > + },
> > + {
> > + .name = BLOCK_OPT_BACKING_FILE,
> > + .type = QEMU_OPT_STRING,
> > + .help = "File name of a base image"
> > + },
> > + {
> > + .name = BLOCK_OPT_BACKING_FMT,
> > + .type = QEMU_OPT_STRING,
> > + .help = "Image format of the base image"
> > + },
> > + {
> > + .name = BLOCK_OPT_ENCRYPT,
> > + .type = QEMU_OPT_BOOL,
> > + .help = "Encrypt the image"
> > + },
> > + {
> > + .name = BLOCK_OPT_CLUSTER_SIZE,
> > + .type = QEMU_OPT_SIZE,
> > + .help = "qcow2 cluster size",
> > + .def_value = QCOW2_DEFAULT_CLUSTER_SIZE
> > + },
> > + {
> > + .name = BLOCK_OPT_PREALLOC,
> > + .type = QEMU_OPT_STRING,
> > + .help = "Preallocation mode (allowed values: off, metadata)"
> > + },
> > + {
> > + .name = BLOCK_OPT_LAZY_REFCOUNTS,
> > + .type = QEMU_OPT_BOOL,
> > + .help = "Postpone refcount updates",
> > + },
> > + { /* end of list */ }
> > + }
> > +};
> > +
> > +/*******************************************************************/
> > +
> > static QemuOptsList qemu_drive_opts = {
> > .name = "drive",
> > .head = QTAILQ_HEAD_INITIALIZER(qemu_drive_opts.head),
> > @@ -659,6 +740,10 @@ static QemuOptsList *vm_config_groups[32] = {
> > &qemu_boot_opts,
> > &qemu_iscsi_opts,
> > &qemu_sandbox_opts,
> > +
> > + &file_proto_create_opts,
> > + &raw_create_opts,
> > + &qcow2_create_opts,
> > NULL,
> > };
> >
> > diff --git a/qemu-img.c b/qemu-img.c
> > index b41e670..a442b5e 100644
> > --- a/qemu-img.c
> > +++ b/qemu-img.c
> > @@ -195,7 +195,9 @@ static int read_password(char *buf, int buf_size)
> > static int print_block_option_help(const char *filename, const char *fmt)
> > {
> > BlockDriver *drv, *proto_drv;
> > - QEMUOptionParameter *create_options = NULL;
> > + QemuOptsList *create_options = NULL;
> > + char create_buff[1024];
> > + char proto_buff[1024];
> >
> > /* Find driver and parse its options */
> > drv = bdrv_find_format(fmt);
> > @@ -209,13 +211,12 @@ static int print_block_option_help(const char
> > *filename, const char *fmt)
> > error_report("Unknown protocol '%s'", filename);
> > return 1;
> > }
> > -
> > - create_options = append_option_parameters(create_options,
> > - drv->create_options);
> > - create_options = append_option_parameters(create_options,
> > - proto_drv->create_options);
> > - print_option_help(create_options);
> > - free_option_parameters(create_options);
> > + get_format_create_options(create_buff, sizeof(create_buff), drv);
> > + get_proto_create_options(proto_buff, sizeof(proto_buff), proto_drv);
> > + create_options = append_opts_list(qemu_find_opts(create_buff),
> > + qemu_find_opts(proto_buff));
> > + print_opts_list(create_options);
> > + free_opts_list(create_options);
> > return 0;
> > }
> >
> > @@ -265,19 +266,19 @@ fail:
> > return NULL;
> > }
> >
> > -static int add_old_style_options(const char *fmt, QEMUOptionParameter
> > *list,
> > +static int add_old_style_options(const char *fmt, QemuOpts *list,
> > const char *base_filename,
> > const char *base_fmt)
> > {
> > if (base_filename) {
> > - if (set_option_parameter(list, BLOCK_OPT_BACKING_FILE,
> > base_filename)) {
> > + if (qemu_opt_set(list, BLOCK_OPT_BACKING_FILE, base_filename)) {
> > error_report("Backing file not supported for file format '%s'",
> > fmt);
> > return -1;
> > }
> > }
> > if (base_fmt) {
> > - if (set_option_parameter(list, BLOCK_OPT_BACKING_FMT, base_fmt)) {
> > + if (qemu_opt_set(list, BLOCK_OPT_BACKING_FMT, base_fmt)) {
> > error_report("Backing file format not supported for file "
> > "format '%s'", fmt);
> > return -1;
> > @@ -664,12 +665,15 @@ static int img_convert(int argc, char **argv)
> > uint8_t * buf = NULL;
> > const uint8_t *buf1;
> > BlockDriverInfo bdi;
> > - QEMUOptionParameter *param = NULL, *create_options = NULL;
> > - QEMUOptionParameter *out_baseimg_param;
> > + QemuOpts *param = NULL;
> > + QemuOptsList *create_options = NULL;
> > + const char *out_baseimg_param;
> > char *options = NULL;
> > const char *snapshot_name = NULL;
> > float local_progress;
> > int min_sparse = 8; /* Need at least 4k of zeros for sparse detection
> > */
> > + char buf_size[1024];
> > + char create_buff[1024], proto_buff[1024];
> >
> > fmt = NULL;
> > out_fmt = "raw";
> > @@ -800,40 +804,40 @@ static int img_convert(int argc, char **argv)
> > goto out;
> > }
> >
> > - create_options = append_option_parameters(create_options,
> > - drv->create_options);
> > - create_options = append_option_parameters(create_options,
> > - proto_drv->create_options);
> > + get_format_create_options(create_buff, sizeof(create_buff), drv);
> > + get_proto_create_options(proto_buff, sizeof(proto_buff), proto_drv);
> > + create_options = append_opts_list(qemu_find_opts(create_buff),
> > + qemu_find_opts(proto_buff));
> >
> > if (options) {
> > - param = parse_option_parameters(options, create_options, param);
> > + param = parse_opts_list(options, create_options, param);
> > if (param == NULL) {
> > error_report("Invalid options for file format '%s'.", out_fmt);
> > ret = -1;
> > goto out;
> > }
> > } else {
> > - param = parse_option_parameters("", create_options, param);
> > + param = qemu_opts_create(create_options, NULL, 0, NULL);
> > }
> > -
> > - set_option_parameter_int(param, BLOCK_OPT_SIZE, total_sectors * 512);
> > + snprintf(buf_size, sizeof(buf_size), "%" PRId64, total_sectors * 512);
> > + qemu_opt_set(param, BLOCK_OPT_SIZE, buf_size);
> > ret = add_old_style_options(out_fmt, param, out_baseimg, NULL);
> > if (ret < 0) {
> > goto out;
> > }
> >
> > /* Get backing file name if -o backing_file was used */
> > - out_baseimg_param = get_option_parameter(param,
> > BLOCK_OPT_BACKING_FILE);
> > + out_baseimg_param = qemu_opt_get(param, BLOCK_OPT_BACKING_FILE);
> > if (out_baseimg_param) {
> > - out_baseimg = out_baseimg_param->value.s;
> > + out_baseimg = out_baseimg_param;
> > }
> >
> > /* Check if compression is supported */
> > if (compress) {
> > - QEMUOptionParameter *encryption =
> > - get_option_parameter(param, BLOCK_OPT_ENCRYPT);
> > - QEMUOptionParameter *preallocation =
> > - get_option_parameter(param, BLOCK_OPT_PREALLOC);
> > + const char *encryption =
> > + qemu_opt_get(param, BLOCK_OPT_ENCRYPT);
> > + const char *preallocation =
> > + qemu_opt_get(param, BLOCK_OPT_PREALLOC);
> >
> > if (!drv->bdrv_write_compressed) {
> > error_report("Compression not supported for this file format");
> > @@ -841,15 +845,15 @@ static int img_convert(int argc, char **argv)
> > goto out;
> > }
> >
> > - if (encryption && encryption->value.n) {
> > + if (encryption) {
> > error_report("Compression and encryption not supported at "
> > "the same time");
> > ret = -1;
> > goto out;
> > }
> >
> > - if (preallocation && preallocation->value.s
> > - && strcmp(preallocation->value.s, "off"))
> > + if (preallocation
> > + && strcmp(preallocation, "off"))
> > {
> > error_report("Compression and preallocation not supported at "
> > "the same time");
> > @@ -1063,8 +1067,7 @@ static int img_convert(int argc, char **argv)
> > }
> > out:
> > qemu_progress_end();
> > - free_option_parameters(create_options);
> > - free_option_parameters(param);
> > + free_opts_list(create_options);
> > qemu_vfree(buf);
> > if (out_bs) {
> > bdrv_delete(out_bs);
> > diff --git a/qemu-option.c b/qemu-option.c
> > index 27891e7..b83ca52 100644
> > --- a/qemu-option.c
> > +++ b/qemu-option.c
> > @@ -153,22 +153,6 @@ int check_params(char *buf, int buf_size,
> > return 0;
> > }
> >
> > -/*
> > - * Searches an option list for an option with the given name
> > - */
> > -QEMUOptionParameter *get_option_parameter(QEMUOptionParameter *list,
> > - const char *name)
> > -{
> > - while (list && list->name) {
> > - if (!strcmp(list->name, name)) {
> > - return list;
> > - }
> > - list++;
> > - }
> > -
> > - return NULL;
> > -}
> > -
> > static void parse_option_bool(const char *name, const char *value, bool
> > *ret,
> > Error **errp)
> > {
> > @@ -240,275 +224,6 @@ static void parse_option_size(const char *name, const
> > char *value,
> > }
> > }
> >
> > -/*
> > - * Sets the value of a parameter in a given option list. The parsing of the
> > - * value depends on the type of option:
> > - *
> > - * OPT_FLAG (uses value.n):
> > - * If no value is given, the flag is set to 1.
> > - * Otherwise the value must be "on" (set to 1) or "off" (set to 0)
> > - *
> > - * OPT_STRING (uses value.s):
> > - * value is strdup()ed and assigned as option value
> > - *
> > - * OPT_SIZE (uses value.n):
> > - * The value is converted to an integer. Suffixes for kilobytes etc.
> > are
> > - * allowed (powers of 1024).
> > - *
> > - * Returns 0 on succes, -1 in error cases
> > - */
> > -int set_option_parameter(QEMUOptionParameter *list, const char *name,
> > - const char *value)
> > -{
> > - bool flag;
> > - Error *local_err = NULL;
> > -
> > - // Find a matching parameter
> > - list = get_option_parameter(list, name);
> > - if (list == NULL) {
> > - fprintf(stderr, "Unknown option '%s'\n", name);
> > - return -1;
> > - }
> > -
> > - // Process parameter
> > - switch (list->type) {
> > - case OPT_FLAG:
> > - parse_option_bool(name, value, &flag, &local_err);
> > - if (!error_is_set(&local_err)) {
> > - list->value.n = flag;
> > - }
> > - break;
> > -
> > - case OPT_STRING:
> > - if (value != NULL) {
> > - list->value.s = g_strdup(value);
> > - } else {
> > - fprintf(stderr, "Option '%s' needs a parameter\n", name);
> > - return -1;
> > - }
> > - break;
> > -
> > - case OPT_SIZE:
> > - parse_option_size(name, value, &list->value.n, &local_err);
> > - break;
> > -
> > - default:
> > - fprintf(stderr, "Bug: Option '%s' has an unknown type\n", name);
> > - return -1;
> > - }
> > -
> > - if (error_is_set(&local_err)) {
> > - qerror_report_err(local_err);
> > - error_free(local_err);
> > - return -1;
> > - }
> > -
> > - return 0;
> > -}
> > -
> > -/*
> > - * Sets the given parameter to an integer instead of a string.
> > - * This function cannot be used to set string options.
> > - *
> > - * Returns 0 on success, -1 in error cases
> > - */
> > -int set_option_parameter_int(QEMUOptionParameter *list, const char *name,
> > - uint64_t value)
> > -{
> > - // Find a matching parameter
> > - list = get_option_parameter(list, name);
> > - if (list == NULL) {
> > - fprintf(stderr, "Unknown option '%s'\n", name);
> > - return -1;
> > - }
> > -
> > - // Process parameter
> > - switch (list->type) {
> > - case OPT_FLAG:
> > - case OPT_NUMBER:
> > - case OPT_SIZE:
> > - list->value.n = value;
> > - break;
> > -
> > - default:
> > - return -1;
> > - }
> > -
> > - return 0;
> > -}
> > -
> > -/*
> > - * Frees a option list. If it contains strings, the strings are freed as
> > well.
> > - */
> > -void free_option_parameters(QEMUOptionParameter *list)
> > -{
> > - QEMUOptionParameter *cur = list;
> > -
> > - while (cur && cur->name) {
> > - if (cur->type == OPT_STRING) {
> > - g_free(cur->value.s);
> > - }
> > - cur++;
> > - }
> > -
> > - g_free(list);
> > -}
> > -
> > -/*
> > - * Count valid options in list
> > - */
> > -static size_t count_option_parameters(QEMUOptionParameter *list)
> > -{
> > - size_t num_options = 0;
> > -
> > - while (list && list->name) {
> > - num_options++;
> > - list++;
> > - }
> > -
> > - return num_options;
> > -}
> > -
> > -/*
> > - * Append an option list (list) to an option list (dest).
> > - *
> > - * If dest is NULL, a new copy of list is created.
> > - *
> > - * Returns a pointer to the first element of dest (or the newly allocated
> > copy)
> > - */
> > -QEMUOptionParameter *append_option_parameters(QEMUOptionParameter *dest,
> > - QEMUOptionParameter *list)
> > -{
> > - size_t num_options, num_dest_options;
> > -
> > - num_options = count_option_parameters(dest);
> > - num_dest_options = num_options;
> > -
> > - num_options += count_option_parameters(list);
> > -
> > - dest = g_realloc(dest, (num_options + 1) *
> > sizeof(QEMUOptionParameter));
> > - dest[num_dest_options].name = NULL;
> > -
> > - while (list && list->name) {
> > - if (get_option_parameter(dest, list->name) == NULL) {
> > - dest[num_dest_options++] = *list;
> > - dest[num_dest_options].name = NULL;
> > - }
> > - list++;
> > - }
> > -
> > - return dest;
> > -}
> > -
> > -/*
> > - * Parses a parameter string (param) into an option list (dest).
> > - *
> > - * list is the template option list. If dest is NULL, a new copy of list is
> > - * created. If list is NULL, this function fails.
> > - *
> > - * A parameter string consists of one or more parameters, separated by
> > commas.
> > - * Each parameter consists of its name and possibly of a value. In the
> > latter
> > - * case, the value is delimited by an = character. To specify a value which
> > - * contains commas, double each comma so it won't be recognized as the end
> > of
> > - * the parameter.
> > - *
> > - * For more details of the parsing see above.
> > - *
> > - * Returns a pointer to the first element of dest (or the newly allocated
> > copy)
> > - * or NULL in error cases
> > - */
> > -QEMUOptionParameter *parse_option_parameters(const char *param,
> > - QEMUOptionParameter *list, QEMUOptionParameter *dest)
> > -{
> > - QEMUOptionParameter *allocated = NULL;
> > - char name[256];
> > - char value[256];
> > - char *param_delim, *value_delim;
> > - char next_delim;
> > -
> > - if (list == NULL) {
> > - return NULL;
> > - }
> > -
> > - if (dest == NULL) {
> > - dest = allocated = append_option_parameters(NULL, list);
> > - }
> > -
> > - while (*param) {
> > -
> > - // Find parameter name and value in the string
> > - param_delim = strchr(param, ',');
> > - value_delim = strchr(param, '=');
> > -
> > - if (value_delim && (value_delim < param_delim || !param_delim)) {
> > - next_delim = '=';
> > - } else {
> > - next_delim = ',';
> > - value_delim = NULL;
> > - }
> > -
> > - param = get_opt_name(name, sizeof(name), param, next_delim);
> > - if (value_delim) {
> > - param = get_opt_value(value, sizeof(value), param + 1);
> > - }
> > - if (*param != '\0') {
> > - param++;
> > - }
> > -
> > - // Set the parameter
> > - if (set_option_parameter(dest, name, value_delim ? value : NULL)) {
> > - goto fail;
> > - }
> > - }
> > -
> > - return dest;
> > -
> > -fail:
> > - // Only free the list if it was newly allocated
> > - free_option_parameters(allocated);
> > - return NULL;
> > -}
> > -
> > -/*
> > - * Prints all options of a list that have a value to stdout
> > - */
> > -void print_option_parameters(QEMUOptionParameter *list)
> > -{
> > - while (list && list->name) {
> > - switch (list->type) {
> > - case OPT_STRING:
> > - if (list->value.s != NULL) {
> > - printf("%s='%s' ", list->name, list->value.s);
> > - }
> > - break;
> > - case OPT_FLAG:
> > - printf("%s=%s ", list->name, list->value.n ? "on" : "off");
> > - break;
> > - case OPT_SIZE:
> > - case OPT_NUMBER:
> > - printf("%s=%" PRId64 " ", list->name, list->value.n);
> > - break;
> > - default:
> > - printf("%s=(unknown type) ", list->name);
> > - break;
> > - }
> > - list++;
> > - }
> > -}
> > -
> > -/*
> > - * Prints an overview of all available options
> > - */
> > -void print_option_help(QEMUOptionParameter *list)
> > -{
> > - printf("Supported options:\n");
> > - while (list && list->name) {
> > - printf("%-16s %s\n", list->name,
> > - list->help ? list->help : "No description available");
> > - list++;
> > - }
> > -}
> > -
> > /* ------------------------------------------------------------------ */
> >
> > static QemuOpt *qemu_opt_find(QemuOpts *opts, const char *name)
> > @@ -832,14 +547,36 @@ void qemu_opts_del(QemuOpts *opts)
> >
> > int qemu_opts_print(QemuOpts *opts, void *dummy)
> > {
> > - QemuOpt *opt;
> > + QemuOpt *opt = NULL;
> > + QemuOptDesc *desc = opts->list->desc;
> >
> > - fprintf(stderr, "%s: %s:", opts->list->name,
> > - opts->id ? opts->id : "<noid>");
> > - QTAILQ_FOREACH(opt, &opts->head, next) {
> > - fprintf(stderr, " %s=\"%s\"", opt->name, opt->str);
> > + while (desc && desc->name) {
> > + opt = qemu_opt_find(opts, desc->name);
> > + switch (desc->type) {
> > + case QEMU_OPT_STRING:
> > + if (opt != NULL) {
> > + printf("%s='%s' ", opt->name, opt->str);
> > + }
> > + break;
> > + case QEMU_OPT_BOOL:
> > + printf("%s=%s ", desc->name, (opt && opt->str) ? "on" : "off");
> > + break;
> > + case QEMU_OPT_NUMBER:
> > + case QEMU_OPT_SIZE:
> > + if (strcmp(desc->name, "cluster_size")) {
> > + printf("%s=%" PRId64 " ", desc->name,
> > + (opt && opt->value.uint) ? opt->value.uint : 0);
> > + } else {
> > + printf("%s=%" PRId64 " ", desc->name,
> > + (opt && opt->value.uint) ? opt->value.uint :
> > desc->def_value);
> > + }
> > + break;
> > + default:
> > + printf("%s=(unknown type) ", desc->name);
> > + break;
> > + }
> > + desc++;
> > }
> > - fprintf(stderr, "\n");
> > return 0;
> > }
> >
>
> Why do you need to change this function?
>
> > @@ -1110,3 +847,140 @@ int qemu_opts_foreach(QemuOptsList *list,
> > qemu_opts_loopfunc func, void *opaque,
> > loc_pop(&loc);
> > return rc;
> > }
> > +
> > +static size_t count_opts_list(QemuOptsList *list)
> > +{
> > + size_t i = 0;
> > +
> > + while (list && list->desc[i].name) {
> > + i++;
> > + }
> > +
> > + return i;
> > +}
> > +
> > +static bool opts_desc_find(QemuOptsList *list, const char *name)
> > +{
> > + const QemuOptDesc *desc = list->desc;
> > + int i;
> > +
> > + for (i = 0; desc[i].name != NULL; i++) {
> > + if (strcmp(desc[i].name, name) == 0) {
> > + return true;;
>
> Extra ;
>
> > + }
> > + }
> > + return false;
> > +}
>
> Duplicates the loop searching list->desc[] yet again. What about
> factoring out all the copies into a function? Separate cleanup patch
> preceding this one.
>
> > +
> > +/* merge two QemuOptsLists to one and return it. */
> > +QemuOptsList *append_opts_list(QemuOptsList *first,
> > + QemuOptsList *second)
> > +{
> > + size_t num_first_options, num_second_options;
> > + QemuOptsList *dest = NULL;
> > + int i = 0;
> > + int index = 0;
> > +
> > + num_first_options = count_opts_list(first);
> > + num_second_options = count_opts_list(second);
> > + if (num_first_options + num_second_options == 0) {
> > + return NULL;
> > + }
> > +
> > + dest = g_malloc0(sizeof(QemuOptsList)
> > + + (num_first_options + num_second_options) * sizeof(QemuOptDesc));
>
> Allocate space for both first and second.
>
> > +
> > + if (first) {
> > + memcpy(dest, first, sizeof(QemuOptsList));
> > + } else if (second) {
> > + memcpy(dest, second, sizeof(QemuOptsList));
> > + }
>
> Copy either first or second.
>
> If both are non-null, the space for second remains blank.
>
> Why copy at all? The loops below copy again, don't they?
>
> > +
> > + while (first && (first->desc[i].name)) {
> > + if (!opts_desc_find(dest, first->desc[i].name)) {
> > + dest->desc[index].name = strdup(first->desc[i].name);
> > + dest->desc[index].help = strdup(first->desc[i].help);
>
> g_strdup()
>
> Why do you need to copy name and help?
>
> > + dest->desc[index].type = first->desc[i].type;
> > + dest->desc[index].def_value = first->desc[i].def_value;
> > + dest->desc[++index].name = NULL;
>
> I'd terminate dest->desc[] after the loop, not in every iteration.
>
> > + }
> > + i++;
> > + }
> > + i = 0;
> > + while (second && (second->desc[i].name)) {
> > + if (!opts_desc_find(dest, second->desc[i].name)) {
> > + dest->desc[index].name = strdup(first->desc[i].name);
> > + dest->desc[index].help = strdup(first->desc[i].help);
> > + dest->desc[index].type = second->desc[i].type;
> > + dest->desc[index].def_value = second->desc[i].def_value;
> > + dest->desc[++i].name = NULL;
> > + }
> > + i++;
> > + }
>
> Please avoid duplicating the loop.
>
> What if first and second both contain the same parameter name?
>
> > + return dest;
> > +}
> > +
> > +void free_opts_list(QemuOptsList *list)
> > +{
> > + int i = 0;
> > +
> > + while (list && list->desc[i].name) {
> > + g_free((char *)list->desc[i].name);
> > + g_free((char *)list->desc[i].help);
> > + i++;
> > + }
> > +
> > + g_free(list);
> > +}
> > +
> > +void print_opts_list(QemuOptsList *list)
> > +{
> > + int i = 0;
> > + printf("Supported options:\n");
> > + while (list && list->desc[i].name) {
> > + printf("%-16s %s\n", list->desc[i].name,
> > + list->desc[i].help ? list->desc[i].help : "No description
> > available");
> > + i++;
> > + }
> > +}
> > +
> > +QemuOpts *parse_opts_list(const char *param,
> > + QemuOptsList *list, QemuOpts *dest)
> > +{
> > + char name[256];
> > + char value[256];
> > + char *param_delim, *value_delim;
> > + char next_delim;
> > +
> > + if (list == NULL) {
> > + return NULL;
> > + }
> > + while (*param) {
> > + param_delim = strchr(param, ',');
> > + value_delim = strchr(param, '=');
> > +
> > + if (value_delim && (value_delim < param_delim || !param_delim)) {
> > + next_delim = '=';
> > + } else {
> > + next_delim = ',';
> > + value_delim = NULL;
> > + }
> > +
> > + param = get_opt_name(name, sizeof(name), param, next_delim);
> > + if (value_delim) {
> > + param = get_opt_value(value, sizeof(value), param + 1);
> > + }
> > + if (*param != '\0') {
> > + param++;
> > + }
> > +
> > + if (qemu_opt_set(dest, name, value_delim ? value : NULL)) {
> > + goto fail;
> > + }
> > + }
> > +
> > + return dest;
> > +
> > +fail:
> > + return NULL;
> > +}
>
> Can you explain why the existing QemuOpts parsers won't do?
>
> > diff --git a/qemu-option.h b/qemu-option.h
> > index ca72986..75718fe 100644
> > --- a/qemu-option.h
> > +++ b/qemu-option.h
> > @@ -31,24 +31,6 @@
> > #include "error.h"
> > #include "qdict.h"
> >
> > -enum QEMUOptionParType {
> > - OPT_FLAG,
> > - OPT_NUMBER,
> > - OPT_SIZE,
> > - OPT_STRING,
> > -};
> > -
> > -typedef struct QEMUOptionParameter {
> > - const char *name;
> > - enum QEMUOptionParType type;
> > - union {
> > - uint64_t n;
> > - char* s;
> > - } value;
> > - const char *help;
> > -} QEMUOptionParameter;
> > -
> > -
> > const char *get_opt_name(char *buf, int buf_size, const char *p, char
> > delim);
> > const char *get_opt_value(char *buf, int buf_size, const char *p);
> > int get_next_param_value(char *buf, int buf_size,
> > @@ -58,27 +40,6 @@ int get_param_value(char *buf, int buf_size,
> > int check_params(char *buf, int buf_size,
> > const char * const *params, const char *str);
> >
> > -
> > -/*
> > - * The following functions take a parameter list as input. This is a
> > pointer to
> > - * the first element of a QEMUOptionParameter array which is terminated by
> > an
> > - * entry with entry->name == NULL.
> > - */
> > -
> > -QEMUOptionParameter *get_option_parameter(QEMUOptionParameter *list,
> > - const char *name);
> > -int set_option_parameter(QEMUOptionParameter *list, const char *name,
> > - const char *value);
> > -int set_option_parameter_int(QEMUOptionParameter *list, const char *name,
> > - uint64_t value);
> > -QEMUOptionParameter *append_option_parameters(QEMUOptionParameter *dest,
> > - QEMUOptionParameter *list);
> > -QEMUOptionParameter *parse_option_parameters(const char *param,
> > - QEMUOptionParameter *list, QEMUOptionParameter *dest);
> > -void free_option_parameters(QEMUOptionParameter *list);
> > -void print_option_parameters(QEMUOptionParameter *list);
> > -void print_option_help(QEMUOptionParameter *list);
> > -
> > /* ------------------------------------------------------------------ */
> >
> > typedef struct QemuOpt QemuOpt;
> > @@ -96,6 +57,7 @@ typedef struct QemuOptDesc {
> > const char *name;
> > enum QemuOptType type;
> > const char *help;
> > + uint64_t def_value;
>
> The only user I can see is qemu_opts_print(), which can't be right.
>
> > } QemuOptDesc;
> >
> > struct QemuOptsList {
> > @@ -152,5 +114,10 @@ typedef int (*qemu_opts_loopfunc)(QemuOpts *opts, void
> > *opaque);
> > int qemu_opts_print(QemuOpts *opts, void *dummy);
> > int qemu_opts_foreach(QemuOptsList *list, qemu_opts_loopfunc func, void
> > *opaque,
> > int abort_on_failure);
> > -
> > +QemuOptsList *append_opts_list(QemuOptsList *dest,
> > + QemuOptsList *list);
> > +void free_opts_list(QemuOptsList *list);
> > +void print_opts_list(QemuOptsList *list);
> > +QemuOpts *parse_opts_list(const char *param,
> > + QemuOptsList *list, QemuOpts *dest);
> > #endif
>