diff mbox series

[PULL,29/31] qemu-option: clean up id vs. list->merge_lists

Message ID 20210123143128.1167797-30-pbonzini@redhat.com (mailing list archive)
State New, archived
Headers show
Series [PULL,01/31] runstate: cleanup reboot and panic actions | expand

Commit Message

Paolo Bonzini Jan. 23, 2021, 2:31 p.m. UTC
Looking at all merge-lists QemuOptsList, here is how they access their
QemuOpts:

reopen_opts in qemu-io-cmds.c ("qemu-img reopen -o")
	qemu_opts_find(&reopen_opts, NULL)

empty_opts in qemu-io.c ("qemu-io open -o")
	qemu_opts_find(&empty_opts, NULL)

qemu_rtc_opts ("-rtc")
	qemu_find_opts_singleton("rtc")

qemu_machine_opts ("-M")
	qemu_find_opts_singleton("machine")

qemu_action_opts ("-name")
	qemu_opts_foreach->process_runstate_actions

qemu_boot_opts ("-boot")
	in hw/nvram/fw_cfg.c and hw/s390x/ipl.c:
	  QTAILQ_FIRST(&qemu_find_opts("bootopts")->head)
	in softmmu/vl.c:
	  qemu_opts_find(qemu_find_opts("boot-opts"), NULL)

qemu_name_opts ("-name")
	qemu_opts_foreach->parse_name
	parse_name does not use id

qemu_mem_opts ("-m")
	qemu_find_opts_singleton("memory")

qemu_icount_opts ("-icount")
	qemu_opts_foreach->do_configure_icount
	do_configure_icount->icount_configure
	icount_configure does not use id

qemu_smp_opts ("-smp")
	qemu_opts_find(qemu_find_opts("smp-opts"), NULL)

qemu_spice_opts ("-spice")
	QTAILQ_FIRST(&qemu_spice_opts.head)

i.e. they don't need an id.  Sometimes its presence is ignored
(e.g. when using qemu_opts_foreach), sometimes all the options
with the id are skipped, sometimes only the first option on the
command line is considered.  -boot does two different things
depending on who's looking at the options.

With this patch we just forbid id on merge-lists QemuOptsLists; if the
command line still works, it has the same semantics as before.

qemu_opts_create's fail_if_exists parameter is now unnecessary:

- it is unused if id is NULL

- opts_parse only passes false if reached from qemu_opts_set_defaults,
in which case this patch enforces that id must be NULL

- other callers that can pass a non-NULL id always set it to true

Assert that it is true in the only case where "fail_if_exists" matters,
i.e. "id && !lists->merge_lists".  This means that if an id is present,
duplicates are always forbidden, which was already the status quo.

Discounting the case that aborts as it's not user-controlled (it's
"just" a matter of inspecting qemu_opts_create callers), the paths
through qemu_opts_create can be summarized as:

- merge_lists = true: singleton opts with NULL id; non-NULL id fails

- merge_lists = false: always return new opts; non-NULL id fails if dup

Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 util/qemu-option.c | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

Comments

Markus Armbruster Jan. 25, 2021, 7:42 a.m. UTC | #1
Paolo Bonzini <pbonzini@redhat.com> writes:

> Looking at all merge-lists QemuOptsList, here is how they access their
> QemuOpts:
>
> reopen_opts in qemu-io-cmds.c ("qemu-img reopen -o")
> 	qemu_opts_find(&reopen_opts, NULL)
>
> empty_opts in qemu-io.c ("qemu-io open -o")
> 	qemu_opts_find(&empty_opts, NULL)
>
> qemu_rtc_opts ("-rtc")
> 	qemu_find_opts_singleton("rtc")
>
> qemu_machine_opts ("-M")
> 	qemu_find_opts_singleton("machine")
>
> qemu_action_opts ("-name")

Pasto: it's "-action".

