There is a bug in the interface: we don't allow vhostfds argument
together with queues. But we allow vhostfd, and try use it for all
queues of multiqueue TAP.

Let's relax the restriction. We already check that number of vhost fds
match queues (or number of fds). So, no matter do vhost fds come from
vhostfds or vhostfd argument. Let's use correct vhost fds for multiqueue
TAP.

To achieve this we move vhost fds parsing to separate function and call
it earlier in net_init_tap(). Then we have vhost fds available (and
already checked) for all further cases.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 net/tap.c | 62 ++++++++++++++++++++++++++-----------------------------
 1 file changed, 29 insertions(+), 33 deletions(-)

diff --git a/net/tap.c b/net/tap.c
index dbbbe3c500..b0b5e0b0fc 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -838,11 +838,31 @@ static int tap_parse_fds_and_queues(const NetdevTapOptions *tap, int **fds,
     return queues;
 }
 
+static bool tap_parse_vhost_fds(const NetdevTapOptions *tap, int **vhost_fds,
+                                unsigned queues, Error **errp)
+{
+    if (!(tap->vhostfd || tap->vhostfds)) {
+        *vhost_fds = NULL;
+        return true;
+    }
+
+    if (net_parse_fds(tap->fd ?: tap->fds, vhost_fds, queues, errp) < 0) {
+        return false;
+    }
+
+    if (!unblock_fds(*vhost_fds, queues, errp)) {
+        net_free_fds(*vhost_fds, queues);
+        return false;
+    }
+
+    return true;
+}
+
 int net_init_tap(const Netdev *netdev, const char *name,
                  NetClientState *peer, Error **errp)
 {
     const NetdevTapOptions *tap;
-    int fd = -1, vhostfd = -1, vnet_hdr = 0, i = 0, queues;
+    int fd = -1, vnet_hdr = 0, i = 0, queues;
     /* for the no-fd, no-helper case */
     char ifname[128];
     int *fds = NULL, *vhost_fds = NULL;
@@ -875,30 +895,13 @@ int net_init_tap(const Netdev *netdev, const char *name,
         return -1;
     }
 
-    if (tap->vhostfds && !tap->fds) {
-        error_setg(errp, "vhostfds= is invalid if fds= wasn't specified");
-        return -1;
-    }
-
-    if (tap->vhostfd && tap->fds) {
-        error_setg(errp, "vhostfd= is invalid with fds=");
-        return -1;
-    }
-
     queues = tap_parse_fds_and_queues(tap, &fds, errp);
     if (queues < 0) {
         return -1;
     }
 
-    if (tap->vhostfd) {
-        vhostfd = monitor_fd_param(monitor_cur(), tap->vhostfd, errp);
-        if (vhostfd == -1) {
-            goto fail;
-        }
-
-        if (!qemu_set_blocking(vhostfd, false, errp)) {
-            goto fail;
-        }
+    if (!tap_parse_vhost_fds(tap, &vhost_fds, queues, errp)) {
+        goto fail;
     }
 
     if (tap->fd) {
@@ -909,20 +912,12 @@ int net_init_tap(const Netdev *netdev, const char *name,
 
         if (!net_init_tap_one(tap, peer, name, NULL,
                               NULL, NULL,
-                              vhostfd, vnet_hdr, fds[0], errp)) {
+                              vhost_fds ? vhost_fds[0] : -1,
+                              vnet_hdr, fds[0], errp)) {
             goto fail;
         }
     } else if (tap->fds) {
-        if (tap->vhostfds && net_parse_fds(tap->vhostfds, &vhost_fds,
-                                           queues, errp) < 0) {
-            goto fail;
-        }
-
         for (i = 0; i < queues; i++) {
-            if (vhost_fds && !qemu_set_blocking(vhost_fds[i], false, errp)) {
-                goto fail;
-            }
-
             if (i == 0) {
                 vnet_hdr = tap_probe_vnet_hdr(fds[i], errp);
                 if (vnet_hdr < 0) {
@@ -948,7 +943,8 @@ int net_init_tap(const Netdev *netdev, const char *name,
         }
 
         if (!net_init_tap_one(tap, peer, name, ifname,
-                              NULL, NULL, vhostfd,
+                              NULL, NULL,
+                              vhost_fds ? vhost_fds[0] : -1,
                               vnet_hdr, fds[0], errp)) {
             goto fail;
         }
@@ -981,7 +977,8 @@ int net_init_tap(const Netdev *netdev, const char *name,
             if (!net_init_tap_one(tap, peer, name, ifname,
                                   i >= 1 ? NULL : script,
                                   i >= 1 ? NULL : downscript,
-                                  vhostfd, vnet_hdr, fd, errp)) {
+                                  vhost_fds ? vhost_fds[i] : -1,
+                                  vnet_hdr, fd, errp)) {
                 goto fail;
             }
         }
@@ -991,7 +988,6 @@ int net_init_tap(const Netdev *netdev, const char *name,
 
 fail:
     close(fd);
-    close(vhostfd);
     net_free_fds(fds, queues);
     net_free_fds(vhost_fds, queues);
     return -1;
-- 
2.52.0

> There is a bug in the interface: we don't allow vhostfds argument
> together with queues. But we allow vhostfd, and try use it for all
> queues of multiqueue TAP.


> Let's relax the restriction. We already check that number of vhost fds
> match queues (or number of fds). So, no matter do vhost fds come from
> vhostfds or vhostfd argument. Let's use correct vhost fds for multiqueue
> TAP.


> To achieve this we move vhost fds parsing to separate function and call
> it earlier in net_init_tap(). Then we have vhost fds available (and
> already checked) for all further cases.

I like the idea behind this change, but given that it modifies the API slightly,
can you include and update to the documentation that clarifies which
options are allowed to coexist and in what context?

Thanks,
        Ben

On 03.02.26 22:40, Chaney, Ben wrote:
> 
>> There is a bug in the interface: we don't allow vhostfds argument
>> together with queues. But we allow vhostfd, and try use it for all
>> queues of multiqueue TAP.
> 
> 
>> Let's relax the restriction. We already check that number of vhost fds
>> match queues (or number of fds). So, no matter do vhost fds come from
>> vhostfds or vhostfd argument. Let's use correct vhost fds for multiqueue
>> TAP.
> 
> 
>> To achieve this we move vhost fds parsing to separate function and call
>> it earlier in net_init_tap(). Then we have vhost fds available (and
>> already checked) for all further cases.
> 
> I like the idea behind this change, but given that it modifies the API slightly,
> can you include and update to the documentation that clarifies which
> options are allowed to coexist and in what context?
> 

Hmm, current documentation lack this information at all, so I don't think
it's a problem of this patch.

Better, I'll add more documentation in separate commit on top (or as part) of
"[PATCH] qapi/net: deprecate TAP fd and vhostfd options".

Thanks for reviewing!

-- 
Best regards,
Vladimir