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

[Stefan Reiter]

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:
> > > Stefan Reiter <s.reiter@proxmox.com> writes:
> > >
> > > > It is possible to specify more than one VNC server on the command line,
> > > > either with an explicit ID or the auto-generated ones à "default",
> > > > "vnc2", "vnc3", ...
> > > >
> > > > It is not possible to change the password on one of these extra VNC
> > > > displays though. Fix this by adding a "display" parameter to the
> > > > "set_password" and "expire_password" QMP and HMP commands.
> > > >
> > > > For HMP, the display is specified using the "-d" value flag.
> > > >
> > > > For QMP, the schema is updated to explicitly express the supported
> > > > variants of the commands with protocol-discriminated unions.
> > > >
> > > > Suggested-by: Eric Blake <eblake@redhat.com>
> > > > Suggested-by: Markus Armbruster <armbru@redhat.com>
> > > > Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
> > >
> > > [...]
> > >
> > > > [...]
> > >
> > > Let me describe what you're doing in my own words, to make sure I got
> > > both the what and the why:
> > >
> > > 1. Change @protocol and @connected from str to enum.
> > >
> > > 2. Turn the argument struct into a union tagged by @protocol, to enable
> > >      3. and 4.
> > >
> > > 3. Add argument @display in branch 'vnc'.
> > >
> > > 4. Use a separate enum for @connected in each union branch, to enable 4.
> > >
> > > 5. Trim this enum in branch 'vnc' to the values that are actually
> > >      supported.  Note that just one value remains, i.e. @connected is now
> > >      an optional argument that can take only the default value.
> > >
> > > 6. Since such an argument is pointless, deprecate @connected in branch
> > >      'vnc'.
> > >
> > > Correct?
> >
> > Yes.
>
> Thanks!
>
> > > Ignorant question: is it possible to have more than one 'spice' display?
> >
> > Not in terms of passwords anyway. So only one SPICE password can be set at
> > any time, i.e. the 'display' parameter in this function does not apply.
> >
> > > Item 5. is not a compatibility break: before your patch,
> > > qmp_set_password() rejects the values you drop.
> >
> > Yes.
> >
> > > Item 5. adds another enum to the schema in return for more accurate
> > > introspection and slightly simpler qmp_set_password().  I'm not sure
> > > that's worthwhile.  I think we could use the same enum for both union
> > > branches.
> >
> > I tried to avoid mixing manual parsing and declarative QAPI stuff as much
> > as I could, so I moved as much as possible to QAPI. I personally like the
> > extra documentation of having the separate enum.
>
> It's a valid choice.  I'm just pointing out another valid choice.  The
> difference between them will go away when we remove deprecated
> @connected.  You choose :)
>
> > > 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.

> >                       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). 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]
  }

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.