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) ?
>
> >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;
> >+}
