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

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?

>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?

>+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;

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