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.
Signed-off-by: John Ferlan <jferlan@redhat.com>
---
src/qemu/qemu_hotplug.c | 10 +++++++++-
src/qemu/qemu_monitor.c | 15 +++++++++++++++
src/qemu/qemu_monitor.h | 2 ++
src/qemu/qemu_monitor_json.c | 28 ++++++++++++++++++++++++++++
src/qemu/qemu_monitor_json.h | 2 ++
tests/qemumonitorjsontest.c | 2 ++
6 files changed, 58 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 0ee9b2bfc..a012b3dcd 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -5333,7 +5333,15 @@ qemuDomainChangeGraphicsPasswords(virQEMUDriverPtr driver,
if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
goto cleanup;
- ret = qemuMonitorSetPassword(priv->mon, type, password, connected);
+
+ /* The connected parameter can only be keep for VNC, thus QMP command
+ * set_password makes the same call hence if available just use the
+ * newer change-vnc-password command. */
+ if (type == VIR_DOMAIN_GRAPHICS_TYPE_VNC &&
+ virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_CHANGE_VNC_PASSWORD))
+ ret = qemuMonitorSetVNCPassword(priv->mon, password);
+ else
+ ret = qemuMonitorSetPassword(priv->mon, type, password, connected);
if (ret == -2) {
if (type != VIR_DOMAIN_GRAPHICS_TYPE_VNC) {
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 56c8345d5..c88634a38 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -2387,6 +2387,21 @@ qemuMonitorSetVNCPasswordLegacy(qemuMonitorPtr mon,
}
+int
+qemuMonitorSetVNCPassword(qemuMonitorPtr mon,
+ const char *password)
+{
+ VIR_DEBUG("password=%p", password);
+
+ QEMU_CHECK_MONITOR_JSON(mon);
+
+ if (!password)
+ password = "";
+
+ return qemuMonitorJSONSetVNCPassword(mon, password);
+}
+
+
static const char *
qemuMonitorTypeToProtocol(int type)
{
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index cf9ab7cb3..eeae72928 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -597,6 +597,8 @@ int qemuMonitorBlockResize(qemuMonitorPtr mon,
unsigned long long size);
int qemuMonitorSetVNCPasswordLegacy(qemuMonitorPtr mon,
const char *password);
+int qemuMonitorSetVNCPassword(qemuMonitorPtr mon,
+ const char *password);
int qemuMonitorSetPassword(qemuMonitorPtr mon,
int type,
const char *password,
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index ad81b70db..f8131ac4d 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -2384,6 +2384,34 @@ qemuMonitorJSONSetVNCPasswordLegacy(qemuMonitorPtr mon,
return ret;
}
+
+int
+qemuMonitorJSONSetVNCPassword(qemuMonitorPtr mon,
+ const char *password)
+{
+ int ret = -1;
+ virJSONValuePtr cmd = qemuMonitorJSONMakeCommand("change-vnc-password",
+ "s:password", password,
+ NULL);
+ virJSONValuePtr reply = NULL;
+
+ if (!cmd)
+ return -1;
+
+ if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0)
+ goto cleanup;
+
+ if (qemuMonitorJSONCheckError(cmd, reply) < 0)
+ goto cleanup;
+
+ ret = 0;
+ cleanup:
+ virJSONValueFree(cmd);
+ virJSONValueFree(reply);
+ return ret;
+}
+
+
/* Returns -1 on error, -2 if not supported */
int qemuMonitorJSONSetPassword(qemuMonitorPtr mon,
const char *protocol,
diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
index dec7a5cf9..b3254ac05 100644
--- a/src/qemu/qemu_monitor_json.h
+++ b/src/qemu/qemu_monitor_json.h
@@ -94,6 +94,8 @@ int qemuMonitorJSONBlockResize(qemuMonitorPtr mon,
int qemuMonitorJSONSetVNCPasswordLegacy(qemuMonitorPtr mon,
const char *password);
+int qemuMonitorJSONSetVNCPassword(qemuMonitorPtr mon,
+ const char *password);
int qemuMonitorJSONSetPassword(qemuMonitorPtr mon,
const char *protocol,
const char *password,
diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c
index f9c59f2f0..6d33333a0 100644
--- a/tests/qemumonitorjsontest.c
+++ b/tests/qemumonitorjsontest.c
@@ -1317,6 +1317,7 @@ cleanup: \
GEN_TEST_FUNC(qemuMonitorJSONSetLink, "vnet0", VIR_DOMAIN_NET_INTERFACE_LINK_STATE_DOWN)
GEN_TEST_FUNC(qemuMonitorJSONBlockResize, "vda", 123456)
GEN_TEST_FUNC(qemuMonitorJSONSetVNCPasswordLegacy, "secret_password")
+GEN_TEST_FUNC(qemuMonitorJSONSetVNCPassword, "secret_password")
GEN_TEST_FUNC(qemuMonitorJSONSetPassword, "spice", "secret_password", "disconnect")
GEN_TEST_FUNC(qemuMonitorJSONExpirePassword, "spice", "123456")
GEN_TEST_FUNC(qemuMonitorJSONSetBalloon, 1024)
@@ -2906,6 +2907,7 @@ mymain(void)
DO_TEST_GEN(qemuMonitorJSONSetLink);
DO_TEST_GEN(qemuMonitorJSONBlockResize);
DO_TEST_GEN(qemuMonitorJSONSetVNCPasswordLegacy);
+ DO_TEST_GEN(qemuMonitorJSONSetVNCPassword);
DO_TEST_GEN(qemuMonitorJSONSetPassword);
DO_TEST_GEN(qemuMonitorJSONExpirePassword);
DO_TEST_GEN(qemuMonitorJSONSetBalloon);
--
2.13.6
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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". 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. 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 :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2025 Red Hat, Inc.