Re: [PATCH v2 17/21] qemu: add qemu-rdp helper unit

[Marc-André Lureau <marcandre.lureau@xxxxxxxxxx>]

Hi

On Fri, Mar 7, 2025 at 6:40 PM Martin Kletzander <mkletzan@xxxxxxxxxx> wrote:
>
> On Tue, Feb 18, 2025 at 02:16:22PM +0400, marcandre.lureau@xxxxxxxxxx wrote:
> >From: Marc-André Lureau <marcandre.lureau@xxxxxxxxxx>
> >
> >Helpers to start the qemu-rdp server and set it up.
> >
> >Signed-off-by: Marc-André Lureau <marcandre.lureau@xxxxxxxxxx>
> >---
> > po/POTFILES            |   1 +
> > src/qemu/meson.build   |   1 +
> > src/qemu/qemu_domain.c |   1 +
> > src/qemu/qemu_domain.h |   2 +
> > src/qemu/qemu_rdp.c    | 405 +++++++++++++++++++++++++++++++++++++++++
> > src/qemu/qemu_rdp.h    |  71 ++++++++
> > 6 files changed, 481 insertions(+)
> > create mode 100644 src/qemu/qemu_rdp.c
> > create mode 100644 src/qemu/qemu_rdp.h
> >
> >diff --git a/src/qemu/qemu_rdp.c b/src/qemu/qemu_rdp.c
> >new file mode 100644
> >index 0000000000..e881b74ee3
> >--- /dev/null
> >+++ b/src/qemu/qemu_rdp.c
> >@@ -0,0 +1,405 @@
> >+/*
> >+ * qemu_rdp.c: QEMU Rdp support
> >+ *
> >+ * This library is free software; you can redistribute it and/or
> >+ * modify it under the terms of the GNU Lesser General Public
> >+ * License as published by the Free Software Foundation; either
> >+ * version 2.1 of the License, or (at your option) any later version.
> >+ *
> >+ * This library is distributed in the hope that it will be useful,
> >+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
> >+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> >+ * Lesser General Public License for more details.
> >+ *
> >+ * You should have received a copy of the GNU Lesser General Public
> >+ * License along with this library.  If not, see
> >+ * <http://www.gnu.org/licenses/>.
> >+ */
> >+
> >+#include <config.h>
> >+
> >+#include <gio/gio.h>
> >+
> >+#include "qemu_dbus.h"
> >+#include "qemu_extdevice.h"
> >+#include "qemu_security.h"
> >+#include "qemu_rdp.h"
> >+#include "virenum.h"
> >+#include "virerror.h"
> >+#include "virjson.h"
> >+#include "virlog.h"
> >+#include "virpidfile.h"
> >+#include "virutil.h"
> >+#include "virgdbus.h"
> >+
> >+#define VIR_FROM_THIS VIR_FROM_NONE
> >+
> >+VIR_LOG_INIT("qemu.rdp");
> >+
> >+VIR_ENUM_IMPL(qemuRdpFeature,
> >+              QEMU_RDP_FEATURE_LAST,
> >+              "",
> >+              "dbus-address",
> >+              "remotefx"
> >+);
> >+
> >+#define ORG_QEMUDISPLAY_RDP "org.QemuDisplay.RDP"
> >+#define ORG_QEMUDISPLAY_RDP_PATH "/org/qemu_display/rdp"
> >+#define ORG_QEMUDISPLAY_RDP_IFACE "org.QemuDisplay.RDP"
> >+
> >+
> >+void
> >+qemuRdpFree(qemuRdp *rdp)
> >+{
> >+    if (!rdp)
> >+        return;
> >+
> >+    virBitmapFree(rdp->features);
> >+    g_free(rdp);
> >+}
> >+
> >+
> >+void
> >+qemuRdpSetFeature(qemuRdp *rdp,
> >+                  qemuRdpFeature feature)
> >+{
> >+    ignore_value(virBitmapSetBit(rdp->features, feature));
> >+}
> >+
> >+
> >+bool
> >+qemuRdpHasFeature(const qemuRdp *rdp,
> >+                  qemuRdpFeature feature)
> >+{
> >+    return virBitmapIsBitSet(rdp->features, feature);
> >+}
> >+
> >+
> >+qemuRdp *
> >+qemuRdpNew(void)
> >+{
> >+    g_autoptr(qemuRdp) rdp = g_new0(qemuRdp, 1);
> >+
> >+    rdp->features = virBitmapNew(QEMU_RDP_FEATURE_LAST);
> >+    rdp->pid = -1;
> >+
> >+    return g_steal_pointer(&rdp);
> >+}
> >+
> >+
> >+qemuRdp *
> >+qemuRdpNewForHelper(const char *helper)
> >+{
> >+    g_autoptr(qemuRdp) rdp = NULL;
> >+    g_autoptr(virCommand) cmd = NULL;
> >+    g_autofree char *output = NULL;
> >+    g_autoptr(virJSONValue) doc = NULL;
> >+    virJSONValue *featuresJSON;
> >+    g_autofree char *helperPath = NULL;
> >+    size_t i, nfeatures;
> >+
> >+    helperPath = virFindFileInPath(helper);
> >+    if (!helperPath) {
> >+        virReportSystemError(errno,
> >+                             _("'%1$s' is not a suitable qemu-rdp helper name"),
> >+                             helper);
> >+        return NULL;
> >+    }
> >+
> >+    rdp = qemuRdpNew();
> >+    cmd = virCommandNewArgList(helperPath, "--print-capabilities", NULL);
> >+    virCommandSetOutputBuffer(cmd, &output);
> >+    if (virCommandRun(cmd, NULL) < 0)
> >+        return NULL;
> >+
> >+    if (!(doc = virJSONValueFromString(output)) ||
> >+        !(featuresJSON = virJSONValueObjectGetArray(doc, "features"))) {
> >+        virReportError(VIR_ERR_INTERNAL_ERROR,
> >+                       _("unable to parse json capabilities '%1$s'"),
>
> It feels to me like this needs a "from" or "for" in order to
> disambiguate the meaning of the string, just:
>
> s/capabilities/capabilities for/
> s/capabilities/capabilities from/
>
> would be sufficient.

The same error already exists in qemu_vhost_user.c and qemu_slirp.c.
Should I touch that too?