> 	qemu_opts_foreach->process_runstate_actions
>
> qemu_boot_opts ("-boot")
> 	in hw/nvram/fw_cfg.c and hw/s390x/ipl.c:
> 	  QTAILQ_FIRST(&qemu_find_opts("bootopts")->head)
> 	in softmmu/vl.c:
> 	  qemu_opts_find(qemu_find_opts("boot-opts"), NULL)
>
> qemu_name_opts ("-name")
> 	qemu_opts_foreach->parse_name
> 	parse_name does not use id
>
> qemu_mem_opts ("-m")
> 	qemu_find_opts_singleton("memory")
>
> qemu_icount_opts ("-icount")
> 	qemu_opts_foreach->do_configure_icount
> 	do_configure_icount->icount_configure
> 	icount_configure does not use id
>
> qemu_smp_opts ("-smp")
> 	qemu_opts_find(qemu_find_opts("smp-opts"), NULL)
>
> qemu_spice_opts ("-spice")
> 	QTAILQ_FIRST(&qemu_spice_opts.head)
>
> i.e. they don't need an id.  Sometimes its presence is ignored
> (e.g. when using qemu_opts_foreach), sometimes all the options
> with the id are skipped, sometimes only the first option on the

Let's insert

    (when using qemu_find_opts_singleton() or qemu_opts_find(list, NULL))

right after skipped, and

> command line is considered.  -boot does two different things

    (when using QTAILQ_FIRST)

right after considered.

> depending on who's looking at the options.
>
> With this patch we just forbid id on merge-lists QemuOptsLists; if the
> command line still works, it has the same semantics as before.
>
> qemu_opts_create's fail_if_exists parameter is now unnecessary:
>
> - it is unused if id is NULL
>
> - opts_parse only passes false if reached from qemu_opts_set_defaults,
> in which case this patch enforces that id must be NULL
>
> - other callers that can pass a non-NULL id always set it to true
>
> Assert that it is true in the only case where "fail_if_exists" matters,
> i.e. "id && !lists->merge_lists".  This means that if an id is present,
> duplicates are always forbidden, which was already the status quo.
>
> Discounting the case that aborts as it's not user-controlled (it's
> "just" a matter of inspecting qemu_opts_create callers), the paths
> through qemu_opts_create can be summarized as:
>
> - merge_lists = true: singleton opts with NULL id; non-NULL id fails
>
> - merge_lists = false: always return new opts; non-NULL id fails if dup
>
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Markus Armbruster <armbru@redhat.com>
Paolo Bonzini Jan. 25, 2021, 7:58 a.m. UTC | #2
Too late but I will point it out in the commit that cleans up the iteration.

Paolo

Il lun 25 gen 2021, 08:42 Markus Armbruster <armbru@redhat.com> ha scritto:

