[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH RFC] remove QEMUOptionParameter
|
From: |
Dong Xu Wang |
|
Subject: |
Re: [Qemu-devel] [PATCH RFC] remove QEMUOptionParameter |
|
Date: |
Tue, 11 Sep 2012 09:57:12 +0800 |
On Tue, Sep 11, 2012 at 1:38 AM, Luiz Capitulino <address@hidden> wrote:
> 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?
I can add them to this series, thank you Luiz.
>
>>
>> > + 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
>>
>
>