Re: [PATCH v4 2/2] monitor: refactor set/expire_password and allow VNC display id

[Markus Armbruster <armbru@redhat.com>]

Stefan Reiter <s.reiter@proxmox.com> writes:

> On 10/14/21 9:14 AM, Markus Armbruster wrote:
>> Stefan Reiter <s.reiter@proxmox.com> writes:
>> 
>>> On 10/12/21 11:24 AM, Markus Armbruster wrote:

[...]

>>>> I'd split this patch into three parts: item 1., 2.+3., 4.-6., because
>>>> each part can stand on its own.
>>>
>>> The reason why I didn't do that initially is more to do with the C side.
>>> I think splitting it up as you describe would make for some awkward diffs
>>> on the qmp_set_password/hmp_set_password side.
>>>
>>> I can try of course.
>> 
>> It's a suggestion, not a demand.  I'm a compulsory patch splitter.
>
> I'll just have a go and see what falls out. I do agree that this patch is a
> bit long on its own.

Thanks!

>>>                       Though I also want to have it said that I'm not a fan
>>> of how the HMP functions have to expand so much to accommodate the QAPI
>>> structure in general.
>> 
>> Care to elaborate?
>
> Well, before this patch 'hmp_set_password' was for all intents and purposes a
> single function call to 'qmp_set_password'. Now it has a bunch of parameter
> parsing and even validation (e.g. enum parsing).

Yes, HMP requires us to do more work manually than QMP does.  Raising
HMP's level of automation to QMP's would be nice, but it would also be a
big project.

>                                                  That's why I asked in the
> v3 patch (I think?) if there was a way to call the QAPI style parsing from
> there, since the parameters are all named the same and in a qdict already..
>
> Something like:
>
>    void hmp_set_password(Monitor *mon, const QDict *qdict)
>    {
>      ExpirePasswordOptions opts = qapi_magical_parse_fn(qdict);
>      qmp_set_password(&opts, &err);
>      [error handling]
>    }

Same structure as qmp_marshal_set_password(), where the "magical parse"
part uses a visitor function generated from the QAPI schema for its
argument type.

HMP works differently.  There, we only have .args_type in
hmp-commands.hx.  Since this is much less expressive than the QAPI
schema, generic code can do much less work for us.  Which means we get
to write more code by hand.

By converting QMP from 'str' to enum, we lift parsing from the
qmp_set_password() to its callers.  qmp_marshal_set_password() does it
for free.  hmp_set_password() needs handwritten code.  It wouldn't with
a QAPI-schema-based HMP, but as I said, that's a big project.

> That being said, I don't mind the current form enough to make this a bigger
> discussion either, so if there isn't an easy way to do the above, let's just
> leave it like it is.

There is no easy way to do the above.