From: Peter Krempa <pkrempa@redhat.com>

qemuRdpAvailable() is called from the capability filing code, thus:
- it must not report spurious errors
- it should not call any extra processes

We can solve the above by just checking existance of 'qemu-rdp' in the
path as:
- at the time of adding of qemuRdpAvailable() there was only one 'qemu-rdp' release
- it supported all the features
- the check can't change as we'd drop the capability

Add comments and gut the check to only check existance of the file.

Fixes: f5e5a9bec9ec3e6c762f5000e3b8a0ba6a3a8c8d
Closes: https://gitlab.com/libvirt/libvirt/-/issues/763
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
---
 src/qemu/qemu_rdp.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/src/qemu/qemu_rdp.c b/src/qemu/qemu_rdp.c
index 984795d599..0c48b87e7b 100644
--- a/src/qemu/qemu_rdp.c
+++ b/src/qemu/qemu_rdp.c
@@ -413,12 +413,25 @@ qemuRdpSetCredentials(virDomainObj *vm,
 }


+/**
+ * qemuRdpAvailable:
+ * @helper: name (or path to) 'qemu-rdp' binary
+ *
+ * Returns whether 'qemu-rdp' is available.
+ *
+ * Important:
+ * This function is called from 'virQEMUDriverGetDomainCapabilities'. It must
+ * not report any errors and must not add any additional checks.
+ *
+ * This function is mocked from 'tests/testutilsqemu.c'
+ *
+ */
 bool
 qemuRdpAvailable(const char *helper)
 {
-    g_autoptr(qemuRdp) rdp = NULL;
-
-    rdp = qemuRdpNewForHelper(helper);
+    g_autofree char *helperPath = NULL;

-    return rdp && qemuRdpHasFeature(rdp, QEMU_RDP_FEATURE_DBUS_ADDRESS);
+    /* This function was added corresponding to the first release of 'qemu-rdp'
+     * thus checking existance of the helper binary is sufficient. */
+    return !!(helperPath = virFindFileInPath(helper));
 }
-- 
2.49.0

On 4/7/25 18:22, Peter Krempa via Devel wrote:
> From: Peter Krempa<pkrempa@redhat.com>
> 
> qemuRdpAvailable() is called from the capability filing code, thus:
> - it must not report spurious errors
> - it should not call any extra processes
> 
> We can solve the above by just checking existance of 'qemu-rdp' in the
> path as:
> - at the time of adding of qemuRdpAvailable() there was only one 'qemu-rdp' release
> - it supported all the features
> - the check can't change as we'd drop the capability
> 
> Add comments and gut the check to only check existance of the file.
> 
> Fixes: f5e5a9bec9ec3e6c762f5000e3b8a0ba6a3a8c8d
> Closes:https://gitlab.com/libvirt/libvirt/-/issues/763
> Signed-off-by: Peter Krempa<pkrempa@redhat.com>

Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com>

-- 
Mit freundlichen Grüßen/Kind regards
    Boris Fiuczynski

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Wolfgang Wendt
Geschäftsführung: David Faller
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

On Mon, Apr 7, 2025 at 8:22 PM Peter Krempa via Devel
<devel@lists.libvirt.org> wrote:
>
> From: Peter Krempa <pkrempa@redhat.com>
>
> qemuRdpAvailable() is called from the capability filing code, thus:
> - it must not report spurious errors
> - it should not call any extra processes
>
> We can solve the above by just checking existance of 'qemu-rdp' in the
> path as:
> - at the time of adding of qemuRdpAvailable() there was only one 'qemu-rdp' release
> - it supported all the features
> - the check can't change as we'd drop the capability
>
> Add comments and gut the check to only check existance of the file.
>
> Fixes: f5e5a9bec9ec3e6c762f5000e3b8a0ba6a3a8c8d
> Closes: https://gitlab.com/libvirt/libvirt/-/issues/763
> Signed-off-by: Peter Krempa <pkrempa@redhat.com>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> ---
>  src/qemu/qemu_rdp.c | 21 +++++++++++++++++----
>  1 file changed, 17 insertions(+), 4 deletions(-)
>
> diff --git a/src/qemu/qemu_rdp.c b/src/qemu/qemu_rdp.c
> index 984795d599..0c48b87e7b 100644
> --- a/src/qemu/qemu_rdp.c
> +++ b/src/qemu/qemu_rdp.c
> @@ -413,12 +413,25 @@ qemuRdpSetCredentials(virDomainObj *vm,
>  }
>
>
> +/**
> + * qemuRdpAvailable:
> + * @helper: name (or path to) 'qemu-rdp' binary
> + *
> + * Returns whether 'qemu-rdp' is available.
> + *
> + * Important:
> + * This function is called from 'virQEMUDriverGetDomainCapabilities'. It must
> + * not report any errors and must not add any additional checks.
> + *
> + * This function is mocked from 'tests/testutilsqemu.c'
> + *
> + */
>  bool
>  qemuRdpAvailable(const char *helper)
>  {
> -    g_autoptr(qemuRdp) rdp = NULL;
> -
> -    rdp = qemuRdpNewForHelper(helper);
> +    g_autofree char *helperPath = NULL;
>
> -    return rdp && qemuRdpHasFeature(rdp, QEMU_RDP_FEATURE_DBUS_ADDRESS);
> +    /* This function was added corresponding to the first release of 'qemu-rdp'
> +     * thus checking existance of the helper binary is sufficient. */
> +    return !!(helperPath = virFindFileInPath(helper));
>  }
> --
> 2.49.0



-- 
Marc-André Lureau