[libvirt] [PATCH 4/4] qemu: Add support for change-vnc-password

[John Ferlan]

On 03/06/2018 10:07 AM, Daniel P. Berrangé wrote:
> On Tue, Mar 06, 2018 at 09:47:02AM -0500, John Ferlan wrote:
>> Rather than use the to be deprecated "change" command, use the
>> change-vnc-password command to perform VNC password changes instead
>> of the set_password command. Since changing VNC password only
>> accepts "keep" for connect and that's the default, no sense in
>> vectoring through "set_password" as we've already checked that the
>> XML 'connected' was set to "keep" in virDomainGraphicsAuthDefParseXML.
> 
> This explanation doesn't make any sense to me.
> 
> Currently, we try 'set_password' first, and if that's not present
> we fallback to the legacy "change" method. This is fine because
> all QEMU since 0.14 have supported "set_password", so we only
> need 'change' for really ancient QEMU.   "set_password" works with
> both SPICE and VNC.
> 
> The "change-vnc-password" method was only introduced in 1.1 QEMU,
> so there's never a situation where we would not have "set_password"
> and yet have "change-vnc-password".

FWIW: When set_password was introduced, rather than go with capability
checking - the code checks for error of CommandNotFound and falls back
to the old "change" command.

> 
> I'm rather puzzelled why  "change-vnc-password" was added to QEMU
> at all, since it seems to be entirely less functional than
> 'set_password' which already exists.  Thus I don't see any benefit
> in using it from libvirt.
> 

Having the value of connected as "keep" is the only way set_password
works for VNC:

set_password(...)
...
    if (strcmp(protocol, "vnc") == 0) {
        if (fail_if_connected || disconnect_if_connected) {
            /* vnc supports "connected=keep" only */
            error_setg(errp, QERR_INVALID_PARAMETER, "connected");
            return;
        }
...

I can only assume the change-vnc-password was introduced to avoid the
need to use set_password and passing "keep" for connected if provided.
Maybe a secondary goal is/was to allow VNC password processing not have
to worry about other changes in set_password. Who knows - my desire to
chase that history is next to nil.

I can drop the series - it doesn't really matter beyond just seeing the
"change" is deprecated. In the long run both have the same
functionality, it's just the change-vnc-password is more direct. The
virDomainGraphicsAuthDefParseXML also checks for "keep", so in the long
run everything is a wash.  In the hindsight of hypervisor specific
checks the ParseXML check probably should be in some qemu specific checker.

John