Paolo Bonzini <pbonz...@redhat.com> writes:
> On 19/01/21 14:58, Markus Armbruster wrote:
>>> qemu_machine_opts ("-M")
>>> qemu_find_opts_singleton("machine")
>>
>> 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.
I got confused.
>> 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.
Thanks!
>> [qemu_action_opts]
>> 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.
Because it's a clever way to do something that should not be done :)
-action wraps around QMP command set-action. This means we need to
parse -action's argument into set-action's argument type. That's a QAPI
type. It's anonymous in the schema, and q_obj_set_action_arg in C.
As implemented, the parsing takes a detour through QemuOpts:
char *optarg
|
| opts = qemu_opts_parse_noisily(optarg);
v
QemuOpts *opts
|
| qdict = qemu_opts_to_qdict(opts, NULL);
v
QDict *qdict
|
| in qmp_marshal_set_action(qdict):
| v = qobject_input_visitor_new_str(qdict);
| visit_type_q_obj_set_action_arg_members(v, &arg, errp);
v
q_obj_set_action_arg arg
qmp_marshal_set_action() then passes the members of
q_obj_set_action_arg() to qmp_set_action().
The detour should be avoided, because QemuOpts should be avoided. Using
the appropriate visitor, we get:
char *optarg
|
| v = qobject_input_visitor_new_str(string, NULL, errp)
| visit_type_q_obj_set_action_arg(v, NULL, &arg, errp);
v
q_obj_set_action_arg arg
except visit_type_q_obj_set_action_arg() doesn't exist, because the QAPI
type is anonymous. So give it a name:
{ 'struct: 'Action',
'data': { '*reboot': 'RebootAction',
'*shutdown': 'ShutdownAction',
'*panic': 'PanicAction',
'*watchdog': 'WatchdogAction' } }
{ 'command': 'set-action',
'data': 'Action',
'allow-preconfig': true }
char *optarg
|
| v = qobject_input_visitor_new_str(string, NULL, errp)
| visit_type_Action(v, NULL, &arg, errp);
v
Action act
To avoid having to pass the members of Action to qmp_set_action(), throw
in 'boxed': true, so you can simply call qmp_set_action(&act, errp).
Aside: I'd like to have this QAPI/CLI boilerplate generated, like we
generate the QAPI/QMP boilerplate. Not today; QAPI-land is busy with
John's static typing work.
>>> 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.
Yes.
>> [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".
D'oh! QemuOpts left me no brain capacity for remembering the simplest
things %-}
>>> 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).
Me too.
