[libvirt PATCH 1/3] conf: add support for VNC power control setting

Daniel P. Berrangé posted 3 patches 4 years, 11 months ago
[libvirt PATCH 1/3] conf: add support for VNC power control setting
Posted by Daniel P. Berrangé 4 years, 11 months ago
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

Re: [libvirt PATCH 1/3] conf: add support for VNC power control setting
Posted by Ján Tomko 4 years, 11 months ago
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
>
Re: [libvirt PATCH 1/3] conf: add support for VNC power control setting
Posted by Peter Krempa 4 years, 11 months ago
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
>