On Thu, Mar 13, 2025 at 12:18:04AM +0400, Marc-André Lureau wrote:
Hi On Fri, Mar 7, 2025 at 7:03 PM Martin Kletzander <mkletzan@xxxxxxxxxx> wrote:On Tue, Feb 18, 2025 at 02:16:25PM +0400, marcandre.lureau@xxxxxxxxxx wrote: >From: Marc-André Lureau <marcandre.lureau@xxxxxxxxxx> > >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@xxxxxxxxxx> >--- > 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) ?
We prefer setting an VIR_ERR_INTERNAL_ERROR instead of aborting the whole daemon.
>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
It doesn't, true.
>+ >+ if (!(rdp = qemuRdpNewForHelper(cfg->qemuRdpName))) >+ return -1; >+ >+ QEMU_DOMAIN_GRAPHICS_PRIVATE(gfx)->rdp = g_steal_pointer(&rdp); >+ >+ return 0; >+}
Attachment:
signature.asc
Description: PGP signature
