Re: [PATCH v7 0/4] VNC-related HMP/QMP fixes

["Daniel P. Berrangé" <berrange@redhat.com>]

On Mon, Jan 31, 2022 at 10:45:08AM +0100, Fabian Ebner wrote:
> Am 25.01.22 um 16:06 schrieb Daniel P. Berrangé:
> > On Mon, Jan 24, 2022 at 02:50:39PM +0100, Markus Armbruster wrote:
> >> Stefan Reiter <s.reiter@proxmox.com> writes:
> >>
> >>> Since the removal of the generic 'qmp_change' command, one can no longer replace
> >>> the 'default' VNC display listen address at runtime (AFAIK). For our users who
> >>> need to set up a secondary VNC access port, this means configuring a second VNC
> >>> display (in addition to our standard one for web-access), but it turns out one
> >>> cannot set a password on this second display at the moment, as the
> >>> 'set_password' call only operates on the 'default' display.
> >>>
> >>> Additionally, using secret objects, the password is only read once at startup.
> >>> This could be considered a bug too, but is not touched in this series and left
> >>> for a later date.
> >>
> >> Related: Vladimir recently posted a patch to add a new command for
> >> changing VNC server listening addresses.  Daniel asked him to work it
> >> into display-reload instead[1].  Vladimir complied[2].
> >>
> >> Daniel, what do you think about this one?  Should it also use
> >> display-reload?
> > 
> > I'd ultimately intend to deprecate & remove the direct setting of
> > passwords on the CLI, and exclusively rely on the 'secret' object
> > for passing in passwords. With this in mind, I'd not be enthusiastic
> > about adding new commands for changing passwords in QMP directly,
> > rather I think we should have a way to change the 'secret' object
> > in use.
> > 
> 
> How should I proceed with this series then? Does adding the new argument
> for the display ID count as "adding new commands"?

Ok, so I've gone back and properly read the series. Since you're simply
extending existing commands with new arguments I've no objection to
carrying on with this approach.

We should still aim to have a general purpose command for live config
changes, but that shouldn't be considered a blocker for you series
here, as your series isn't making the existing situation worse.

> If what I should do is switching to only using secret objects, would the
> plan be something like the following?
> 
> 1. Add an option to display-reload for switching the display's
> password-secret while adding SPICE as a valid display type.
> 2. Also include the set password action (i.e. disconnect/fail/keep) and
> expiration time as part of that option.
> 3. Extend display-reload to also take an optional display ID for VNC.
> 4. Deprecate expire_password and set_password.

I've realized that we shouldn't overload display-reload for dual purposes.

We have two distinct usage scenarios that are meaningul

 - Re-loading the value of the existing secret
 - Changing which secret is used

The 'display-reload' command is reasonable for the first.

We should have a 'display-update' command for the second.

Either way, I don't think this series should be blocked on this since
it is merely modifying an existing command.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|