Re: [PATCH v2 20/21] qemu: add RDP support

Hi On Fri, Mar 7, 2025 at 7:03 PM Martin Kletzander <mkletzan(a)redhat.com&gt; wrote: > > On Tue, Feb 18, 2025 at 02:16:25PM +0400, marcandre.lureau(a)redhat.com wrote: > >From: Marc-André Lureau <marcandre.lureau(a)redhat.com&gt; > > > >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(a)redhat.com&gt; > >--- > > 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/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c > >index 28ca321c5c..5dae70cf7f 100644 > >--- a/src/qemu/qemu_hotplug.c > >+++ b/src/qemu/qemu_hotplug.c > >@@ -4423,6 +4423,7 @@ int > > qemuDomainChangeGraphicsPasswords(virDomainObj *vm, > > int type, > > virDomainGraphicsAuthDef *auth, > >+ const char *defaultUsername, > > const char *defaultPasswd, > > int asyncJob) > > { > >@@ -4432,13 +4433,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, ""); > >+ } > >+ > > It's not immediately visible that defaultPasswd must not be NULL if the > graphics type is RDP. The patch is semantically fine, but I worry about > the future, would you mind adding a simple check here so that we do not > end up calling g_variant_new() with NULL? assert(password != NULL) ?

> > >diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c > >index 30d6560d52..53572f7315 100644 > >--- a/src/qemu/qemu_process.c > >+++ b/src/qemu/qemu_process.c > >@@ -5988,6 +6077,42 @@ qemuProcessPrepareHostNetwork(virDomainObj *vm) > > return 0; > > } > > > >+#include "qemu_rdp.h" > >+ > > Any reason why you cannot put the include at the top with the other ones? oops, that's a left over, thanks! > > >+static int > >+qemuPrepareGraphicsRdp(virQEMUDriver *driver, > >+ virDomainGraphicsDef *gfx) > >+{ > >+ g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); > >+ g_autoptr(qemuRdp) rdp = NULL; > > No need for autoptr variable here, there is no place where this function > might fail with this variable set to non-NULL; yeah, I suppose the code evolved. It doesn't hurt though. ok

> > >+ > >+ if (!(rdp = qemuRdpNewForHelper(cfg->qemuRdpName))) > >+ return -1; > >+ > >+ QEMU_DOMAIN_GRAPHICS_PRIVATE(gfx)->rdp = g_steal_pointer(&rdp); > >+ > >+ return 0; > >+}