> Paolo Bonzini <pbonzini@redhat.com> writes:
>
> > Looking at all merge-lists QemuOptsList, here is how they access their
> > QemuOpts:
> >
> > reopen_opts in qemu-io-cmds.c ("qemu-img reopen -o")
> >       qemu_opts_find(&reopen_opts, NULL)
> >
> > empty_opts in qemu-io.c ("qemu-io open -o")
> >       qemu_opts_find(&empty_opts, NULL)
> >
> > qemu_rtc_opts ("-rtc")
> >       qemu_find_opts_singleton("rtc")
> >
> > qemu_machine_opts ("-M")
> >       qemu_find_opts_singleton("machine")
> >
> > qemu_action_opts ("-name")
>
> Pasto: it's "-action".
>
> >       qemu_opts_foreach->process_runstate_actions
> >
> > qemu_boot_opts ("-boot")
> >       in hw/nvram/fw_cfg.c and hw/s390x/ipl.c:
> >         QTAILQ_FIRST(&qemu_find_opts("bootopts")->head)
> >       in softmmu/vl.c:
> >         qemu_opts_find(qemu_find_opts("boot-opts"), NULL)
> >
> > qemu_name_opts ("-name")
> >       qemu_opts_foreach->parse_name
> >       parse_name does not use id
> >
> > qemu_mem_opts ("-m")
> >       qemu_find_opts_singleton("memory")
> >
> > qemu_icount_opts ("-icount")
> >       qemu_opts_foreach->do_configure_icount
> >       do_configure_icount->icount_configure
> >       icount_configure does not use id
> >
> > qemu_smp_opts ("-smp")
> >       qemu_opts_find(qemu_find_opts("smp-opts"), NULL)
> >
> > qemu_spice_opts ("-spice")
> >       QTAILQ_FIRST(&qemu_spice_opts.head)
> >
> > i.e. they don't need an id.  Sometimes its presence is ignored
> > (e.g. when using qemu_opts_foreach), sometimes all the options
> > with the id are skipped, sometimes only the first option on the
>
> Let's insert
>
>     (when using qemu_find_opts_singleton() or qemu_opts_find(list, NULL))
>
> right after skipped, and
>
> > command line is considered.  -boot does two different things
>
>     (when using QTAILQ_FIRST)
>
> right after considered.
>
> > depending on who's looking at the options.
> >
> > With this patch we just forbid id on merge-lists QemuOptsLists; if the
> > command line still works, it has the same semantics as before.
> >
> > qemu_opts_create's fail_if_exists parameter is now unnecessary:
> >
> > - it is unused if id is NULL
> >
> > - opts_parse only passes false if reached from qemu_opts_set_defaults,
> > in which case this patch enforces that id must be NULL
> >
> > - other callers that can pass a non-NULL id always set it to true
> >
> > Assert that it is true in the only case where "fail_if_exists" matters,
> > i.e. "id && !lists->merge_lists".  This means that if an id is present,
> > duplicates are always forbidden, which was already the status quo.
> >
> > Discounting the case that aborts as it's not user-controlled (it's
> > "just" a matter of inspecting qemu_opts_create callers), the paths
> > through qemu_opts_create can be summarized as:
> >
> > - merge_lists = true: singleton opts with NULL id; non-NULL id fails
> >
> > - merge_lists = false: always return new opts; non-NULL id fails if dup
> >
> > Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>
>
diff mbox series

Patch

diff --git a/util/qemu-option.c b/util/qemu-option.c
index c88e159f18..91f4120ce1 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -619,7 +619,17 @@  QemuOpts *qemu_opts_create(QemuOptsList *list, const char *id,
 {
     QemuOpts *opts = NULL;
 
-    if (id) {
+    if (list->merge_lists) {
+        if (id) {
+            error_setg(errp, QERR_INVALID_PARAMETER, "id");
+            return NULL;
+        }
+        opts = qemu_opts_find(list, NULL);
+        if (opts) {
+            return opts;
+        }
+    } else if (id) {
+        assert(fail_if_exists);
         if (!id_wellformed(id)) {
             error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "id",
                        "an identifier");
@@ -629,17 +639,8 @@  QemuOpts *qemu_opts_create(QemuOptsList *list, const char *id,
         }
         opts = qemu_opts_find(list, id);
         if (opts != NULL) {
-            if (fail_if_exists && !list->merge_lists) {
-                error_setg(errp, "Duplicate ID '%s' for %s", id, list->name);
-                return NULL;
-            } else {
-                return opts;
-            }
-        }
-    } else if (list->merge_lists) {
-        opts = qemu_opts_find(list, NULL);
-        if (opts) {
-            return opts;
+            error_setg(errp, "Duplicate ID '%s' for %s", id, list->name);
+            return NULL;
         }
     }
     opts = g_malloc0(sizeof(*opts));
@@ -893,7 +894,7 @@  static QemuOpts *opts_parse(QemuOptsList *list, const char *params,
      * (if unlikely) future misuse:
      */
     assert(!defaults || list->merge_lists);
-    opts = qemu_opts_create(list, id, !defaults, errp);
+    opts = qemu_opts_create(list, id, !list->merge_lists, errp);
     g_free(id);
     if (opts == NULL) {
         return NULL;