Paolo Bonzini <pbonz...@redhat.com> writes:
> qemu_opts_set is used to create default network backends and to
> parse sugar options -kernel, -initrd, -append, -bios and -dtb.
> Switch the former to qemu_opts_parse, so that qemu_opts_set
> is now only used on merge-lists QemuOptsList (for which it makes
> the most sense indeed)... except in the testcase, which is
> changed to use a merge-list QemuOptsList.
>
> With this change we can remove the id parameter. With the
> parameter always NULL, we know that qemu_opts_create cannot fail
> and can pass &error_abort to it.
>
> Signed-off-by: Paolo Bonzini <pbonz...@redhat.com>
> ---
> include/qemu/option.h | 3 +--
> softmmu/vl.c | 19 +++++++------------
> tests/test-qemu-opts.c | 24 +++++++++++++++++++++---
> util/qemu-option.c | 9 +++------
> 4 files changed, 32 insertions(+), 23 deletions(-)
>
> diff --git a/include/qemu/option.h b/include/qemu/option.h
> index ac69352e0e..f73e0dc7d9 100644
> --- a/include/qemu/option.h
> +++ b/include/qemu/option.h
> @@ -119,8 +119,7 @@ QemuOpts *qemu_opts_create(QemuOptsList *list, const char
> *id,
> int fail_if_exists, Error **errp);
> void qemu_opts_reset(QemuOptsList *list);
> void qemu_opts_loc_restore(QemuOpts *opts);
> -bool qemu_opts_set(QemuOptsList *list, const char *id,
> - const char *name, const char *value, Error **errp);
> +bool qemu_opts_set(QemuOptsList *list, const char *name, const char *value,
> Error **errp);
Long line. Please break before Error.
> const char *qemu_opts_id(QemuOpts *opts);
> void qemu_opts_set_id(QemuOpts *opts, char *id);
> void qemu_opts_del(QemuOpts *opts);
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index a71164494e..65607fe55e 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -3107,20 +3107,16 @@ void qemu_init(int argc, char **argv, char **envp)
> }
> break;
> case QEMU_OPTION_kernel:
> - qemu_opts_set(qemu_find_opts("machine"), NULL, "kernel",
> optarg,
> - &error_abort);
> + qemu_opts_set(qemu_find_opts("machine"), "kernel", optarg,
> &error_abort);
> break;
> case QEMU_OPTION_initrd:
> - qemu_opts_set(qemu_find_opts("machine"), NULL, "initrd",
> optarg,
> - &error_abort);
> + qemu_opts_set(qemu_find_opts("machine"), "initrd", optarg,
> &error_abort);
> break;
> case QEMU_OPTION_append:
> - qemu_opts_set(qemu_find_opts("machine"), NULL, "append",
> optarg,
> - &error_abort);
> + qemu_opts_set(qemu_find_opts("machine"), "append", optarg,
> &error_abort);
> break;
> case QEMU_OPTION_dtb:
> - qemu_opts_set(qemu_find_opts("machine"), NULL, "dtb", optarg,
> - &error_abort);
> + qemu_opts_set(qemu_find_opts("machine"), "dtb", optarg,
> &error_abort);
> break;
> case QEMU_OPTION_cdrom:
> drive_add(IF_DEFAULT, 2, optarg, CDROM_OPTS);
> @@ -3230,8 +3226,7 @@ void qemu_init(int argc, char **argv, char **envp)
> }
> break;
> case QEMU_OPTION_bios:
> - qemu_opts_set(qemu_find_opts("machine"), NULL, "firmware",
> optarg,
> - &error_abort);
> + qemu_opts_set(qemu_find_opts("machine"), "firmware", optarg,
> &error_abort);
> break;
> case QEMU_OPTION_singlestep:
> singlestep = 1;
Long lines. Please keep the line breaks.
> @@ -4258,9 +4253,9 @@ void qemu_init(int argc, char **argv, char **envp)
>
> if (default_net) {
> QemuOptsList *net = qemu_find_opts("net");
> - qemu_opts_set(net, NULL, "type", "nic", &error_abort);
> + qemu_opts_parse(net, "nic", true, &error_abort);
> #ifdef CONFIG_SLIRP
> - qemu_opts_set(net, NULL, "type", "user", &error_abort);
> + qemu_opts_parse(net, "user", true, &error_abort);
> #endif
> }
>
Looks safe to me, but I don't quite get why you switch to
qemu_opts_parse(). The commit message explains it is "so that
qemu_opts_set is now only used on merge-lists QemuOptsList (for which it
makes the most sense indeed)..." Is there anything wrong with using ot
on non-merge-lists QemuOptsList?
Am I missing something?
> diff --git a/tests/test-qemu-opts.c b/tests/test-qemu-opts.c
> index 297ffe79dd..322b32871b 100644
> --- a/tests/test-qemu-opts.c
> +++ b/tests/test-qemu-opts.c
> @@ -84,11 +84,25 @@ static QemuOptsList opts_list_03 = {
> },
> };
>
> +static QemuOptsList opts_list_04 = {
> + .name = "opts_list_04",
> + .head = QTAILQ_HEAD_INITIALIZER(opts_list_04.head),
> + .merge_lists = true,
> + .desc = {
> + {
> + .name = "str3",
> + .type = QEMU_OPT_STRING,
> + },
> + { /* end of list */ }
> + },
> +};
> +
> static void register_opts(void)
> {
> qemu_add_opts(&opts_list_01);
> qemu_add_opts(&opts_list_02);
> qemu_add_opts(&opts_list_03);
> + qemu_add_opts(&opts_list_04);
> }
>
> static void test_find_unknown_opts(void)
> @@ -402,17 +416,21 @@ static void test_qemu_opts_set(void)
> QemuOpts *opts;
> const char *opt;
>
> - list = qemu_find_opts("opts_list_01");
> + list = qemu_find_opts("opts_list_04");
> g_assert(list != NULL);
> g_assert(QTAILQ_EMPTY(&list->head));
> - g_assert_cmpstr(list->name, ==, "opts_list_01");
> + g_assert_cmpstr(list->name, ==, "opts_list_04");
>
> /* should not find anything at this point */
> opts = qemu_opts_find(list, NULL);
> g_assert(opts == NULL);
>
> /* implicitly create opts and set str3 value */
> - qemu_opts_set(list, NULL, "str3", "value", &error_abort);
> + qemu_opts_set(list, "str3", "first_value", &error_abort);
This part the commit message mentions.
> + g_assert(!QTAILQ_EMPTY(&list->head));
> +
> + /* set it again */
> + qemu_opts_set(list, "str3", "value", &error_abort);
> g_assert(!QTAILQ_EMPTY(&list->head));
This one not.
What are you trying to accomplish?
>
> /* get the just created opts */
> diff --git a/util/qemu-option.c b/util/qemu-option.c
> index 59be4f9d21..c88e159f18 100644
> --- a/util/qemu-option.c
> +++ b/util/qemu-option.c
> @@ -665,15 +665,12 @@ void qemu_opts_loc_restore(QemuOpts *opts)
> loc_restore(&opts->loc);
> }
>
> -bool qemu_opts_set(QemuOptsList *list, const char *id,
> - const char *name, const char *value, Error **errp)
> +bool qemu_opts_set(QemuOptsList *list, const char *name, const char *value,
> Error **errp)
Long line. Please break before Error.
> {
> QemuOpts *opts;
>
> - opts = qemu_opts_create(list, id, 1, errp);
> - if (!opts) {
> - return false;
> - }
> + assert(list->merge_lists);
> + opts = qemu_opts_create(list, NULL, 0, &error_abort);
> return qemu_opt_set(opts, name, value, errp);
> }
Yes, qemu_opts_create() can fail only if its second paramter is
non-null, or its thirs paramter is non-zero.
I can see quite a few such calls. Could be simplified with a wrapper
that takes just the first parameter and can't fail. Not now.
Just enough confusion on my part to withhold my R-by for now.
