[libvirt PATCH 0/3] qemu: add support for VNC power control option

Daniel P. Berrangé (3): conf: add support for VNC power control setting qemu: probe for -vnc power-control option support qemu: wire up support for VNC power control options docs/formatdomain.rst | 5 +++++ docs/schemas/domaincommon.rng | 5 +++++ src/conf/domain_conf.c | 12 ++++++++++++ src/conf/domain_conf.h | 1 + src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 9 +++++++++ tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml | 1 + tests/qemuxml2argvdata/graphics-vnc-policy.args | 2 +- tests/qemuxml2argvdata/graphics-vnc-policy.xml | 2 +- tests/qemuxml2argvtest.c | 2 +- 11 files changed, 39 insertions(+), 3 deletions(-) -- 2.29.2

The <graphics type="vnc" .... powerControl="on"/> option instructs the VNC server to enable an extension that lets the client perform a graceful shutdown, reboot and hard reset. This is enabled by default since it cannot be assumed that the VNC client user has administrator rights over the guest OS. In the case where the VNC user is a guest administrator though, it is reasonable to allow direct power control host side too. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- docs/formatdomain.rst | 5 +++++ docs/schemas/domaincommon.rng | 5 +++++ src/conf/domain_conf.c | 12 ++++++++++++ src/conf/domain_conf.h | 1 + 4 files changed, 23 insertions(+) diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index eafd6b3396..580319365c 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -5791,6 +5791,11 @@ interaction with the admin. ``autoport`` having no effect due to security reasons) :since:`Since 1.0.6` . + For VNC, the ``powerControl`` attribute can be used to enable VM shutdown, + reboot and reset power control features for the VNC client. This is + appropriate if the authenticated VNC client user already has administrator + privileges in the guest :since:`Since 7.1.0`. + Although VNC doesn't support OpenGL natively, it can be paired with graphics type ``egl-headless`` (see below) which will instruct QEMU to open and use drm nodes for OpenGL rendering. diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index e6de934456..49dc4b5130 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -3663,6 +3663,11 @@ </choice> </attribute> </optional> + <optional> + <attribute name="powerControl"> + <ref name="virYesNo"/> + </attribute> + </optional> </group> <group> <optional> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b731744f04..91933bf292 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -13150,6 +13150,7 @@ virDomainGraphicsDefParseXMLVNC(virDomainGraphicsDefPtr def, g_autofree char *websocketGenerated = virXMLPropString(node, "websocketGenerated"); g_autofree char *sharePolicy = virXMLPropString(node, "sharePolicy"); g_autofree char *autoport = virXMLPropString(node, "autoport"); + g_autofree char *powerControl = virXMLPropString(node, "powerControl"); if (virDomainGraphicsListensParseXML(def, node, ctxt, flags) < 0) return -1; @@ -13206,6 +13207,13 @@ virDomainGraphicsDefParseXMLVNC(virDomainGraphicsDefPtr def, } } + if (powerControl && + virStringParseYesNo(powerControl, &def->data.vnc.powerControl) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot parse vnc power control '%s'"), powerControl); + return -1; + } + def->data.vnc.keymap = virXMLPropString(node, "keymap"); if (virDomainGraphicsAuthDefParseXML(node, &def->data.vnc.auth, @@ -27148,6 +27156,10 @@ virDomainGraphicsDefFormat(virBufferPtr buf, virDomainGraphicsVNCSharePolicyTypeToString( def->data.vnc.sharePolicy)); + if (def->data.vnc.powerControl) + virBufferAsprintf(buf, " powerControl='%s'", + def->data.vnc.powerControl ? "yes" : "no"); + virDomainGraphicsAuthDefFormatAttr(buf, &def->data.vnc.auth, flags); break; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 930eed60de..544ec1b2fa 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1712,6 +1712,7 @@ struct _virDomainGraphicsDef { char *keymap; virDomainGraphicsAuthDef auth; int sharePolicy; + bool powerControl; } vnc; struct { char *display; -- 2.29.2

