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

[Martin Kletzander <mkletzan@xxxxxxxxxx>]

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?

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

Attachment: signature.asc
Description: PGP signature