| Message ID | 20210123143128.1167797-30-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [PULL,01/31] runstate: cleanup reboot and panic actions | expand |
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>
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 --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;
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(-)