qemu: do not use deprecated options for new virtiofsd (virtiofs epopee)

[libvirt PATCHv2 3/5] qemu: fill capabilities for virtiofsd

Posted by Ján Tomko 2 months, 2 weeks ago

Run the daemon with --print-capabilities first, to see what it supports.

Signed-off-by: Ján Tomko <jtomko@redhat.com>
---
 src/conf/domain_conf.h     |  1 +
 src/qemu/qemu_vhost_user.c | 60 ++++++++++++++++++++++++++++++++++++++
 src/qemu/qemu_vhost_user.h | 12 ++++++++
 src/qemu/qemu_virtiofs.c   |  9 ++++--
 4 files changed, 80 insertions(+), 2 deletions(-)

diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 2818a9f1f5..b3a0d26cde 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -899,6 +899,7 @@ struct _virDomainFSDef {
     virDomainIdMapDef idmap;
     virDomainVirtioOptions *virtio;
     virObject *privateData;
+    virBitmap *caps;
 };
 
 
diff --git a/src/qemu/qemu_vhost_user.c b/src/qemu/qemu_vhost_user.c
index 0294daab80..30a60db528 100644
--- a/src/qemu/qemu_vhost_user.c
+++ b/src/qemu/qemu_vhost_user.c
@@ -22,6 +22,7 @@
 
 #include "qemu_vhost_user.h"
 #include "qemu_interop_config.h"
+#include "virbitmap.h"
 #include "virjson.h"
 #include "virlog.h"
 #include "viralloc.h"
@@ -90,6 +91,12 @@ VIR_ENUM_IMPL(qemuVhostUserGPUFeature,
               "render-node",
 );
 
