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

[Markus Armbruster <armbru@redhat.com>]

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