[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 01/25] qemu-option: clean up id vs. list->merge_lists

From: Paolo Bonzini
Subject: Re: [PATCH 01/25] qemu-option: clean up id vs. list->merge_lists
Date: Tue, 19 Jan 2021 15:20:41 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.6.0

On 19/01/21 14:58, Markus Armbruster wrote:
qemu_machine_opts ("-M")

Gone since your commit 4988a4ea4d "vl: switch -M parsing to keyval".

Which is part of this series and not yet included in QEMU. Hence the commit message talks about it in the present tense.

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.  A bug fix of
sorts.  Should the commit message should cover it?

Yeah, I can add that.

should not use QemuOpts at all.  Use of qmp_marshal_FOO() is an
anti-pattern.  Needs cleanup.  Not in this patch, and probably not even
in this series.

--verbose needed. Why is it an anti-pattern? I found it a clever way to avoid code duplication. :) Doesn't matter for this series, anyway.

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

It can break working (if weird) command lines, such as ones relying on
"merge ignoring ID" behavior of -name, -icount, -action.  Okay.

Right, I wrote that down as a feature. The important thing is keeping things the same if they still work.

[If !lists->merge_lists], if id= is specified, it must be unique,
i.e. no prior opts with the same id.

Else, we don't check for prior opts without id.

There's at most one opts with a certain id, but there could be any
number without id.  Is this what we want?

Yes, positively.  Example: "qemu-system-x86_64 -device foo -device bar".

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

This renders the qemu_opts_foreach() silly.  Cleanup is in order, not
necessarily in this patch.

Agreed.  This one is already tricky enough (though I like the outcome).


reply via email to

[Prev in Thread] Current Thread [Next in Thread]