| Message ID | 20201109133931.979563-5-pbonzini@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Deprecate or forbid crazy QemuOpts cases | expand |
Paolo Bonzini <pbonzini@redhat.com> writes: > Forbid ids if the option is intended to be a singleton, as indicated by > list->merge_lists. Well, ->merge_lists need not imply singleton. Perhaps we only ever use it that way. Careful review is called for. > This avoids that "./qemu-system-x86_64 -M q35,id=ff" > uses a "pc" machine type. Just like any other QemuOptsList, "machine" may have any number of QemuOpts. The ones with non-null ID happen to be ignored silently. Known[*] trap for the unwary. > Instead it errors out. The affected options > are "qemu-img reopen -o", reopen_opts in qemu-io-cmds.c > "qemu-io open -o", empty_opts in qemu-io.c > -rtc, -M, -boot, -name, > -m, -icount, -smp, qemu_rtc_opts, qemu_machine_opts, qemu_boot_opts, qemu_name_opts, qemu_mem_opts, qemu_icount_opts, qemu_smp_opts in vl.c > -spice. qemu_spice_opts in spice-core.c. Are these all singletons? There's also machine_opts in qemu-config.c, but that's only to make query-command-line-options lie backward-compatibly. > > 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. Sounds like you're specializing the code (which might be good). > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > util/qemu-option.c | 27 ++++++++++++++------------- > 1 file changed, 14 insertions(+), 13 deletions(-) > > 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; > + } If lists>merge_lists, you no longer check id_wellformed(). Easy enough to fix: lift the check before this conditional. > + } 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)); Paths through the function before the patch: id fail_if_exists merge_lists | return null don't care false | new opts null don't care true | existing or else new opts non-null false don't care | existing or else new opts non-null true true | existing or else new opts non-null true false | new opts / fail if exist Too many cases. After the patch: id fail_if_exists merge_lists | return non-null don't care true | fail null don't care true | existing or else new opts non-null false false | abort non-null true false | new opts / fail if exist null don't care false | new opts Still too many :) I'm running out of time for today, and I still need to convince myself that the changes in behavior are all okay. > @@ -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; [*] Known to the QemuOpts cognoscenti, whose number could be embarrasingly close to one.
On 09/11/20 17:56, Markus Armbruster wrote: > Just like any other QemuOptsList, "machine" may have any number of > QemuOpts. The ones with non-null ID happen to be ignored silently. > Known[*] trap for the unwary. > > Are these all singletons? They are never qemu_opts_find'd with non-NULL id, so I'd say they are. > If lists>merge_lists, you no longer check id_wellformed(). Easy enough > to fix: lift the check before this conditional. Intentional: we always error with INVALID_PARAMETER, so it's pointless to check if the id is well-formed. > After the patch: > > id fail_if_exists merge_lists | return > non-null don't care true | fail > null don't care true | existing or else new opts > non-null false false | abort > non-null true false | new opts / fail if exist > null don't care false | new opts > > Still too many Discounting the case that aborts as it's not user-controlled (it's "just" a matter of inspecting qemu_opts_create callers), the rest can be summarized as: - merge_lists = false: singleton opts with NULL id; non-NULL id fails - merge_lists = true: always return new opts; non-NULL id fails if dup > [*] Known to the QemuOpts cognoscenti, whose number could be > embarrasingly close to one. Maybe not one, but a single hand certainly has a surplus of fingers. Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > On 09/11/20 17:56, Markus Armbruster wrote: >> Just like any other QemuOptsList, "machine" may have any number of >> QemuOpts. The ones with non-null ID happen to be ignored silently. >> Known[*] trap for the unwary. >> >> Are these all singletons? > > They are never qemu_opts_find'd with non-NULL id, so I'd say they are. We also need to check qemu_opts_foreach(). >> If lists>merge_lists, you no longer check id_wellformed(). Easy enough >> to fix: lift the check before this conditional. > > Intentional: we always error with INVALID_PARAMETER, so it's pointless > to check if the id is well-formed. My head was about to explode, and then it farted instead. Sorry fore the noise! >> After the patch: >> >> id fail_if_exists merge_lists | return >> non-null don't care true | fail >> null don't care true | existing or else new opts >> non-null false false | abort >> non-null true false | new opts / fail if exist >> null don't care false | new opts >> >> Still too many > > Discounting the case that aborts as it's not user-controlled (it's > "just" a matter of inspecting qemu_opts_create callers), the rest can be > summarized as: > > - merge_lists = false: singleton opts with NULL id; non-NULL id fails Do you mean merge_lists = true here, and ... > - merge_lists = true: always return new opts; non-NULL id fails if dup ... = false here? >> [*] Known to the QemuOpts cognoscenti, whose number could be >> embarrasingly close to one. > > Maybe not one, but a single hand certainly has a surplus of fingers. > > Paolo
On 09/11/20 19:38, Markus Armbruster wrote: >> They are never qemu_opts_find'd with non-NULL id, so I'd say they are. > > We also need to check qemu_opts_foreach(). Using qemu_opts_foreach means that e.g. -name id=... was not ignored unlike -M id=.... However, it will be an error now. We have to check if the callback or its callees use the opt->id Reminder of how the affected options are affected: reopen_opts in qemu-io-cmds.c qemu_opts_find(&reopen_opts, NULL) empty_opts in qemu-io.c qemu_opts_find(&empty_opts, NULL) qemu_rtc_opts qemu_find_opts_singleton("rtc") qemu_machine_opts qemu_find_opts_singleton("machine") qemu_boot_opts QTAILQ_FIRST(&qemu_find_opts("bootopts")->head) qemu_name_opts qemu_opts_foreach->parse_name parse_name does not use id qemu_mem_opts qemu_find_opts_singleton("memory") qemu_icount_opts qemu_opts_foreach->do_configuree_icount do_configure_icount->icount_configure icount_configure does not use id qemu_smp_opts qemu_opts_find(qemu_find_opts("smp-opts"), NULL) qemu_spice_opts QTAILQ_FIRST(&qemu_spice_opts.head) To preempt your question, I can add this in the commit message. Anyway I think it's relatively self-explanatory for most of these that they do not need "id". >> - merge_lists = false: singleton opts with NULL id; non-NULL id fails > > Do you mean merge_lists = true here, and ... > >> - merge_lists = true: always return new opts; non-NULL id fails if dup > > ... = false here? Of course. 1-1 in the brain fart competition. Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > On 09/11/20 19:38, Markus Armbruster wrote: >>> They are never qemu_opts_find'd with non-NULL id, so I'd say they are. >> >> We also need to check qemu_opts_foreach(). > > Using qemu_opts_foreach means that e.g. -name id=... was not ignored > unlike -M id=.... However, it will be an error now. We have to check > if the callback or its callees use the opt->id Yes. > Reminder of how the affected options are affected: In general, the patch rejects only options that used to be silently ignored. As we will see below, there are exceptions where we reject options that used to work. Do we want that? If yes, discuss in commit message and release notes. More below. > reopen_opts in qemu-io-cmds.c qemu_opts_find(&reopen_opts, NULL) qopts = qemu_opts_find(&reopen_opts, NULL); opts = qopts ? qemu_opts_to_qdict(qopts, NULL) : qdict_new(); qemu_opts_reset(&reopen_opts); I guess this could use qemu_find_opts_singleton(). Not sure we want it, and even if we do, it's not this patch's job. > > empty_opts in qemu-io.c qemu_opts_find(&empty_opts, NULL) Likewise. > qemu_rtc_opts qemu_find_opts_singleton("rtc") > > qemu_machine_opts qemu_find_opts_singleton("machine") No surprises or funnies here. > qemu_boot_opts > QTAILQ_FIRST(&qemu_find_opts("bootopts")->head) Spelled "boot-opts", and used with a variable spliced on, which defeated my grep. It's in hw/nvram/fw_cfg.c and hw/s390x/ipl.c. vl.c additionally has qemu_opts_find(qemu_find_opts("boot-opts"), NULL). If the user passes multiple -boot with different ID, we merge the ones with same ID, and then vl.c gets the (merged) one without ID, but the other two get the (merged) one that contains the first -boot. All three silently ignore the ones they don't get. Awesomely weird. I'd call it a bug. Test case: $ upstream-qemu -S -display none -boot strict=on,id=id -boot strict=off vl.c uses strict=off, but fw_cfg.c uses strinct=on,id=id. Outlawing "id" with .merge_lists like this patch does turns the cases where the two methods yield different results into errors. This could break working (if crazy) configurations. That's okay; I can't see how the craziness could be fixed without breaking crazy configurations. I think the commit message should cover this. I believe we could use qemu_find_opts_singleton() in all three spots. Not this patch's job. > > qemu_name_opts qemu_opts_foreach->parse_name > parse_name does not use id First, we use .merge_lists to merge -name with the same ID into a single QemuOpts, then we use code to merge the resulting QemuOpts, ignoring ID. Stupid. Outlawing "id" with .merge_lists like this patch does makes the second merge a no-op. This is one of the cases where we reject options that used to work. If that's wanted, follow-up patch to drop the useless second merge. If not, unset qemu_name_opts.merge_lists in a separate patch before this one. > qemu_mem_opts qemu_find_opts_singleton("memory") No surprises or funnies here. > qemu_icount_opts qemu_opts_foreach->do_configuree_icount > do_configure_icount->icount_configure > icount_configure does not use id Same story as for qemu_name_opts. > qemu_smp_opts > qemu_opts_find(qemu_find_opts("smp-opts"), NULL) No surprises or funnies here. > qemu_spice_opts QTAILQ_FIRST(&qemu_spice_opts.head) We use the merged one that contains the first -spice, and silently ignore the rest. At least we're consistent here. This is one of the cases where we reject options that used to work. If that's wanted, follow-up patch to replace the QTAILQ_FIRST() by something saner. If not, unset qemu_spice_opts.merge_lists in a separate patch before this one, and merge like we do for qemu_name_opts. > To preempt your question, I can add this in the commit message. Anyway > I think it's relatively self-explanatory for most of these that they do > not need "id". Except where they don't need it, but permit it to have an effect anyway. One of the issues with QemuOpts is that there are too many "clever" ways to use it. >>> - merge_lists = false: singleton opts with NULL id; non-NULL id fails >> >> Do you mean merge_lists = true here, and ... >> >>> - merge_lists = true: always return new opts; non-NULL id fails if dup >> >> ... = false here? > > Of course. 1-1 in the brain fart competition. Hah!
On 10/11/20 09:29, Markus Armbruster wrote: > As we will see below, there are exceptions where we reject > options that used to work. Do we want that? I think that, as long as we gain in consistency, we do. The special casing of "id" is a wart of the current parser and the less it shows that "id" is treated specially, the best. Deprecating or downright rejecting working combinations is always walking a fine line, but here we're almost in https://xkcd.com/1172/ territory. Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > On 10/11/20 09:29, Markus Armbruster wrote: >> As we will see below, there are exceptions where we reject >> options that used to work. Do we want that? > > I think that, as long as we gain in consistency, we do. The special > casing of "id" is a wart of the current parser and the less it shows > that "id" is treated specially, the best. > > Deprecating or downright rejecting working combinations is always > walking a fine line, but here we're almost in https://xkcd.com/1172/ > territory. I find it hard to disagree ;)
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;
Forbid ids if the option is intended to be a singleton, as indicated by list->merge_lists. This avoids that "./qemu-system-x86_64 -M q35,id=ff" uses a "pc" machine type. Instead it errors out. The affected options are "qemu-img reopen -o", "qemu-io open -o", -rtc, -M, -boot, -name, -m, -icount, -smp, -spice. 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. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- util/qemu-option.c | 27 ++++++++++++++------------- 1 file changed, 14 insertions(+), 13 deletions(-)