+VIR_ENUM_IMPL(qemuVhostUserFSFeature,
+              QEMU_VHOST_USER_FS_FEATURE_LAST,
+              "migrate-precopy",
+              "separate-options",
+);
+
 typedef struct _qemuVhostUserGPU qemuVhostUserGPU;
 struct _qemuVhostUserGPU {
     size_t nfeatures;
@@ -414,6 +421,53 @@ qemuVhostUserFillDomainGPU(virQEMUDriver *driver,
     return ret;
 }
 
+int
+qemuVhostUserFillFSCapabilities(virBitmap **caps,
+                                const char *binary,
+                                bool reportError G_GNUC_UNUSED)
+{
+    g_autoptr(virJSONValue) doc = NULL;
+    g_autofree char *output = NULL;
+    g_autoptr(virCommand) cmd = NULL;
+    virJSONValue *featuresJSON;
+    size_t nfeatures;
+    size_t i;
+    g_autoptr(virBitmap) features = NULL;
+
+    cmd = virCommandNewArgList(binary, "--print-capabilities", NULL);
+    virCommandSetOutputBuffer(cmd, &output);
+    if (virCommandRun(cmd, NULL) < 0)
+        return -2;
+
+    if (!(doc = virJSONValueFromString(output))) {
+        virReportError(VIR_ERR_INTERNAL_ERROR,
+                       _("unable to parse json capabilities '%1$s'"),
+                       binary);
+        return -1;
+    }
+
+    /* Older virtiofsd did not print any features */
+    if (!(featuresJSON = virJSONValueObjectGetArray(doc, "features")))
+        return 0;
+
+    features = virBitmapNew(0);
+    nfeatures = virJSONValueArraySize(featuresJSON);
+
+    for (i = 0; i < nfeatures; i++) {
+        virJSONValue *item = virJSONValueArrayGet(featuresJSON, i);
+        const char *tmpStr = virJSONValueGetString(item);
+        int tmp;
+
+        if ((tmp = qemuVhostUserFSFeatureTypeFromString(tmpStr)) < 0) {
+            VIR_DEBUG("ignoring unknown virtiofs feature '%s'", tmpStr);
+            continue;
+        }
+        virBitmapSetBitExpand(features, tmp);
+    }
+
+    *caps = g_steal_pointer(&features);
+    return 0;
+}
 
 int
 qemuVhostUserFillDomainFS(virQEMUDriver *driver,
@@ -435,6 +489,12 @@ qemuVhostUserFillDomainFS(virQEMUDriver *driver,
             continue;
 
         fs->binary = g_strdup(vu->binary);
+
+        /* skip binaries that can't report their capabilities */
+        if (qemuVhostUserFillFSCapabilities(&fs->caps,
+                                            vu->binary,
+                                            false) == -1)
+            continue;
         break;
     }
 
diff --git a/src/qemu/qemu_vhost_user.h b/src/qemu/qemu_vhost_user.h
index d1aa6ca189..f9cd549d5d 100644
--- a/src/qemu/qemu_vhost_user.h
+++ b/src/qemu/qemu_vhost_user.h
@@ -46,3 +46,15 @@ qemuVhostUserFillDomainGPU(virQEMUDriver *driver,
 int
 qemuVhostUserFillDomainFS(virQEMUDriver *driver,
                           virDomainFSDef *fs);
+
+int
+qemuVhostUserFillFSCapabilities(virBitmap **caps,
+                                const char *binary,
+                                bool reportError);
+typedef enum {
+    QEMU_VHOST_USER_FS_FEATURE_MIGRATE_PRECOPY = 0,
+    QEMU_VHOST_USER_FS_FEATURE_SEPARATE_OPTIONS,
+    QEMU_VHOST_USER_FS_FEATURE_LAST
+} qemuVhostUserFSFeature;
+
+VIR_ENUM_DECL(qemuVhostUserFSFeature);
diff --git a/src/qemu/qemu_virtiofs.c b/src/qemu/qemu_virtiofs.c
index 78897d8177..aba6a19562 100644
--- a/src/qemu/qemu_virtiofs.c
+++ b/src/qemu/qemu_virtiofs.c
@@ -446,8 +446,13 @@ qemuVirtioFSPrepareDomain(virQEMUDriver *driver,
     if (fs->sock)
         return 0;
 
-    if (!fs->binary && qemuVhostUserFillDomainFS(driver, fs) < 0)
-        return -1;
+    if (fs->binary) {
+        if (qemuVhostUserFillFSCapabilities(&fs->caps, fs->binary, true) < 0)
+            return -1;
+    } else {
+        if (qemuVhostUserFillDomainFS(driver, fs) < 0)
+            return -1;
+    }
 
     if (!driver->privileged && !fs->idmap.uidmap) {
         if (qemuVirtioFSPrepareIdMap(fs) < 0)
-- 
2.45.2

Re: [libvirt PATCHv2 3/5] qemu: fill capabilities for virtiofsd

Posted by Michal Prívozník 2 months, 1 week ago

On 7/4/24 15:54, Ján Tomko wrote:
> Run the daemon with --print-capabilities first, to see what it supports.
> 
> Signed-off-by: Ján Tomko <jtomko@redhat.com>
> ---
>  src/conf/domain_conf.h     |  1 +
>  src/qemu/qemu_vhost_user.c | 60 ++++++++++++++++++++++++++++++++++++++
>  src/qemu/qemu_vhost_user.h | 12 ++++++++
>  src/qemu/qemu_virtiofs.c   |  9 ++++--
>  4 files changed, 80 insertions(+), 2 deletions(-)

Since I don't have /usr/libexec/virtiofsd on my system, then I'm seeing some tests failing after this.

> 
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 2818a9f1f5..b3a0d26cde 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -899,6 +899,7 @@ struct _virDomainFSDef {
>      virDomainIdMapDef idmap;
>      virDomainVirtioOptions *virtio;
>      virObject *privateData;
> +    virBitmap *caps;

Surely this must be coupled with corresponding virBitmapFree() call in virDomainFSDefFree().


>  };
>  
>  
> diff --git a/src/qemu/qemu_vhost_user.c b/src/qemu/qemu_vhost_user.c
> index 0294daab80..30a60db528 100644
> --- a/src/qemu/qemu_vhost_user.c
> +++ b/src/qemu/qemu_vhost_user.c
> @@ -22,6 +22,7 @@
>  
>  #include "qemu_vhost_user.h"
>  #include "qemu_interop_config.h"
> +#include "virbitmap.h"
>  #include "virjson.h"
>  #include "virlog.h"
>  #include "viralloc.h"
> @@ -90,6 +91,12 @@ VIR_ENUM_IMPL(qemuVhostUserGPUFeature,
>                "render-node",
>  );
>  
> +VIR_ENUM_IMPL(qemuVhostUserFSFeature,
> +              QEMU_VHOST_USER_FS_FEATURE_LAST,
> +              "migrate-precopy",
> +              "separate-options",
> +);
> +
>  typedef struct _qemuVhostUserGPU qemuVhostUserGPU;
>  struct _qemuVhostUserGPU {
>      size_t nfeatures;
> @@ -414,6 +421,53 @@ qemuVhostUserFillDomainGPU(virQEMUDriver *driver,
>      return ret;
>  }
>  
> +int
> +qemuVhostUserFillFSCapabilities(virBitmap **caps,
> +                                const char *binary,
> +                                bool reportError G_GNUC_UNUSED)

If it's unused then why introduce it at all?

> +{
> +    g_autoptr(virJSONValue) doc = NULL;
> +    g_autofree char *output = NULL;
> +    g_autoptr(virCommand) cmd = NULL;
> +    virJSONValue *featuresJSON;
> +    size_t nfeatures;
> +    size_t i;
> +    g_autoptr(virBitmap) features = NULL;
> +
> +    cmd = virCommandNewArgList(binary, "--print-capabilities", NULL);
> +    virCommandSetOutputBuffer(cmd, &output);
> +    if (virCommandRun(cmd, NULL) < 0)
> +        return -2;
> +
> +    if (!(doc = virJSONValueFromString(output))) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("unable to parse json capabilities '%1$s'"),
> +                       binary);
> +        return -1;
> +    }
> +
> +    /* Older virtiofsd did not print any features */
> +    if (!(featuresJSON = virJSONValueObjectGetArray(doc, "features")))
> +        return 0;
> +
> +    features = virBitmapNew(0);
> +    nfeatures = virJSONValueArraySize(featuresJSON);
> +
> +    for (i = 0; i < nfeatures; i++) {
> +        virJSONValue *item = virJSONValueArrayGet(featuresJSON, i);
> +        const char *tmpStr = virJSONValueGetString(item);
> +        int tmp;
> +
> +        if ((tmp = qemuVhostUserFSFeatureTypeFromString(tmpStr)) < 0) {
> +            VIR_DEBUG("ignoring unknown virtiofs feature '%s'", tmpStr);
> +            continue;
> +        }
> +        virBitmapSetBitExpand(features, tmp);
> +    }
> +
> +    *caps = g_steal_pointer(&features);
> +    return 0;
> +}



Squash this in please:


diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index bfef89e1be..abc8e4ed66 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -2600,6 +2600,7 @@ void virDomainFSDefFree(virDomainFSDef *def)
     g_free(def->sock);
     g_free(def->idmap.uidmap);
     g_free(def->idmap.gidmap);
+    virBitmapFree(def->caps);
 
     g_free(def);
 }
diff --git a/src/qemu/qemu_vhost_user.c b/src/qemu/qemu_vhost_user.c
index 30a60db528..de3ef640a3 100644
--- a/src/qemu/qemu_vhost_user.c
+++ b/src/qemu/qemu_vhost_user.c
@@ -423,8 +423,7 @@ qemuVhostUserFillDomainGPU(virQEMUDriver *driver,
 
 int
 qemuVhostUserFillFSCapabilities(virBitmap **caps,
-                                const char *binary,
-                                bool reportError G_GNUC_UNUSED)
+                                const char *binary)
 {
     g_autoptr(virJSONValue) doc = NULL;
     g_autofree char *output = NULL;
@@ -492,8 +491,7 @@ qemuVhostUserFillDomainFS(virQEMUDriver *driver,
 
         /* skip binaries that can't report their capabilities */
         if (qemuVhostUserFillFSCapabilities(&fs->caps,
-                                            vu->binary,
-                                            false) == -1)
+                                            vu->binary) == -1)
             continue;
         break;
     }
diff --git a/src/qemu/qemu_vhost_user.h b/src/qemu/qemu_vhost_user.h
index f9cd549d5d..c39fbfebe8 100644
--- a/src/qemu/qemu_vhost_user.h
+++ b/src/qemu/qemu_vhost_user.h
@@ -49,8 +49,7 @@ qemuVhostUserFillDomainFS(virQEMUDriver *driver,
 
 int
 qemuVhostUserFillFSCapabilities(virBitmap **caps,
-                                const char *binary,
-                                bool reportError);
+                                const char *binary);
 typedef enum {
     QEMU_VHOST_USER_FS_FEATURE_MIGRATE_PRECOPY = 0,
     QEMU_VHOST_USER_FS_FEATURE_SEPARATE_OPTIONS,
diff --git a/src/qemu/qemu_virtiofs.c b/src/qemu/qemu_virtiofs.c
index aba6a19562..0df8d67b1b 100644
--- a/src/qemu/qemu_virtiofs.c
+++ b/src/qemu/qemu_virtiofs.c
@@ -447,7 +447,7 @@ qemuVirtioFSPrepareDomain(virQEMUDriver *driver,
         return 0;
 
     if (fs->binary) {
-        if (qemuVhostUserFillFSCapabilities(&fs->caps, fs->binary, true) < 0)
+        if (qemuVhostUserFillFSCapabilities(&fs->caps, fs->binary) < 0)
             return -1;
     } else {
         if (qemuVhostUserFillDomainFS(driver, fs) < 0)
diff --git a/tests/qemuxmlconftest.c b/tests/qemuxmlconftest.c
index 4f2966109d..389d31800b 100644
--- a/tests/qemuxmlconftest.c
+++ b/tests/qemuxmlconftest.c
@@ -1076,6 +1076,8 @@ mymain(void)
 
     virFileWrapperAddPrefix("/usr/libexec/qemu/vhost-user",
                             abs_srcdir "/qemuvhostuserdata/usr/libexec/qemu/vhost-user");
+    virFileWrapperAddPrefix("/usr/libexec/virtiofsd",
+                            abs_srcdir "/qemuvhostuserdata/usr/libexec/qemu/vhost-user/test-virtiofsd");
 
     if (!(conn = virGetConnect()))
         return EXIT_FAILURE;

[libvirt PATCHv2 1/5] tests: vhostuser: add virtiofsd json descriptor
[libvirt PATCHv2 2/5] tests: qemuxmlconf: adjust test case to new virtiofsd
[libvirt PATCHv2 3/5] qemu: fill capabilities for virtiofsd
[libvirt PATCHv2 4/5] qemu: do not use deprecated options for new virtiofsd
[libvirt PATCHv2 5/5] qemu: migration: allow migration for virtiofs