Paolo Bonzini <pbonz...@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 <pbonz...@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.
