From: Marc-André Lureau <marcandre.lureau@redhat.com>
Wire the external server RDP support with QEMU.
Check the configuration, allocate a port, start the process
and set the credentials.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
docs/formatdomain.rst | 25 ++++--
src/conf/domain_conf.h | 1 +
src/qemu/qemu_extdevice.c | 46 +++++++++--
src/qemu/qemu_hotplug.c | 49 ++++++++++-
src/qemu/qemu_hotplug.h | 1 +
src/qemu/qemu_process.c | 167 ++++++++++++++++++++++++++++++++++----
6 files changed, 257 insertions(+), 32 deletions(-)
diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
index 785b51bb4e..358feb2625 100644
--- a/docs/formatdomain.rst
+++ b/docs/formatdomain.rst
@@ -6410,7 +6410,7 @@ interaction with the admin.
<graphics type='vnc' port='5904' sharePolicy='allow-exclusive'>
<listen type='address' address='1.2.3.4'/>
</graphics>
- <graphics type='rdp' autoport='yes' multiUser='yes' />
+ <graphics type='rdp' autoport='yes' multiUser='yes'/>
<graphics type='desktop' fullscreen='yes'/>
<graphics type='spice'>
<listen type='network' network='rednet'/>
@@ -6573,14 +6573,21 @@ interaction with the admin.
Starts a RDP server. The ``port`` attribute specifies the TCP port number
(with -1 as legacy syntax indicating that it should be auto-allocated).
The ``autoport`` attribute is the new preferred syntax for indicating
- auto-allocation of the TCP port to use. In the VirtualBox driver, the
- ``autoport`` will make the hypervisor pick available port from 3389-3689
- range when the VM is started. The chosen port will be reflected in the
- ``port`` attribute. The ``multiUser`` attribute is a boolean deciding
- whether multiple simultaneous connections to the VM are permitted. The
- ``replaceUser`` attribute is a boolean deciding whether the existing
- connection must be dropped and a new connection must be established by the
- VRDP server, when a new client connects in single connection mode.
+ auto-allocation of the TCP port to use.
+
+ A ``dbus`` graphics is also required to enable the QEMU RDP support, which
+ uses an external "qemu-rdp" helper process. The ``username`` and
+ ``passwd`` attributes set the credentials (when they are not set, the RDP
+ access may be disabled by the helper). :since:`Since 11.1.0`
+
+ In the VirtualBox driver, the ``autoport`` will make the hypervisor pick
+ available port from 3389-3689 range when the VM is started. The chosen
+ port will be reflected in the ``port`` attribute. The ``multiUser``
+ attribute is a boolean deciding whether multiple simultaneous connections
+ to the VM are permitted. The ``replaceUser`` attribute is a boolean
+ deciding whether the existing connection must be dropped and a new
+ connection must be established by the VRDP server, when a new client
+ connects in single connection mode.
``desktop``
This value is reserved for VirtualBox domains for the moment. It displays
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index b08e2549b7..ff05920030 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2025,6 +2025,7 @@ struct _virDomainGraphicsDef {
} sdl;
struct {
int port;
+ bool portReserved;
bool autoport;
bool replaceUser;
bool multiUser;
diff --git a/src/qemu/qemu_extdevice.c b/src/qemu/qemu_extdevice.c
index 954cb323a4..e0c9cd1d91 100644
--- a/src/qemu/qemu_extdevice.c
+++ b/src/qemu/qemu_extdevice.c
@@ -236,14 +236,28 @@ qemuExtDevicesStart(virQEMUDriver *driver,
for (i = 0; i < def->ngraphics; i++) {
virDomainGraphicsDef *graphics = def->graphics[i];
- if (graphics->type != VIR_DOMAIN_GRAPHICS_TYPE_DBUS)
+ switch (graphics->type) {
+ case VIR_DOMAIN_GRAPHICS_TYPE_DBUS: {
+ if (graphics->data.dbus.p2p || graphics->data.dbus.fromConfig)
+ continue;
+ if (qemuDBusStart(driver, vm) < 0)
+ return -1;
continue;
-
- if (graphics->data.dbus.p2p || graphics->data.dbus.fromConfig)
+ }
+ case VIR_DOMAIN_GRAPHICS_TYPE_RDP: {
+ if (qemuRdpStart(vm, graphics) < 0)
+ return -1;
continue;
-
- if (qemuDBusStart(driver, vm) < 0)
- return -1;
+ }
+ case VIR_DOMAIN_GRAPHICS_TYPE_SDL:
+ case VIR_DOMAIN_GRAPHICS_TYPE_VNC:
+ case VIR_DOMAIN_GRAPHICS_TYPE_DESKTOP:
+ case VIR_DOMAIN_GRAPHICS_TYPE_SPICE:
+ case VIR_DOMAIN_GRAPHICS_TYPE_EGL_HEADLESS:
+ case VIR_DOMAIN_GRAPHICS_TYPE_LAST:
+ default:
+ continue;
+ }
}
for (i = 0; i < def->ndisks; i++) {
@@ -309,6 +323,26 @@ qemuExtDevicesStop(virQEMUDriver *driver,
qemuVirtioFSStop(driver, vm, fs);
}
+ for (i = 0; i < def->ngraphics; i++) {
+ virDomainGraphicsDef *graphics = def->graphics[i];
+
+ switch (graphics->type) {
+ case VIR_DOMAIN_GRAPHICS_TYPE_RDP: {
+ qemuRdpStop(vm, graphics);
+ continue;
+ }
+ case VIR_DOMAIN_GRAPHICS_TYPE_DBUS:
+ case VIR_DOMAIN_GRAPHICS_TYPE_SDL:
+ case VIR_DOMAIN_GRAPHICS_TYPE_VNC:
+ case VIR_DOMAIN_GRAPHICS_TYPE_DESKTOP:
+ case VIR_DOMAIN_GRAPHICS_TYPE_SPICE:
+ case VIR_DOMAIN_GRAPHICS_TYPE_EGL_HEADLESS:
+ case VIR_DOMAIN_GRAPHICS_TYPE_LAST:
+ default:
+ continue;
+ }
+ }
+
for (i = 0; i < def->ndisks; i++) {
virDomainDiskDef *disk = def->disks[i];
qemuNbdkitStopStorageSource(disk->src, vm, true);
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 5a7e6c3b12..38d21642fe 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -4410,6 +4410,7 @@ int
qemuDomainChangeGraphicsPasswords(virDomainObj *vm,
int type,
virDomainGraphicsAuthDef *auth,
+ const char *defaultUsername,
const char *defaultPasswd,
int asyncJob)
{
@@ -4419,13 +4420,19 @@ qemuDomainChangeGraphicsPasswords(virDomainObj *vm,
g_autofree char *validTo = NULL;
const char *connected = NULL;
const char *password;
+ const char *username;
int ret = -1;
if (!auth->passwd && !defaultPasswd)
return 0;
+ username = auth->username ? auth->username : defaultUsername;
password = auth->passwd ? auth->passwd : defaultPasswd;
+ if (type == VIR_DOMAIN_GRAPHICS_TYPE_RDP) {
+ return qemuRdpSetCredentials(vm, username, password, "");
+ }
+
if (auth->connected)
connected = virDomainGraphicsAuthConnectedTypeToString(auth->connected);
@@ -4555,6 +4562,7 @@ qemuDomainChangeGraphics(virQEMUDriver *driver,
if (qemuDomainChangeGraphicsPasswords(vm,
VIR_DOMAIN_GRAPHICS_TYPE_VNC,
&dev->data.vnc.auth,
+ NULL,
cfg->vncPassword,
VIR_ASYNC_JOB_NONE) < 0)
return -1;
@@ -4602,6 +4610,7 @@ qemuDomainChangeGraphics(virQEMUDriver *driver,
if (qemuDomainChangeGraphicsPasswords(vm,
VIR_DOMAIN_GRAPHICS_TYPE_SPICE,
&dev->data.spice.auth,
+ NULL,
cfg->spicePassword,
VIR_ASYNC_JOB_NONE) < 0)
return -1;
@@ -4617,8 +4626,46 @@ qemuDomainChangeGraphics(virQEMUDriver *driver,
}
break;
- case VIR_DOMAIN_GRAPHICS_TYPE_SDL:
case VIR_DOMAIN_GRAPHICS_TYPE_RDP:
+ if ((olddev->data.rdp.autoport != dev->data.rdp.autoport) ||
+ (!dev->data.rdp.autoport &&
+ (olddev->data.rdp.port != dev->data.rdp.port))) {
+ virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+ _("cannot change port settings on rdp graphics"));
+ return -1;
+ }
+
+ /* If a password lifetime was, or is set, or action if connected has
+ * changed, then we must always run, even if new password matches
+ * old password */
+ if (olddev->data.rdp.auth.expires ||
+ dev->data.rdp.auth.expires ||
+ olddev->data.rdp.auth.connected != dev->data.rdp.auth.connected ||
+ STRNEQ_NULLABLE(olddev->data.rdp.auth.username,
+ dev->data.rdp.auth.username) ||
+ STRNEQ_NULLABLE(olddev->data.rdp.auth.passwd,
+ dev->data.rdp.auth.passwd)) {
+ VIR_DEBUG("Updating password on RDP server %p %p",
+ dev->data.rdp.auth.passwd, cfg->rdpPassword);
+ if (qemuDomainChangeGraphicsPasswords(vm,
+ VIR_DOMAIN_GRAPHICS_TYPE_RDP,
+ &dev->data.rdp.auth,
+ cfg->rdpUsername,
+ cfg->rdpPassword,
+ VIR_ASYNC_JOB_NONE) < 0)
+ return -1;
+
+ /* Steal the new dev's char * reference */
+ VIR_FREE(olddev->data.rdp.auth.username);
+ olddev->data.rdp.auth.username = g_steal_pointer(&dev->data.rdp.auth.username);
+ VIR_FREE(olddev->data.rdp.auth.passwd);
+ olddev->data.rdp.auth.passwd = g_steal_pointer(&dev->data.rdp.auth.passwd);
+ olddev->data.rdp.auth.validTo = dev->data.rdp.auth.validTo;
+ olddev->data.rdp.auth.expires = dev->data.rdp.auth.expires;
+ olddev->data.rdp.auth.connected = dev->data.rdp.auth.connected;
+ }
+ break;
+ case VIR_DOMAIN_GRAPHICS_TYPE_SDL:
case VIR_DOMAIN_GRAPHICS_TYPE_DESKTOP:
case VIR_DOMAIN_GRAPHICS_TYPE_EGL_HEADLESS:
case VIR_DOMAIN_GRAPHICS_TYPE_DBUS:
diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h
index 4fe7f4923e..8108d96de9 100644
--- a/src/qemu/qemu_hotplug.h
+++ b/src/qemu/qemu_hotplug.h
@@ -51,6 +51,7 @@ int qemuDomainFindGraphicsIndex(virDomainDef *def,
int qemuDomainChangeGraphicsPasswords(virDomainObj *vm,
int type,
virDomainGraphicsAuthDef *auth,
+ const char *defaultUsername,
const char *defaultPasswd,
int asyncJob);
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 34a755a49a..c0dd9c0b35 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -2949,18 +2949,29 @@ qemuProcessInitPasswords(virQEMUDriver *driver,
for (i = 0; i < vm->def->ngraphics; ++i) {
virDomainGraphicsDef *graphics = vm->def->graphics[i];
- if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC) {
- ret = qemuDomainChangeGraphicsPasswords(vm,
- VIR_DOMAIN_GRAPHICS_TYPE_VNC,
- &graphics->data.vnc.auth,
- cfg->vncPassword,
- asyncJob);
- } else if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE) {
- ret = qemuDomainChangeGraphicsPasswords(vm,
- VIR_DOMAIN_GRAPHICS_TYPE_SPICE,
- &graphics->data.spice.auth,
- cfg->spicePassword,
- asyncJob);
+
+ switch (graphics->type) {
+ case VIR_DOMAIN_GRAPHICS_TYPE_VNC:
+ ret = qemuDomainChangeGraphicsPasswords(
+ vm, VIR_DOMAIN_GRAPHICS_TYPE_VNC, &graphics->data.vnc.auth, NULL,
+ cfg->vncPassword, asyncJob);
+ break;
+ case VIR_DOMAIN_GRAPHICS_TYPE_SPICE:
+ ret = qemuDomainChangeGraphicsPasswords(
+ vm, VIR_DOMAIN_GRAPHICS_TYPE_SPICE, &graphics->data.spice.auth,
+ NULL, cfg->spicePassword, asyncJob);
+ break;
+ case VIR_DOMAIN_GRAPHICS_TYPE_RDP:
+ ret = qemuDomainChangeGraphicsPasswords(
+ vm, VIR_DOMAIN_GRAPHICS_TYPE_RDP, &graphics->data.rdp.auth,
+ cfg->rdpUsername, cfg->rdpPassword, asyncJob);
+ break;
+ case VIR_DOMAIN_GRAPHICS_TYPE_SDL:
+ case VIR_DOMAIN_GRAPHICS_TYPE_DESKTOP:
+ case VIR_DOMAIN_GRAPHICS_TYPE_EGL_HEADLESS:
+ case VIR_DOMAIN_GRAPHICS_TYPE_DBUS:
+ case VIR_DOMAIN_GRAPHICS_TYPE_LAST:
+ break;
}
if (ret < 0)
@@ -4239,6 +4250,30 @@ qemuProcessSPICEAllocatePorts(virQEMUDriver *driver,
return 0;
}
+static int
+qemuProcessRDPAllocatePorts(virQEMUDriver *driver,
+ virDomainGraphicsDef *graphics,
+ bool allocate)
+{
+ unsigned short port;
+
+ if (!allocate) {
+ if (graphics->data.rdp.autoport)
+ graphics->data.rdp.port = 3389;
+
+ return 0;
+ }
+
+ if (graphics->data.rdp.autoport) {
+ if (virPortAllocatorAcquire(driver->rdpPorts, &port) < 0)
+ return -1;
+ graphics->data.rdp.port = port;
+ graphics->data.rdp.portReserved = true;
+ }
+
+ return 0;
+}
+
static int
qemuProcessVerifyHypervFeatures(virDomainDef *def,
@@ -4943,8 +4978,16 @@ qemuProcessGraphicsReservePorts(virDomainGraphicsDef *graphics,
}
break;
- case VIR_DOMAIN_GRAPHICS_TYPE_SDL:
case VIR_DOMAIN_GRAPHICS_TYPE_RDP:
+ if (!graphics->data.rdp.autoport ||
+ reconnect) {
+ if (virPortAllocatorSetUsed(graphics->data.rdp.port) < 0)
+ return -1;
+ graphics->data.rdp.portReserved = true;
+ }
+ break;
+
+ case VIR_DOMAIN_GRAPHICS_TYPE_SDL:
case VIR_DOMAIN_GRAPHICS_TYPE_DESKTOP:
case VIR_DOMAIN_GRAPHICS_TYPE_EGL_HEADLESS:
case VIR_DOMAIN_GRAPHICS_TYPE_DBUS:
@@ -4983,8 +5026,12 @@ qemuProcessGraphicsAllocatePorts(virQEMUDriver *driver,
return -1;
break;
- case VIR_DOMAIN_GRAPHICS_TYPE_SDL:
case VIR_DOMAIN_GRAPHICS_TYPE_RDP:
+ if (qemuProcessRDPAllocatePorts(driver, graphics, allocate) < 0)
+ return -1;
+ break;
+
+ case VIR_DOMAIN_GRAPHICS_TYPE_SDL:
case VIR_DOMAIN_GRAPHICS_TYPE_DESKTOP:
case VIR_DOMAIN_GRAPHICS_TYPE_EGL_HEADLESS:
case VIR_DOMAIN_GRAPHICS_TYPE_DBUS:
@@ -5151,8 +5198,11 @@ qemuProcessGraphicsSetupListen(virQEMUDriver *driver,
listenAddr = cfg->spiceListen;
break;
- case VIR_DOMAIN_GRAPHICS_TYPE_SDL:
case VIR_DOMAIN_GRAPHICS_TYPE_RDP:
+ listenAddr = cfg->rdpListen;
+ break;
+
+ case VIR_DOMAIN_GRAPHICS_TYPE_SDL:
case VIR_DOMAIN_GRAPHICS_TYPE_DESKTOP:
case VIR_DOMAIN_GRAPHICS_TYPE_EGL_HEADLESS:
case VIR_DOMAIN_GRAPHICS_TYPE_DBUS:
@@ -5435,6 +5485,23 @@ qemuProcessStartWarnShmem(virDomainObj *vm)
}
+static bool
+virDomainDefHasDBus(const virDomainDef *def, bool p2p)
+{
+ size_t i = 0;
+
+ for (i = 0; i < def->ngraphics; i++) {
+ virDomainGraphicsDef *graphics = def->graphics[i];
+
+ if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_DBUS) {
+ return graphics->data.dbus.p2p == p2p;
+ }
+ }
+
+ return false;
+}
+
+
static int
qemuProcessStartValidateGraphics(virDomainObj *vm)
{
@@ -5453,8 +5520,30 @@ qemuProcessStartValidateGraphics(virDomainObj *vm)
}
break;
- case VIR_DOMAIN_GRAPHICS_TYPE_SDL:
case VIR_DOMAIN_GRAPHICS_TYPE_RDP:
+ if (graphics->nListens > 1) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("qemu-rdp does not support multiple listens for one graphics device."));
+ return -1;
+ }
+ if (graphics->data.rdp.multiUser) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("qemu-rdp doesn't support the 'multiUser' attribute."));
+ return -1;
+ }
+ if (graphics->data.rdp.replaceUser) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("qemu-rdp doesn't support the 'replaceUser' attribute."));
+ return -1;
+ }
+ if (!virDomainDefHasDBus(vm->def, false)) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("qemu-rdp support requires a D-Bus bus graphics device."));
+ return -1;
+ }
+ break;
+
+ case VIR_DOMAIN_GRAPHICS_TYPE_SDL:
case VIR_DOMAIN_GRAPHICS_TYPE_DESKTOP:
case VIR_DOMAIN_GRAPHICS_TYPE_EGL_HEADLESS:
case VIR_DOMAIN_GRAPHICS_TYPE_DBUS:
@@ -5939,6 +6028,42 @@ qemuProcessPrepareHostNetwork(virDomainObj *vm)
return 0;
}
+#include "qemu_rdp.h"
+
+static int
+qemuPrepareGraphicsRdp(virQEMUDriver *driver,
+ virDomainGraphicsDef *gfx)
+{
+ g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
+ g_autoptr(qemuRdp) rdp = NULL;
+
+ if (!(rdp = qemuRdpNewForHelper(cfg->qemuRdpName)))
+ return -1;
+
+ QEMU_DOMAIN_GRAPHICS_PRIVATE(gfx)->rdp = g_steal_pointer(&rdp);
+
+ return 0;
+}
+
+
+static int
+qemuProcessPrepareGraphics(virDomainObj *vm)
+{
+ qemuDomainObjPrivate *priv = vm->privateData;
+ size_t i;
+
+ for (i = 0; i < vm->def->ngraphics; i++) {
+ virDomainGraphicsDef *gfx = vm->def->graphics[i];
+
+ if (gfx->type == VIR_DOMAIN_GRAPHICS_TYPE_RDP &&
+ qemuPrepareGraphicsRdp(priv->driver, gfx) < 0)
+ return -1;
+
+ }
+
+ return 0;
+}
+
struct qemuProcessSetupVcpuSchedCoreHelperData {
pid_t vcpupid;
@@ -7410,6 +7535,10 @@ qemuProcessPrepareHost(virQEMUDriver *driver,
if (qemuProcessPrepareHostNetwork(vm) < 0)
return -1;
+ VIR_DEBUG("Preparing graphics");
+ if (qemuProcessPrepareGraphics(vm) < 0)
+ return -1;
+
/* Must be run before security labelling */
VIR_DEBUG("Preparing host devices");
if (!cfg->relaxedACS)
@@ -8982,6 +9111,12 @@ void qemuProcessStop(virQEMUDriver *driver,
graphics->data.spice.tlsPortReserved = false;
}
}
+ if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_RDP) {
+ if (graphics->data.rdp.portReserved) {
+ virPortAllocatorRelease(graphics->data.rdp.port);
+ graphics->data.rdp.portReserved = false;
+ }
+ }
}
for (i = 0; i < vm->ndeprecations; i++)
--
2.47.0On Wed, Jan 29, 2025 at 05:40:40PM +0400, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Wire the external server RDP support with QEMU.
>
> Check the configuration, allocate a port, start the process
> and set the credentials.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> docs/formatdomain.rst | 25 ++++--
> src/conf/domain_conf.h | 1 +
> src/qemu/qemu_extdevice.c | 46 +++++++++--
> src/qemu/qemu_hotplug.c | 49 ++++++++++-
> src/qemu/qemu_hotplug.h | 1 +
> src/qemu/qemu_process.c | 167 ++++++++++++++++++++++++++++++++++----
> 6 files changed, 257 insertions(+), 32 deletions(-)
> + if (!virDomainDefHasDBus(vm->def, false)) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("qemu-rdp support requires a D-Bus bus graphics device."));
> + return -1;
> + }
IMHO this is not the correct way to model it.
Use of DBus is an internal impl detail of the RDP backend that is
being configured. Users shouldn't have to know that - they should
just be able to request 'RDP' alone for the <graphics> type.
Consider if we wanted to provide a non-integrated VNC server for
QEMU. We wouldn't do that by requiring use of a <graphics type=dbus>
alongside the <graphics type=vnc>, as that config has the distinct
meaning of requiring VNC and DBus in parallel.
Where we have a choice of underlying implementations, we would
typically have something like a <backend type="qemu|dbus"/>
as a child element to select between them.
With 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 :|
Hi
On Wed, Jan 29, 2025 at 7:25 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Wed, Jan 29, 2025 at 05:40:40PM +0400, marcandre.lureau@redhat.com wrote:
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > Wire the external server RDP support with QEMU.
> >
> > Check the configuration, allocate a port, start the process
> > and set the credentials.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> > docs/formatdomain.rst | 25 ++++--
> > src/conf/domain_conf.h | 1 +
> > src/qemu/qemu_extdevice.c | 46 +++++++++--
> > src/qemu/qemu_hotplug.c | 49 ++++++++++-
> > src/qemu/qemu_hotplug.h | 1 +
> > src/qemu/qemu_process.c | 167 ++++++++++++++++++++++++++++++++++----
> > 6 files changed, 257 insertions(+), 32 deletions(-)
>
>
> > + if (!virDomainDefHasDBus(vm->def, false)) {
> > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> > + _("qemu-rdp support requires a D-Bus bus graphics device."));
> > + return -1;
> > + }
>
> IMHO this is not the correct way to model it.
>
> Use of DBus is an internal impl detail of the RDP backend that is
> being configured. Users shouldn't have to know that - they should
> just be able to request 'RDP' alone for the <graphics> type.
>
> Consider if we wanted to provide a non-integrated VNC server for
> QEMU. We wouldn't do that by requiring use of a <graphics type=dbus>
> alongside the <graphics type=vnc>, as that config has the distinct
> meaning of requiring VNC and DBus in parallel.
>
> Where we have a choice of underlying implementations, we would
> typically have something like a <backend type="qemu|dbus"/>
> as a child element to select between them.
I wish it would be an implementation detail. Unfortunately, the qemu
devices needs to be configured to export themself on dbus too
(chardev/usb/audio), this is also exposed on the domain XML. For the
most simple case, ie display only, we could probably hide the dbus
implementation detail. But in general, we probably better be explicit
about it. Being explicit would also avoid surprises when checking the
list of available displays (think domdisplay --all, or by inspecting
the bus). I am not sure it's worth trying to hide it (for ex, should
<redirdev bus='usb'> be exposed on dbus by default when dbus or rdp is
enabled?). If people feel it's really best, I can give it a try, but I
fear it's going to be more struggle and surprises without much
benefit.
On Thu, Jan 30, 2025 at 04:42:24PM +0400, Marc-André Lureau wrote:
> Hi
>
> On Wed, Jan 29, 2025 at 7:25 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
> >
> > On Wed, Jan 29, 2025 at 05:40:40PM +0400, marcandre.lureau@redhat.com wrote:
> > > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> > >
> > > Wire the external server RDP support with QEMU.
> > >
> > > Check the configuration, allocate a port, start the process
> > > and set the credentials.
> > >
> > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > ---
> > > docs/formatdomain.rst | 25 ++++--
> > > src/conf/domain_conf.h | 1 +
> > > src/qemu/qemu_extdevice.c | 46 +++++++++--
> > > src/qemu/qemu_hotplug.c | 49 ++++++++++-
> > > src/qemu/qemu_hotplug.h | 1 +
> > > src/qemu/qemu_process.c | 167 ++++++++++++++++++++++++++++++++++----
> > > 6 files changed, 257 insertions(+), 32 deletions(-)
> >
> >
> > > + if (!virDomainDefHasDBus(vm->def, false)) {
> > > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> > > + _("qemu-rdp support requires a D-Bus bus graphics device."));
> > > + return -1;
> > > + }
> >
> > IMHO this is not the correct way to model it.
> >
> > Use of DBus is an internal impl detail of the RDP backend that is
> > being configured. Users shouldn't have to know that - they should
> > just be able to request 'RDP' alone for the <graphics> type.
> >
> > Consider if we wanted to provide a non-integrated VNC server for
> > QEMU. We wouldn't do that by requiring use of a <graphics type=dbus>
> > alongside the <graphics type=vnc>, as that config has the distinct
> > meaning of requiring VNC and DBus in parallel.
> >
> > Where we have a choice of underlying implementations, we would
> > typically have something like a <backend type="qemu|dbus"/>
> > as a child element to select between them.
>
> I wish it would be an implementation detail. Unfortunately, the qemu
> devices needs to be configured to export themself on dbus too
> (chardev/usb/audio), this is also exposed on the domain XML. For the
> most simple case, ie display only, we could probably hide the dbus
> implementation detail. But in general, we probably better be explicit
> about it. Being explicit would also avoid surprises when checking the
> list of available displays (think domdisplay --all, or by inspecting
> the bus). I am not sure it's worth trying to hide it (for ex, should
> <redirdev bus='usb'> be exposed on dbus by default when dbus or rdp is
> enabled?). If people feel it's really best, I can give it a try, but I
> fear it's going to be more struggle and surprises without much
> benefit.
Hmm, yeah, that's awkward really, so guess we can't do much better than
having dual <graphics> tags :-(
With 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 :|
© 2016 - 2025 Red Hat, Inc.