On Tue, Feb 16, 2021 at 14:08:50 +0000, Daniel Berrange wrote:
The <graphics type="vnc" .... powerControl="on"/> option instructs the VNC server to enable an extension that lets the client perform a graceful shutdown, reboot and hard reset.
This is enabled by default since it cannot be assumed that the VNC client user has administrator rights over the guest OS. In the case where the VNC user is a guest administrator though, it is reasonable to allow direct power control host side too.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- docs/formatdomain.rst | 5 +++++ docs/schemas/domaincommon.rng | 5 +++++ src/conf/domain_conf.c | 12 ++++++++++++ src/conf/domain_conf.h | 1 + 4 files changed, 23 insertions(+)
[...] A XML2XML test case would show that 'no' is useless in this impl, see below.
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 930eed60de..544ec1b2fa 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1712,6 +1712,7 @@ struct _virDomainGraphicsDef { char *keymap; virDomainGraphicsAuthDef auth; int sharePolicy; + bool powerControl;
This is declared as bool.
} vnc; struct { char *display;
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b731744f04..91933bf292 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c
[...]
@@ -13206,6 +13207,13 @@ virDomainGraphicsDefParseXMLVNC(virDomainGraphicsDefPtr def, } }
+ if (powerControl && + virStringParseYesNo(powerControl, &def->data.vnc.powerControl) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot parse vnc power control '%s'"), powerControl); + return -1; + } + def->data.vnc.keymap = virXMLPropString(node, "keymap");
if (virDomainGraphicsAuthDefParseXML(node, &def->data.vnc.auth, @@ -27148,6 +27156,10 @@ virDomainGraphicsDefFormat(virBufferPtr buf, virDomainGraphicsVNCSharePolicyTypeToString( def->data.vnc.sharePolicy));
+ if (def->data.vnc.powerControl) + virBufferAsprintf(buf, " powerControl='%s'", + def->data.vnc.powerControl ? "yes" : "no");
So this doesn't make much sense. You can't use 'no' since it will vanish from the XML. Did you want to use a Tristate?
+ virDomainGraphicsAuthDefFormatAttr(buf, &def->data.vnc.auth, flags); break;
-- 2.29.2

On a Tuesday in 2021, Daniel P. Berrangé wrote:
The <graphics type="vnc" .... powerControl="on"/> option instructs the VNC server to enable an extension that lets the client perform a graceful shutdown, reboot and hard reset.
This is enabled by default since it cannot be assumed that the VNC client user has administrator rights over the guest OS. In the case where the VNC user is a guest administrator though, it is reasonable to allow direct power control host side too.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- docs/formatdomain.rst | 5 +++++ docs/schemas/domaincommon.rng | 5 +++++ src/conf/domain_conf.c | 12 ++++++++++++ src/conf/domain_conf.h | 1 + 4 files changed, 23 insertions(+)
diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index eafd6b3396..580319365c 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -5791,6 +5791,11 @@ interaction with the admin. ``autoport`` having no effect due to security reasons) :since:`Since 1.0.6` .
+ For VNC, the ``powerControl`` attribute can be used to enable VM shutdown, + reboot and reset power control features for the VNC client. This is + appropriate if the authenticated VNC client user already has administrator + privileges in the guest :since:`Since 7.1.0`. + Although VNC doesn't support OpenGL natively, it can be paired with graphics type ``egl-headless`` (see below) which will instruct QEMU to open and use drm nodes for OpenGL rendering. diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index e6de934456..49dc4b5130 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -3663,6 +3663,11 @@ </choice> </attribute> </optional> + <optional> + <attribute name="powerControl"> + <ref name="virYesNo"/> + </attribute> + </optional> </group> <group> <optional> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b731744f04..91933bf292 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -13150,6 +13150,7 @@ virDomainGraphicsDefParseXMLVNC(virDomainGraphicsDefPtr def, g_autofree char *websocketGenerated = virXMLPropString(node, "websocketGenerated"); g_autofree char *sharePolicy = virXMLPropString(node, "sharePolicy"); g_autofree char *autoport = virXMLPropString(node, "autoport"); + g_autofree char *powerControl = virXMLPropString(node, "powerControl");
if (virDomainGraphicsListensParseXML(def, node, ctxt, flags) < 0) return -1; @@ -13206,6 +13207,13 @@ virDomainGraphicsDefParseXMLVNC(virDomainGraphicsDefPtr def, } }
+ if (powerControl && + virStringParseYesNo(powerControl, &def->data.vnc.powerControl) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot parse vnc power control '%s'"), powerControl); + return -1; + } + def->data.vnc.keymap = virXMLPropString(node, "keymap");
if (virDomainGraphicsAuthDefParseXML(node, &def->data.vnc.auth, @@ -27148,6 +27156,10 @@ virDomainGraphicsDefFormat(virBufferPtr buf, virDomainGraphicsVNCSharePolicyTypeToString( def->data.vnc.sharePolicy));
+ if (def->data.vnc.powerControl) + virBufferAsprintf(buf, " powerControl='%s'", + def->data.vnc.powerControl ? "yes" : "no");
The "no" branch is dead code. If you don't want to format "no" unless it was explicitly requested, use virTristateBool. Jano
+ virDomainGraphicsAuthDefFormatAttr(buf, &def->data.vnc.auth, flags); break;
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 930eed60de..544ec1b2fa 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1712,6 +1712,7 @@ struct _virDomainGraphicsDef { char *keymap; virDomainGraphicsAuthDef auth; int sharePolicy; + bool powerControl; } vnc; struct { char *display; -- 2.29.2

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml | 1 + 3 files changed, 4 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 3f8593a9e5..8fa23f14be 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -617,6 +617,7 @@ VIR_ENUM_IMPL(virQEMUCaps, "cpu-max", "memory-backend-file.x-use-canonical-path-for-ramblock-id", "vnc-opts", + "vnc-power-control", ); @@ -3297,6 +3298,7 @@ static struct virQEMUCapsCommandLineProps virQEMUCapsCommandLine[] = { { "fw_cfg", "file", QEMU_CAPS_FW_CFG }, { "fsdev", "fmode", QEMU_CAPS_FSDEV_CREATEMODE }, /* Could have also checked fsdev->dmode */ { "vnc", "display", QEMU_CAPS_VNC_OPTS }, + { "vnc", "power-control", QEMU_CAPS_VNC_POWER_CONTROL }, }; static int diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 38574eef16..e327db0148 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -597,6 +597,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */ QEMU_CAPS_CPU_MAX, /* -cpu max */ QEMU_CAPS_X_USE_CANONICAL_PATH_FOR_RAMBLOCK_ID, /* -object memory-backend-file,x-use-canonical-path-for-ramblock-id= */ QEMU_CAPS_VNC_OPTS, /* -vnc uses QemuOpts parser instead of custom code */ + QEMU_CAPS_VNC_POWER_CONTROL, /* -vnc power-control option */ QEMU_CAPS_LAST /* this must always be the last item */ } virQEMUCapsFlags; diff --git a/tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml index 23fb5b7393..3ef05ec94e 100644 --- a/tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml @@ -256,6 +256,7 @@ <flag name='cpu-max'/> <flag name='memory-backend-file.x-use-canonical-path-for-ramblock-id'/> <flag name='vnc-opts'/> + <flag name='vnc-power-control'/> <version>5002050</version> <kvmVersion>0</kvmVersion> <microcodeVersion>43100242</microcodeVersion> -- 2.29.2

On Tue, Feb 16, 2021 at 14:08:51 +0000, Daniel Berrange wrote:
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml | 1 + 3 files changed, 4 insertions(+)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>

This allows the VNC client user to perform a shutdown, reboot and reset of the VM from the host side. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/qemu/qemu_command.c | 9 +++++++++ tests/qemuxml2argvdata/graphics-vnc-policy.args | 2 +- tests/qemuxml2argvdata/graphics-vnc-policy.xml | 2 +- tests/qemuxml2argvtest.c | 2 +- 4 files changed, 12 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index d801018aa2..266cf7332e 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7700,6 +7700,15 @@ qemuBuildGraphicsVNCCommandLine(virQEMUDriverConfigPtr cfg, /* TODO: Support ACLs later */ } + if (graphics->data.vnc.powerControl) { + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VNC_POWER_CONTROL)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("VNC power control is not available")); + return -1; + } + virBufferAddLit(&opt, ",power-control=on"); + } + virCommandAddArg(cmd, "-vnc"); virCommandAddArgBuffer(cmd, &opt); if (graphics->data.vnc.keymap) diff --git a/tests/qemuxml2argvdata/graphics-vnc-policy.args b/tests/qemuxml2argvdata/graphics-vnc-policy.args index 85b7d722e0..d3a8d66161 100644 --- a/tests/qemuxml2argvdata/graphics-vnc-policy.args +++ b/tests/qemuxml2argvdata/graphics-vnc-policy.args @@ -26,5 +26,5 @@ server=on,wait=off \ -usb \ -drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \ -device ide-hd,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,bootindex=1 \ --vnc '[::]:59630,share=allow-exclusive' \ +-vnc '[::]:59630,share=allow-exclusive,power-control=on' \ -vga cirrus diff --git a/tests/qemuxml2argvdata/graphics-vnc-policy.xml b/tests/qemuxml2argvdata/graphics-vnc-policy.xml index 344f019079..4bee773438 100644 --- a/tests/qemuxml2argvdata/graphics-vnc-policy.xml +++ b/tests/qemuxml2argvdata/graphics-vnc-policy.xml @@ -25,7 +25,7 @@ <controller type='pci' index='0' model='pci-root'/> <input type='mouse' bus='ps2'/> <input type='keyboard' bus='ps2'/> - <graphics type='vnc' port='65530' autoport='no' listen='::' sharePolicy='allow-exclusive'> + <graphics type='vnc' port='65530' autoport='no' listen='::' sharePolicy='allow-exclusive' powerControl='yes'> <listen type='address' address='::'/> </graphics> <video> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index db438c5466..b4df042fea 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1364,7 +1364,7 @@ mymain(void) QEMU_CAPS_VNC, QEMU_CAPS_DEVICE_CIRRUS_VGA); DO_TEST("graphics-vnc-policy", QEMU_CAPS_VNC, - QEMU_CAPS_DEVICE_CIRRUS_VGA); + QEMU_CAPS_DEVICE_CIRRUS_VGA, QEMU_CAPS_VNC_POWER_CONTROL); DO_TEST("graphics-vnc-no-listen-attr", QEMU_CAPS_VNC, QEMU_CAPS_DEVICE_CIRRUS_VGA); DO_TEST("graphics-vnc-remove-generated-socket", QEMU_CAPS_VNC, -- 2.29.2

On Tue, Feb 16, 2021 at 14:08:52 +0000, Daniel Berrange wrote:
This allows the VNC client user to perform a shutdown, reboot and reset of the VM from the host side.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/qemu/qemu_command.c | 9 +++++++++ tests/qemuxml2argvdata/graphics-vnc-policy.args | 2 +- tests/qemuxml2argvdata/graphics-vnc-policy.xml | 2 +- tests/qemuxml2argvtest.c | 2 +- 4 files changed, 12 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index d801018aa2..266cf7332e 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7700,6 +7700,15 @@ qemuBuildGraphicsVNCCommandLine(virQEMUDriverConfigPtr cfg, /* TODO: Support ACLs later */ }
+ if (graphics->data.vnc.powerControl) { + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VNC_POWER_CONTROL)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("VNC power control is not available")); + return -1; + }
Invoking this check from qemuValidateDomainDeviceDefGraphics will give you define-time check whether the VM supports this rather than startup-time.
+ virBufferAddLit(&opt, ",power-control=on"); + }
So we don't want to be able to explicitly turn this off?
+ virCommandAddArg(cmd, "-vnc"); virCommandAddArgBuffer(cmd, &opt); if (graphics->data.vnc.keymap)
[...]
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index db438c5466..b4df042fea 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1364,7 +1364,7 @@ mymain(void) QEMU_CAPS_VNC, QEMU_CAPS_DEVICE_CIRRUS_VGA); DO_TEST("graphics-vnc-policy", QEMU_CAPS_VNC, - QEMU_CAPS_DEVICE_CIRRUS_VGA); + QEMU_CAPS_DEVICE_CIRRUS_VGA, QEMU_CAPS_VNC_POWER_CONTROL);
Rather than adding the capability, either convert the test case to DO_TEST_CAPS_LATEST or add a separate _LATEST() case for it.
DO_TEST("graphics-vnc-no-listen-attr", QEMU_CAPS_VNC, QEMU_CAPS_DEVICE_CIRRUS_VGA); DO_TEST("graphics-vnc-remove-generated-socket", QEMU_CAPS_VNC, -- 2.29.2
participants (3)
-
Daniel P. Berrangé
-
Ján Tomko
-
Peter Krempa