[PATCH v3 00/12] net: refactoring and fixes

[PATCH v3 00/12] net: refactoring and fixes

[Vladimir Sementsov-Ogievskiy]

Hi all!

Here are some refactoring and fixes, mostly in net/tap.c I'm making on
my way to implementing TAP fd migration (through UNIX domain socket).

v3:

01: fix checking, switch to int type for queues
08: drop MAX_TAP_QUEUES constant
09,10: prefer int type for queues


Vladimir Sementsov-Ogievskiy (12):
  net/af-xdp: fix type overflow
  net/tap: net_init_tap_one(): add return value
  net/tap: net_init_tap(): drop extra vhostfdname variable
  net/tap: net_init_tap(): refactor parameter checking
  net/tap: net_init_tap(): common fail label
  net/tap: net_init_tap_one() refactor to get vhostfd param
  net/tap: net_init_tap_one(): drop model parameter
  net: introduce net_parse_fds()
  net/tap: move fds parameters handling to separate functions
  net/tap: fix vhostfds/vhostfd parameters API
  net/tap: net_init_tap(): merge fd=, fds= and helper= cases into one
  net/tap: net_init_tap(): relax QEMU hubs check

 net/af-xdp.c |  44 ++-----
 net/tap.c    | 319 +++++++++++++++++++++------------------------------
 net/util.c   |  50 ++++++++
 net/util.h   |  14 +++
 4 files changed, 201 insertions(+), 226 deletions(-)

-- 
2.52.0

[PATCH v3 01/12] net/af-xdp: fix type overflow

[Vladimir Sementsov-Ogievskiy]

In for-loop in net_init_af_xdp, we do nc->queue_index = i,
where is is int64_t for 0 to queues-1, and nc->queue_index is
unsigned int.

Also in parse_socket_fds, g_strv_length() returns guint which
is equivalent to unsigned int.

Let's simply use int type for queues, and update the check
appropriately. It could be unsigned int, but in future commits
we'll share with net/tap.c the common function which will return
number of queues or negative error. So, let's simply use int for
queues-related variables, that simplifies things.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 net/af-xdp.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/net/af-xdp.c b/net/af-xdp.c
index 14f302ea21..ff1cb30a98 100644
--- a/net/af-xdp.c
+++ b/net/af-xdp.c
@@ -442,14 +442,14 @@ static NetClientInfo net_af_xdp_info = {
 };
 
 static int *parse_socket_fds(const char *sock_fds_str,
-                             int64_t n_expected, Error **errp)
+                             int n_expected, Error **errp)
 {
     gchar **substrings = g_strsplit(sock_fds_str, ":", -1);
-    int64_t i, n_sock_fds = g_strv_length(substrings);
+    int i, n_sock_fds = g_strv_length(substrings);
     int *sock_fds = NULL;
 
     if (n_sock_fds != n_expected) {
-        error_setg(errp, "expected %"PRIi64" socket fds, got %"PRIi64,
+        error_setg(errp, "expected %d socket fds, got %d",
                    n_expected, n_sock_fds);
         goto exit;
     }
@@ -484,7 +484,7 @@ int net_init_af_xdp(const Netdev *netdev,
     unsigned int ifindex;
     uint32_t prog_id = 0;
     g_autofree int *sock_fds = NULL;
-    int64_t i, queues;
+    int i, queues;
     Error *err = NULL;
     AFXDPState *s;
     bool inhibit;
@@ -496,13 +496,14 @@ int net_init_af_xdp(const Netdev *netdev,
         return -1;
     }
 
-    queues = opts->has_queues ? opts->queues : 1;
-    if (queues < 1) {
+    if (opts->has_queues && (opts->queues < 1 || opts->queues > INT_MAX)) {
         error_setg(errp, "invalid number of queues (%" PRIi64 ") for '%s'",
-                   queues, opts->ifname);
+                   opts->queues, opts->ifname);
         return -1;
     }
 
+    queues = opts->has_queues ? opts->queues : 1;
+
     inhibit = opts->has_inhibit && opts->inhibit;
     if (inhibit && !opts->sock_fds && !opts->map_path) {
         error_setg(errp, "'inhibit=on' requires 'sock-fds' or 'map-path'");
@@ -537,7 +538,7 @@ int net_init_af_xdp(const Netdev *netdev,
 
     for (i = 0; i < queues; i++) {
         nc = qemu_new_net_client(&net_af_xdp_info, peer, "af-xdp", name);
-        qemu_set_info_str(nc, "af-xdp%"PRIi64" to %s", i, opts->ifname);
+        qemu_set_info_str(nc, "af-xdp%d to %s", i, opts->ifname);
         nc->queue_index = i;
 
         if (!nc0) {
-- 
2.52.0

[PATCH v3 02/12] net/tap: net_init_tap_one(): add return value

[Vladimir Sementsov-Ogievskiy]

Follow common recommendations in include/qapi/error.h of having
a return value together with errp. This allows to avoid error propagation.

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

diff --git a/net/tap.c b/net/tap.c
index 8d7ab6ba6f..a389aec218 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -703,7 +703,7 @@ static int net_tap_init(const NetdevTapOptions *tap, int *vnet_hdr,
 
 #define MAX_TAP_QUEUES 1024
 
-static void net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
+static bool net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
                              const char *model, const char *name,
                              const char *ifname, const char *script,
                              const char *downscript, const char *vhostfdname,
@@ -783,10 +783,11 @@ static void net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
         }
     }
 
-    return;
+    return true;
 
 failed:
     qemu_del_net_client(&s->nc);
+    return false;
 }
 
 static int get_fds(char *str, char *fds[], int max)
@@ -821,7 +822,6 @@ int net_init_tap(const Netdev *netdev, const char *name,
     const NetdevTapOptions *tap;
     int fd, vnet_hdr = 0, i = 0, queues;
     /* for the no-fd, no-helper case */
-    Error *err = NULL;
     const char *vhostfdname;
     char ifname[128];
     int ret = 0;
@@ -869,11 +869,9 @@ int net_init_tap(const Netdev *netdev, const char *name,
             return -1;
         }
 
-        net_init_tap_one(tap, peer, "tap", name, NULL,
-                         NULL, NULL,
-                         vhostfdname, vnet_hdr, fd, &err);
-        if (err) {
-            error_propagate(errp, err);
+        if (!net_init_tap_one(tap, peer, "tap", name, NULL,
+                              NULL, NULL,
+                              vhostfdname, vnet_hdr, fd, errp)) {
             close(fd);
             return -1;
         }
@@ -930,12 +928,10 @@ int net_init_tap(const Netdev *netdev, const char *name,
                 goto free_fail;
             }
 
-            net_init_tap_one(tap, peer, "tap", name, ifname,
-                             NULL, NULL,
-                             tap->vhostfds ? vhost_fds[i] : NULL,
-                             vnet_hdr, fd, &err);
-            if (err) {
-                error_propagate(errp, err);
+            if (!net_init_tap_one(tap, peer, "tap", name, ifname,
+                                  NULL, NULL,
+                                  tap->vhostfds ? vhost_fds[i] : NULL,
+                                  vnet_hdr, fd, errp)) {
                 ret = -1;
                 goto free_fail;
             }
@@ -975,11 +971,9 @@ free_fail:
             return -1;
         }
 
-        net_init_tap_one(tap, peer, "bridge", name, ifname,
-                         NULL, NULL, vhostfdname,
-                         vnet_hdr, fd, &err);
-        if (err) {
-            error_propagate(errp, err);
+        if (!net_init_tap_one(tap, peer, "bridge", name, ifname,
+                              NULL, NULL, vhostfdname,
+                              vnet_hdr, fd, errp)) {
             close(fd);
             return -1;
         }
@@ -1015,12 +1009,10 @@ free_fail:
                 }
             }
 
-            net_init_tap_one(tap, peer, "tap", name, ifname,
-                             i >= 1 ? NULL : script,
-                             i >= 1 ? NULL : downscript,
-                             vhostfdname, vnet_hdr, fd, &err);
-            if (err) {
-                error_propagate(errp, err);
+            if (!net_init_tap_one(tap, peer, "tap", name, ifname,
+                                  i >= 1 ? NULL : script,
+                                  i >= 1 ? NULL : downscript,
+                                  vhostfdname, vnet_hdr, fd, errp)) {
                 close(fd);
                 return -1;
             }
-- 
2.52.0

[PATCH v3 03/12] net/tap: net_init_tap(): drop extra vhostfdname variable

[Vladimir Sementsov-Ogievskiy]

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

diff --git a/net/tap.c b/net/tap.c
index a389aec218..55b8b33ea8 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -822,14 +822,12 @@ int net_init_tap(const Netdev *netdev, const char *name,
     const NetdevTapOptions *tap;
     int fd, vnet_hdr = 0, i = 0, queues;
     /* for the no-fd, no-helper case */
-    const char *vhostfdname;
     char ifname[128];
     int ret = 0;
 
     assert(netdev->type == NET_CLIENT_DRIVER_TAP);
     tap = &netdev->u.tap;
     queues = tap->has_queues ? tap->queues : 1;
-    vhostfdname = tap->vhostfd;
 
     /* QEMU hubs do not support multiqueue tap, in this case peer is set.
      * For -netdev, peer is always NULL. */
@@ -871,7 +869,7 @@ int net_init_tap(const Netdev *netdev, const char *name,
 
         if (!net_init_tap_one(tap, peer, "tap", name, NULL,
                               NULL, NULL,
-                              vhostfdname, vnet_hdr, fd, errp)) {
+                              tap->vhostfd, vnet_hdr, fd, errp)) {
             close(fd);
             return -1;
         }
@@ -972,7 +970,7 @@ free_fail:
         }
 
         if (!net_init_tap_one(tap, peer, "bridge", name, ifname,
-                              NULL, NULL, vhostfdname,
+                              NULL, NULL, tap->vhostfd,
                               vnet_hdr, fd, errp)) {
             close(fd);
             return -1;
@@ -1012,7 +1010,7 @@ free_fail:
             if (!net_init_tap_one(tap, peer, "tap", name, ifname,
                                   i >= 1 ? NULL : script,
                                   i >= 1 ? NULL : downscript,
-                                  vhostfdname, vnet_hdr, fd, errp)) {
+                                  tap->vhostfd, vnet_hdr, fd, errp)) {
                 close(fd);
                 return -1;
             }
-- 
2.52.0

[PATCH v3 04/12] net/tap: net_init_tap(): refactor parameter checking

[Vladimir Sementsov-Ogievskiy]

Move checks to the top of the function to simplify further
refactoring. Merge duplicated checks.

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

diff --git a/net/tap.c b/net/tap.c
index 55b8b33ea8..331a779585 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -841,16 +841,30 @@ int net_init_tap(const Netdev *netdev, const char *name,
         return -1;
     }
 
-    if (tap->fd) {
-        if (tap->ifname || tap->script || tap->downscript ||
-            tap->has_vnet_hdr || tap->helper || tap->has_queues ||
-            tap->fds || tap->vhostfds) {
-            error_setg(errp, "ifname=, script=, downscript=, vnet_hdr=, "
-                       "helper=, queues=, fds=, and vhostfds= "
-                       "are invalid with fd=");
-            return -1;
-        }
+    if (tap->has_queues + !!tap->helper + !!tap->fds + !!tap->fd > 1) {
+        error_setg(errp, "queues=, helper=, fds= and fd= are mutual exclusive");
+        return -1;
+    }
 
+    if ((tap->fd || tap->fds || tap->helper) &&
+        (tap->ifname || tap->script || tap->downscript ||
+         tap->has_vnet_hdr)) {
+        error_setg(errp, "ifname=, script=, downscript=, vnet_hdr= "
+                   "are invalid with fd=/fds=/helper=");
+        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;
+    }
+
+    if (tap->fd) {
         fd = monitor_fd_param(monitor_cur(), tap->fd, errp);
         if (fd == -1) {
             return -1;
@@ -878,15 +892,6 @@ int net_init_tap(const Netdev *netdev, const char *name,
         char **vhost_fds;
         int nfds = 0, nvhosts = 0;
 
-        if (tap->ifname || tap->script || tap->downscript ||
-            tap->has_vnet_hdr || tap->helper || tap->has_queues ||
-            tap->vhostfd) {
-            error_setg(errp, "ifname=, script=, downscript=, vnet_hdr=, "
-                       "helper=, queues=, and vhostfd= "
-                       "are invalid with fds=");
-            return -1;
-        }
-
         fds = g_new0(char *, MAX_TAP_QUEUES);
         vhost_fds = g_new0(char *, MAX_TAP_QUEUES);
 
@@ -946,13 +951,6 @@ free_fail:
         g_free(vhost_fds);
         return ret;
     } else if (tap->helper) {
-        if (tap->ifname || tap->script || tap->downscript ||
-            tap->has_vnet_hdr || tap->has_queues || tap->vhostfds) {
-            error_setg(errp, "ifname=, script=, downscript=, vnet_hdr=, "
-                       "queues=, and vhostfds= are invalid with helper=");
-            return -1;
-        }
-
         fd = net_bridge_run_helper(tap->helper,
                                    tap->br ?: DEFAULT_BRIDGE_INTERFACE,
                                    errp);
@@ -981,11 +979,6 @@ free_fail:
         g_autofree char *downscript =
             tap_parse_script(tap->downscript, DEFAULT_NETWORK_DOWN_SCRIPT);
 
-        if (tap->vhostfds) {
-            error_setg(errp, "vhostfds= is invalid if fds= wasn't specified");
-            return -1;
-        }
-
         if (tap->ifname) {
             pstrcpy(ifname, sizeof ifname, tap->ifname);
         } else {
-- 
2.52.0

[PATCH v3 05/12] net/tap: net_init_tap(): common fail label

[Vladimir Sementsov-Ogievskiy]

Add common failure label. This:

- simplifies failure paths in the function
- get rid of unusual free_fail: in the middle of the function
- simplify further refactoring

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

diff --git a/net/tap.c b/net/tap.c
index 331a779585..2eb8a2caeb 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -820,10 +820,12 @@ int net_init_tap(const Netdev *netdev, const char *name,
                  NetClientState *peer, Error **errp)
 {
     const NetdevTapOptions *tap;
-    int fd, 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 ret = 0;
+    char **fds = NULL, **vhost_fds = NULL;
+    int nfds = 0, nvhosts = 0;
+
 
     assert(netdev->type == NET_CLIENT_DRIVER_TAP);
     tap = &netdev->u.tap;
@@ -867,31 +869,24 @@ int net_init_tap(const Netdev *netdev, const char *name,
     if (tap->fd) {
         fd = monitor_fd_param(monitor_cur(), tap->fd, errp);
         if (fd == -1) {
-            return -1;
+            goto fail;
         }
 
         if (!qemu_set_blocking(fd, false, errp)) {
-            close(fd);
-            return -1;
+            goto fail;
         }
 
         vnet_hdr = tap_probe_vnet_hdr(fd, errp);
         if (vnet_hdr < 0) {
-            close(fd);
-            return -1;
+            goto fail;
         }
 
         if (!net_init_tap_one(tap, peer, "tap", name, NULL,
                               NULL, NULL,
                               tap->vhostfd, vnet_hdr, fd, errp)) {
-            close(fd);
-            return -1;
+            goto fail;
         }
     } else if (tap->fds) {
-        char **fds;
-        char **vhost_fds;
-        int nfds = 0, nvhosts = 0;
-
         fds = g_new0(char *, MAX_TAP_QUEUES);
         vhost_fds = g_new0(char *, MAX_TAP_QUEUES);
 
@@ -901,77 +896,58 @@ int net_init_tap(const Netdev *netdev, const char *name,
             if (nfds != nvhosts) {
                 error_setg(errp, "The number of fds passed does not match "
                            "the number of vhostfds passed");
-                ret = -1;
-                goto free_fail;
+                goto fail;
             }
         }
 
         for (i = 0; i < nfds; i++) {
             fd = monitor_fd_param(monitor_cur(), fds[i], errp);
             if (fd == -1) {
-                ret = -1;
-                goto free_fail;
+                goto fail;
             }
 
             if (!qemu_set_blocking(fd, false, errp)) {
-                ret = -1;
-                goto free_fail;
+                goto fail;
             }
 
             if (i == 0) {
                 vnet_hdr = tap_probe_vnet_hdr(fd, errp);
                 if (vnet_hdr < 0) {
-                    ret = -1;
-                    goto free_fail;
+                    goto fail;
                 }
             } else if (vnet_hdr != tap_probe_vnet_hdr(fd, NULL)) {
                 error_setg(errp,
                            "vnet_hdr not consistent across given tap fds");
-                ret = -1;
-                goto free_fail;
+                goto fail;
             }
 
             if (!net_init_tap_one(tap, peer, "tap", name, ifname,
                                   NULL, NULL,
                                   tap->vhostfds ? vhost_fds[i] : NULL,
                                   vnet_hdr, fd, errp)) {
-                ret = -1;
-                goto free_fail;
+                goto fail;
             }
         }
-
-free_fail:
-        for (i = 0; i < nvhosts; i++) {
-            g_free(vhost_fds[i]);
-        }
-        for (i = 0; i < nfds; i++) {
-            g_free(fds[i]);
-        }
-        g_free(fds);
-        g_free(vhost_fds);
-        return ret;
     } else if (tap->helper) {
         fd = net_bridge_run_helper(tap->helper,
                                    tap->br ?: DEFAULT_BRIDGE_INTERFACE,
                                    errp);
         if (fd == -1) {
-            return -1;
+            goto fail;
         }
 
         if (!qemu_set_blocking(fd, false, errp)) {
-            return -1;
+            goto fail;
         }
         vnet_hdr = tap_probe_vnet_hdr(fd, errp);
         if (vnet_hdr < 0) {
-            close(fd);
-            return -1;
+            goto fail;
         }
 
         if (!net_init_tap_one(tap, peer, "bridge", name, ifname,
                               NULL, NULL, tap->vhostfd,
                               vnet_hdr, fd, errp)) {
-            close(fd);
-            return -1;
+            goto fail;
         }
     } else {
         g_autofree char *script =
@@ -989,14 +965,13 @@ free_fail:
             fd = net_tap_init(tap, &vnet_hdr, i >= 1 ? NULL : script,
                               ifname, sizeof ifname, queues > 1, errp);
             if (fd == -1) {
-                return -1;
+                goto fail;
             }
 
             if (queues > 1 && i == 0 && !tap->ifname) {
                 if (tap_fd_get_ifname(fd, ifname)) {
                     error_setg(errp, "Fail to get ifname");
-                    close(fd);
-                    return -1;
+                    goto fail;
                 }
             }
 
@@ -1004,13 +979,28 @@ free_fail:
                                   i >= 1 ? NULL : script,
                                   i >= 1 ? NULL : downscript,
                                   tap->vhostfd, vnet_hdr, fd, errp)) {
-                close(fd);
-                return -1;
+                goto fail;
             }
         }
     }
 
     return 0;
+
+fail:
+    close(fd);
+    if (vhost_fds) {
+        for (i = 0; i < nvhosts; i++) {
+            g_free(vhost_fds[i]);
+        }
+        g_free(vhost_fds);
+    }
+    if (fds) {
+        for (i = 0; i < nfds; i++) {
+            g_free(fds[i]);
+        }
+        g_free(fds);
+    }
+    return -1;
 }
 
 int tap_enable(NetClientState *nc)
-- 
2.52.0

[PATCH v3 06/12] net/tap: net_init_tap_one() refactor to get vhostfd param

[Vladimir Sementsov-Ogievskiy]

Get vhostfd instead of vhostfdname:

- more symmetry with fd param
- prepare to further changes

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

diff --git a/net/tap.c b/net/tap.c
index 2eb8a2caeb..2c5f8e73fe 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -706,11 +706,10 @@ static int net_tap_init(const NetdevTapOptions *tap, int *vnet_hdr,
 static bool net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
                              const char *model, const char *name,
                              const char *ifname, const char *script,
-                             const char *downscript, const char *vhostfdname,
+                             const char *downscript, int vhostfd,
                              int vnet_hdr, int fd, Error **errp)
 {
     TAPState *s = net_tap_fd_init(peer, model, name, fd, vnet_hdr);
-    int vhostfd;
     bool sndbuf_required = tap->has_sndbuf;
     int sndbuf =
         (tap->has_sndbuf && tap->sndbuf) ? MIN(tap->sndbuf, INT_MAX) : INT_MAX;
@@ -738,7 +737,7 @@ static bool net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
     }
 
     if (tap->has_vhost ? tap->vhost :
-        vhostfdname || (tap->has_vhostforce && tap->vhostforce)) {
+        (vhostfd != -1) || (tap->has_vhostforce && tap->vhostforce)) {
         VhostNetOptions options;
 
         options.backend_type = VHOST_BACKEND_TYPE_KERNEL;
@@ -749,15 +748,7 @@ static bool net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
             options.busyloop_timeout = 0;
         }
 
-        if (vhostfdname) {
-            vhostfd = monitor_fd_param(monitor_cur(), vhostfdname, errp);
-            if (vhostfd == -1) {
-                goto failed;
-            }
-            if (!qemu_set_blocking(vhostfd, false, errp)) {
-                goto failed;
-            }
-        } else {
+        if (vhostfd == -1) {
             vhostfd = open("/dev/vhost-net", O_RDWR);
             if (vhostfd < 0) {
                 error_setg_file_open(errp, errno, "/dev/vhost-net");
@@ -820,7 +811,7 @@ int net_init_tap(const Netdev *netdev, const char *name,
                  NetClientState *peer, Error **errp)
 {
     const NetdevTapOptions *tap;
-    int fd = -1, vnet_hdr = 0, i = 0, queues;
+    int fd = -1, vhostfd = -1, vnet_hdr = 0, i = 0, queues;
     /* for the no-fd, no-helper case */
     char ifname[128];
     char **fds = NULL, **vhost_fds = NULL;
@@ -866,6 +857,17 @@ int net_init_tap(const Netdev *netdev, const char *name,
         return -1;
     }
 
+    if (tap->vhostfd) {
+        vhostfd = monitor_fd_param(monitor_cur(), tap->vhostfd, errp);
+        if (vhostfd == -1) {
+            return -1;
+        }
+
+        if (!qemu_set_blocking(vhostfd, false, errp)) {
+            goto fail;
+        }
+    }
+
     if (tap->fd) {
         fd = monitor_fd_param(monitor_cur(), tap->fd, errp);
         if (fd == -1) {
@@ -883,7 +885,7 @@ int net_init_tap(const Netdev *netdev, const char *name,
 
         if (!net_init_tap_one(tap, peer, "tap", name, NULL,
                               NULL, NULL,
-                              tap->vhostfd, vnet_hdr, fd, errp)) {
+                              vhostfd, vnet_hdr, fd, errp)) {
             goto fail;
         }
     } else if (tap->fds) {
@@ -910,6 +912,17 @@ int net_init_tap(const Netdev *netdev, const char *name,
                 goto fail;
             }
 
+            if (tap->vhostfds) {
+                vhostfd = monitor_fd_param(monitor_cur(), vhost_fds[i], errp);
+                if (vhostfd == -1) {
+                    goto fail;
+                }
+
+                if (!qemu_set_blocking(vhostfd, false, errp)) {
+                    goto fail;
+                }
+            }
+
             if (i == 0) {
                 vnet_hdr = tap_probe_vnet_hdr(fd, errp);
                 if (vnet_hdr < 0) {
@@ -923,7 +936,7 @@ int net_init_tap(const Netdev *netdev, const char *name,
 
             if (!net_init_tap_one(tap, peer, "tap", name, ifname,
                                   NULL, NULL,
-                                  tap->vhostfds ? vhost_fds[i] : NULL,
+                                  vhostfd,
                                   vnet_hdr, fd, errp)) {
                 goto fail;
             }
@@ -945,7 +958,7 @@ int net_init_tap(const Netdev *netdev, const char *name,
         }
 
         if (!net_init_tap_one(tap, peer, "bridge", name, ifname,
-                              NULL, NULL, tap->vhostfd,
+                              NULL, NULL, vhostfd,
                               vnet_hdr, fd, errp)) {
             goto fail;
         }
@@ -978,7 +991,7 @@ int net_init_tap(const Netdev *netdev, const char *name,
             if (!net_init_tap_one(tap, peer, "tap", name, ifname,
                                   i >= 1 ? NULL : script,
                                   i >= 1 ? NULL : downscript,
-                                  tap->vhostfd, vnet_hdr, fd, errp)) {
+                                  vhostfd, vnet_hdr, fd, errp)) {
                 goto fail;
             }
         }
@@ -988,6 +1001,7 @@ int net_init_tap(const Netdev *netdev, const char *name,
 
 fail:
     close(fd);
+    close(vhostfd);
     if (vhost_fds) {
         for (i = 0; i < nvhosts; i++) {
             g_free(vhost_fds[i]);
-- 
2.52.0

[PATCH v3 07/12] net/tap: net_init_tap_one(): drop model parameter

[Vladimir Sementsov-Ogievskiy]

It could be simply derived from tap parameter. And this change
simplifies further refactoring.

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

diff --git a/net/tap.c b/net/tap.c
index 2c5f8e73fe..db3fe380a4 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -704,12 +704,13 @@ static int net_tap_init(const NetdevTapOptions *tap, int *vnet_hdr,
 #define MAX_TAP_QUEUES 1024
 
 static bool net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
-                             const char *model, const char *name,
+                             const char *name,
                              const char *ifname, const char *script,
                              const char *downscript, int vhostfd,
                              int vnet_hdr, int fd, Error **errp)
 {
-    TAPState *s = net_tap_fd_init(peer, model, name, fd, vnet_hdr);
+    TAPState *s = net_tap_fd_init(peer, tap->helper ? "bridge" : "tap",
+                                  name, fd, vnet_hdr);
     bool sndbuf_required = tap->has_sndbuf;
     int sndbuf =
         (tap->has_sndbuf && tap->sndbuf) ? MIN(tap->sndbuf, INT_MAX) : INT_MAX;
@@ -883,7 +884,7 @@ int net_init_tap(const Netdev *netdev, const char *name,
             goto fail;
         }
 
-        if (!net_init_tap_one(tap, peer, "tap", name, NULL,
+        if (!net_init_tap_one(tap, peer, name, NULL,
                               NULL, NULL,
                               vhostfd, vnet_hdr, fd, errp)) {
             goto fail;
@@ -934,7 +935,7 @@ int net_init_tap(const Netdev *netdev, const char *name,
                 goto fail;
             }
 
-            if (!net_init_tap_one(tap, peer, "tap", name, ifname,
+            if (!net_init_tap_one(tap, peer, name, ifname,
                                   NULL, NULL,
                                   vhostfd,
                                   vnet_hdr, fd, errp)) {
@@ -957,7 +958,7 @@ int net_init_tap(const Netdev *netdev, const char *name,
             goto fail;
         }
 
-        if (!net_init_tap_one(tap, peer, "bridge", name, ifname,
+        if (!net_init_tap_one(tap, peer, name, ifname,
                               NULL, NULL, vhostfd,
                               vnet_hdr, fd, errp)) {
             goto fail;
@@ -988,7 +989,7 @@ int net_init_tap(const Netdev *netdev, const char *name,
                 }
             }
 
-            if (!net_init_tap_one(tap, peer, "tap", name, ifname,
+            if (!net_init_tap_one(tap, peer, name, ifname,
                                   i >= 1 ? NULL : script,
                                   i >= 1 ? NULL : downscript,
                                   vhostfd, vnet_hdr, fd, errp)) {
-- 
2.52.0

[PATCH v3 08/12] net: introduce net_parse_fds()

[Vladimir Sementsov-Ogievskiy]

Add common net_parse_fds() and net_free_fds() helpers and use them
in tap.c and af-xdp.c.

Choose returning queues instead of fds, because we'll have derived
helper in net/tap, which will be able to return fds=NULL and non-zero
queues on success. That's also why we move to INT_MAX for queues, to
support negative return value for net_parse_fds() (for failure paths).

Note that redundant restriction of MAX_TAP_QUEUES is dropped for tap.c

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 net/af-xdp.c | 33 ++-----------------
 net/tap.c    | 92 +++++++++++-----------------------------------------
 net/util.c   | 50 ++++++++++++++++++++++++++++
 net/util.h   | 14 ++++++++
 4 files changed, 85 insertions(+), 104 deletions(-)

diff --git a/net/af-xdp.c b/net/af-xdp.c
index ff1cb30a98..1ffd6363a8 100644
--- a/net/af-xdp.c
+++ b/net/af-xdp.c
@@ -21,6 +21,7 @@
 #include "clients.h"
 #include "monitor/monitor.h"
 #include "net/net.h"
+#include "net/util.h"
 #include "qapi/error.h"
 #include "qemu/cutils.h"
 #include "qemu/error-report.h"
@@ -441,35 +442,6 @@ static NetClientInfo net_af_xdp_info = {
     .cleanup = af_xdp_cleanup,
 };
 
-static int *parse_socket_fds(const char *sock_fds_str,
-                             int n_expected, Error **errp)
-{
-    gchar **substrings = g_strsplit(sock_fds_str, ":", -1);
-    int i, n_sock_fds = g_strv_length(substrings);
-    int *sock_fds = NULL;
-
-    if (n_sock_fds != n_expected) {
-        error_setg(errp, "expected %d socket fds, got %d",
-                   n_expected, n_sock_fds);
-        goto exit;
-    }
-
-    sock_fds = g_new(int, n_sock_fds);
-
-    for (i = 0; i < n_sock_fds; i++) {
-        sock_fds[i] = monitor_fd_param(monitor_cur(), substrings[i], errp);
-        if (sock_fds[i] < 0) {
-            g_free(sock_fds);
-            sock_fds = NULL;
-            goto exit;
-        }
-    }
-
-exit:
-    g_strfreev(substrings);
-    return sock_fds;
-}
-
 /*
  * The exported init function.
  *
@@ -530,8 +502,7 @@ int net_init_af_xdp(const Netdev *netdev,
     }
 
     if (opts->sock_fds) {
-        sock_fds = parse_socket_fds(opts->sock_fds, queues, errp);
-        if (!sock_fds) {
+        if (net_parse_fds(opts->sock_fds, &sock_fds, queues, errp) < 0) {
             return -1;
         }
     }
diff --git a/net/tap.c b/net/tap.c
index db3fe380a4..2d4630c350 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -45,6 +45,7 @@
 #include "hw/virtio/vhost.h"
 
 #include "net/tap.h"
+#include "net/util.h"
 
 #include "net/vhost_net.h"
 
@@ -701,8 +702,6 @@ static int net_tap_init(const NetdevTapOptions *tap, int *vnet_hdr,
     return fd;
 }
 
-#define MAX_TAP_QUEUES 1024
-
 static bool net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
                              const char *name,
                              const char *ifname, const char *script,
@@ -782,32 +781,6 @@ failed:
     return false;
 }
 
-static int get_fds(char *str, char *fds[], int max)
-{
-    char *ptr = str, *this;
-    size_t len = strlen(str);
-    int i = 0;
-
-    while (i < max && ptr < str + len) {
-        this = strchr(ptr, ':');
-
-        if (this == NULL) {
-            fds[i] = g_strdup(ptr);
-        } else {
-            fds[i] = g_strndup(ptr, this - ptr);
-        }
-
-        i++;
-        if (this == NULL) {
-            break;
-        } else {
-            ptr = this + 1;
-        }
-    }
-
-    return i;
-}
-
 int net_init_tap(const Netdev *netdev, const char *name,
                  NetClientState *peer, Error **errp)
 {
@@ -815,9 +788,7 @@ int net_init_tap(const Netdev *netdev, const char *name,
     int fd = -1, vhostfd = -1, vnet_hdr = 0, i = 0, queues;
     /* for the no-fd, no-helper case */
     char ifname[128];
-    char **fds = NULL, **vhost_fds = NULL;
-    int nfds = 0, nvhosts = 0;
-
+    int *fds = NULL, *vhost_fds = NULL;
 
     assert(netdev->type == NET_CLIENT_DRIVER_TAP);
     tap = &netdev->u.tap;
@@ -890,46 +861,31 @@ int net_init_tap(const Netdev *netdev, const char *name,
             goto fail;
         }
     } else if (tap->fds) {
-        fds = g_new0(char *, MAX_TAP_QUEUES);
-        vhost_fds = g_new0(char *, MAX_TAP_QUEUES);
-
-        nfds = get_fds(tap->fds, fds, MAX_TAP_QUEUES);
-        if (tap->vhostfds) {
-            nvhosts = get_fds(tap->vhostfds, vhost_fds, MAX_TAP_QUEUES);
-            if (nfds != nvhosts) {
-                error_setg(errp, "The number of fds passed does not match "
-                           "the number of vhostfds passed");
-                goto fail;
-            }
+        queues = net_parse_fds(tap->fds, &fds, 0, errp);
+        if (queues < 0) {
+            goto fail;
         }
 
-        for (i = 0; i < nfds; i++) {
-            fd = monitor_fd_param(monitor_cur(), fds[i], errp);
-            if (fd == -1) {
-                goto fail;
-            }
+        if (tap->vhostfds && net_parse_fds(tap->vhostfds, &vhost_fds,
+                                           queues, errp) < 0) {
+            goto fail;
+        }
 
-            if (!qemu_set_blocking(fd, false, errp)) {
+        for (i = 0; i < queues; i++) {
+            if (!qemu_set_blocking(fds[i], false, errp)) {
                 goto fail;
             }
 
-            if (tap->vhostfds) {
-                vhostfd = monitor_fd_param(monitor_cur(), vhost_fds[i], errp);
-                if (vhostfd == -1) {
-                    goto fail;
-                }
-
-                if (!qemu_set_blocking(vhostfd, false, errp)) {
-                    goto fail;
-                }
+            if (vhost_fds && !qemu_set_blocking(vhost_fds[i], false, errp)) {
+                goto fail;
             }
 
             if (i == 0) {
-                vnet_hdr = tap_probe_vnet_hdr(fd, errp);
+                vnet_hdr = tap_probe_vnet_hdr(fds[i], errp);
                 if (vnet_hdr < 0) {
                     goto fail;
                 }
-            } else if (vnet_hdr != tap_probe_vnet_hdr(fd, NULL)) {
+            } else if (vnet_hdr != tap_probe_vnet_hdr(fds[i], NULL)) {
                 error_setg(errp,
                            "vnet_hdr not consistent across given tap fds");
                 goto fail;
@@ -937,8 +893,8 @@ int net_init_tap(const Netdev *netdev, const char *name,
 
             if (!net_init_tap_one(tap, peer, name, ifname,
                                   NULL, NULL,
-                                  vhostfd,
-                                  vnet_hdr, fd, errp)) {
+                                  vhost_fds ? vhost_fds[i] : -1,
+                                  vnet_hdr, fds[i], errp)) {
                 goto fail;
             }
         }
@@ -1003,18 +959,8 @@ int net_init_tap(const Netdev *netdev, const char *name,
 fail:
     close(fd);
     close(vhostfd);
-    if (vhost_fds) {
-        for (i = 0; i < nvhosts; i++) {
-            g_free(vhost_fds[i]);
-        }
-        g_free(vhost_fds);
-    }
-    if (fds) {
-        for (i = 0; i < nfds; i++) {
-            g_free(fds[i]);
-        }
-        g_free(fds);
-    }
+    net_free_fds(fds, queues);
+    net_free_fds(vhost_fds, queues);
     return -1;
 }
 
diff --git a/net/util.c b/net/util.c
index 0b3dbfe5d3..1998a6554e 100644
--- a/net/util.c
+++ b/net/util.c
@@ -23,6 +23,8 @@
  */
 
 #include "qemu/osdep.h"
+#include "monitor/monitor.h"
+#include "qapi/error.h"
 #include "util.h"
 
 int net_parse_macaddr(uint8_t *macaddr, const char *p)
@@ -57,3 +59,51 @@ int net_parse_macaddr(uint8_t *macaddr, const char *p)
 
     return 0;
 }
+
+void net_free_fds(int *fds, int nfds)
+{
+    int i;
+
+    if (!fds || nfds <= 0) {
+        return;
+    }
+
+    for (i = 0; i < nfds; i++) {
+        if (fds[i] != -1) {
+            close(fds[i]);
+        }
+    }
+
+    g_free(fds);
+}
+
+int net_parse_fds(const char *fds_param, int **fds, int expected_nfds,
+                  Error **errp)
+{
+    g_auto(GStrv) fdnames = g_strsplit(fds_param, ":", -1);
+    unsigned nfds = g_strv_length(fdnames);
+    int i;
+
+    if (nfds > INT_MAX) {
+        error_setg(errp, "fds parameter exceeds maximum of %d", INT_MAX);
+        return -1;
+    }
+
+    if (expected_nfds && nfds != expected_nfds) {
+        error_setg(errp, "expected %u socket fds, got %u", expected_nfds, nfds);
+        return -1;
+    }
+
+    *fds = g_new(int, nfds);
+
+    for (i = 0; i < nfds; i++) {
+        (*fds)[i] = monitor_fd_param(monitor_cur(), fdnames[i], errp);
+        if ((*fds)[i] == -1) {
+            net_free_fds(*fds, i);
+            *fds = NULL;
+            return -1;
+        }
+    }
+
+    return nfds;
+}
diff --git a/net/util.h b/net/util.h
index 288312979f..4dbc5d416d 100644
--- a/net/util.h
+++ b/net/util.h
@@ -83,4 +83,18 @@ static inline bool in6_equal_net(const struct in6_addr *a,
 
 int net_parse_macaddr(uint8_t *macaddr, const char *p);
 
+/*
+ * Close all @fds and free @fds itself
+ */
+void net_free_fds(int *fds, int nfds);
+
+/*
+ * Parse @fds_param, where monitor fds are separated by a colon.
+ * @nfds must be non-NULL. If *@nfds is zero - set it accordingly.
+ * If *@nfds is non-zero - check that we have exactly *@nfds fds
+ * and fail otherwise.
+ */
+int net_parse_fds(const char *fds_param, int **fds, int expected_nfds,
+                  Error **errp);
+
 #endif /* QEMU_NET_UTIL_H */
-- 
2.52.0

[PATCH v3 09/12] net/tap: move fds parameters handling to separate functions

[Vladimir Sementsov-Ogievskiy]

This significantly simplify the code in net_init_tap().

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

diff --git a/net/tap.c b/net/tap.c
index 2d4630c350..b794f80e34 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -781,6 +781,61 @@ failed:
     return false;
 }
 
+static bool unblock_fds(int *fds, int nfds, Error **errp)
+{
+    for (int i = 0; i < nfds; i++) {
+        if (!qemu_set_blocking(fds[i], false, errp)) {
+            return false;
+        }
+    }
+
+    return true;
+}
+
+static int tap_parse_fds_and_queues(const NetdevTapOptions *tap, int **fds,
+                                    Error **errp)
+{
+    int queues = 1;
+
+    if (tap->has_queues + !!tap->helper + !!tap->fds + !!tap->fd > 1) {
+        error_setg(errp, "queues=, helper=, fds= and fd= are mutual exclusive");
+        return -1;
+    }
+
+    if (tap->has_queues) {
+        if (tap->queues > INT_MAX) {
+            error_setg(errp, "queues exceeds maximum %d", INT_MAX);
+            return -1;
+        }
+        queues = tap->queues;
+        *fds = NULL;
+    } else if (tap->fd || tap->fds) {
+        queues = net_parse_fds(tap->fd ?: tap->fds, fds,
+                               tap->fd ? 1 : 0, errp);
+        if (!*fds) {
+            return -1;
+        }
+    } else if (tap->helper) {
+        int fd = net_bridge_run_helper(tap->helper,
+                                       tap->br ?: DEFAULT_BRIDGE_INTERFACE,
+                                       errp);
+        if (fd < 0) {
+            return -1;
+        }
+
+        queues = 1;
+        *fds = g_new(int, 1);
+        **fds = fd;
+    }
+
+    if (*fds && !unblock_fds(*fds, queues, errp)) {
+        net_free_fds(*fds, queues);
+        return -1;
+    }
+
+    return queues;
+}
+
 int net_init_tap(const Netdev *netdev, const char *name,
                  NetClientState *peer, Error **errp)
 {
@@ -792,7 +847,6 @@ int net_init_tap(const Netdev *netdev, const char *name,
 
     assert(netdev->type == NET_CLIENT_DRIVER_TAP);
     tap = &netdev->u.tap;
-    queues = tap->has_queues ? tap->queues : 1;
 
     /* QEMU hubs do not support multiqueue tap, in this case peer is set.
      * For -netdev, peer is always NULL. */
@@ -829,10 +883,15 @@ int net_init_tap(const Netdev *netdev, const char *name,
         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) {
-            return -1;
+            goto fail;
         }
 
         if (!qemu_set_blocking(vhostfd, false, errp)) {
@@ -841,41 +900,23 @@ int net_init_tap(const Netdev *netdev, const char *name,
     }
 
     if (tap->fd) {
-        fd = monitor_fd_param(monitor_cur(), tap->fd, errp);
-        if (fd == -1) {
-            goto fail;
-        }
-
-        if (!qemu_set_blocking(fd, false, errp)) {
-            goto fail;
-        }
-
-        vnet_hdr = tap_probe_vnet_hdr(fd, errp);
+        vnet_hdr = tap_probe_vnet_hdr(fds[0], errp);
         if (vnet_hdr < 0) {
             goto fail;
         }
 
         if (!net_init_tap_one(tap, peer, name, NULL,
                               NULL, NULL,
-                              vhostfd, vnet_hdr, fd, errp)) {
+                              vhostfd, vnet_hdr, fds[0], errp)) {
             goto fail;
         }
     } else if (tap->fds) {
-        queues = net_parse_fds(tap->fds, &fds, 0, errp);
-        if (queues < 0) {
-            goto fail;
-        }
-
         if (tap->vhostfds && net_parse_fds(tap->vhostfds, &vhost_fds,
                                            queues, errp) < 0) {
             goto fail;
         }
 
         for (i = 0; i < queues; i++) {
-            if (!qemu_set_blocking(fds[i], false, errp)) {
-                goto fail;
-            }
-
             if (vhost_fds && !qemu_set_blocking(vhost_fds[i], false, errp)) {
                 goto fail;
             }
@@ -899,24 +940,14 @@ int net_init_tap(const Netdev *netdev, const char *name,
             }
         }
     } else if (tap->helper) {
-        fd = net_bridge_run_helper(tap->helper,
-                                   tap->br ?: DEFAULT_BRIDGE_INTERFACE,
-                                   errp);
-        if (fd == -1) {
-            goto fail;
-        }
-
-        if (!qemu_set_blocking(fd, false, errp)) {
-            goto fail;
-        }
-        vnet_hdr = tap_probe_vnet_hdr(fd, errp);
+        vnet_hdr = tap_probe_vnet_hdr(fds[0], errp);
         if (vnet_hdr < 0) {
             goto fail;
         }
 
         if (!net_init_tap_one(tap, peer, name, ifname,
                               NULL, NULL, vhostfd,
-                              vnet_hdr, fd, errp)) {
+                              vnet_hdr, fds[0], errp)) {
             goto fail;
         }
     } else {
-- 
2.52.0

[PATCH v3 10/12] net/tap: fix vhostfds/vhostfd parameters API

[Vladimir Sementsov-Ogievskiy]

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 b794f80e34..64164643d2 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -836,11 +836,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,
+                                int 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;
@@ -873,30 +893,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) {
@@ -907,20 +910,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) {
@@ -946,7 +941,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;
         }
@@ -979,7 +975,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;
             }
         }
@@ -989,7 +986,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

[PATCH v3 11/12] net/tap: net_init_tap(): merge fd=, fds= and helper= cases into one

[Vladimir Sementsov-Ogievskiy]

Now fd= and helper= cases are just a duplication of fds= case with
queues=1. Let's merge them all.

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

diff --git a/net/tap.c b/net/tap.c
index 64164643d2..2318158adc 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -902,19 +902,7 @@ int net_init_tap(const Netdev *netdev, const char *name,
         goto fail;
     }
 
-    if (tap->fd) {
-        vnet_hdr = tap_probe_vnet_hdr(fds[0], errp);
-        if (vnet_hdr < 0) {
-            goto fail;
-        }
-
-        if (!net_init_tap_one(tap, peer, name, NULL,
-                              NULL, NULL,
-                              vhost_fds ? vhost_fds[0] : -1,
-                              vnet_hdr, fds[0], errp)) {
-            goto fail;
-        }
-    } else if (tap->fds) {
+    if (fds) {
         for (i = 0; i < queues; i++) {
             if (i == 0) {
                 vnet_hdr = tap_probe_vnet_hdr(fds[i], errp);
@@ -934,18 +922,6 @@ int net_init_tap(const Netdev *netdev, const char *name,
                 goto fail;
             }
         }
-    } else if (tap->helper) {
-        vnet_hdr = tap_probe_vnet_hdr(fds[0], errp);
-        if (vnet_hdr < 0) {
-            goto fail;
-        }
-
-        if (!net_init_tap_one(tap, peer, name, ifname,
-                              NULL, NULL,
-                              vhost_fds ? vhost_fds[0] : -1,
-                              vnet_hdr, fds[0], errp)) {
-            goto fail;
-        }
     } else {
         g_autofree char *script =
             tap_parse_script(tap->script, DEFAULT_NETWORK_SCRIPT);
-- 
2.52.0

[PATCH v3 12/12] net/tap: net_init_tap(): relax QEMU hubs check

[Vladimir Sementsov-Ogievskiy]

queues may be set to 1, as well as fds may contain only one fd.
No reason to block such cases. Let's check exactly number of queues.

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

diff --git a/net/tap.c b/net/tap.c
index 2318158adc..1623ddff5f 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -868,13 +868,6 @@ int net_init_tap(const Netdev *netdev, const char *name,
     assert(netdev->type == NET_CLIENT_DRIVER_TAP);
     tap = &netdev->u.tap;
 
-    /* QEMU hubs do not support multiqueue tap, in this case peer is set.
-     * For -netdev, peer is always NULL. */
-    if (peer && (tap->has_queues || tap->fds || tap->vhostfds)) {
-        error_setg(errp, "Multiqueue tap cannot be used with hubs");
-        return -1;
-    }
-
     if (tap->has_vhost && !tap->vhost && (tap->vhostfds || tap->vhostfd)) {
         error_setg(errp, "vhostfd(s)= is not valid without vhost");
         return -1;
@@ -898,6 +891,15 @@ int net_init_tap(const Netdev *netdev, const char *name,
         return -1;
     }
 
+    /*
+     * QEMU hubs do not support multiqueue tap, in this case peer is set.
+     * For -netdev, peer is always NULL.
+     */
+    if (peer && queues > 1) {
+        error_setg(errp, "Multiqueue tap cannot be used with hubs");
+        goto fail;
+    }
+
     if (!tap_parse_vhost_fds(tap, &vhost_fds, queues, errp)) {
         goto fail;
     }
-- 
2.52.0

Re: [PATCH v3 09/12] net/tap: move fds parameters handling to separate functions

[Chaney, Ben]

On 2/18/26, 3:29 PM, "Vladimir Sementsov-Ogievskiy" <vsementsov@yandex-team.ru <mailto:vsementsov@yandex-team.ru>> wrote:

> + queues = tap_parse_fds_and_queues(tap, &fds, errp);
> + if (queues < 0) {
> + return -1;
> + }
> +

Are we appropriately validating that queues != 0?
I believe we need to throw an error in that case.

Thanks,
        Ben

Re: [PATCH v3 00/12] net: refactoring and fixes

[Chaney, Ben]

> Here are some refactoring and fixes, mostly in net/tap.c I'm making on
> my way to implementing TAP fd migration (through UNIX domain socket).
>
>
> v3:
>
>
> 01: fix checking, switch to int type for queues
> 08: drop MAX_TAP_QUEUES constant
> 09,10: prefer int type for queues

Reviewed-by: Ben Chaney <bchaney@akamai.com>

Re: [PATCH v3 09/12] net/tap: move fds parameters handling to separate functions

[Vladimir Sementsov-Ogievskiy]

On 19.02.26 20:00, Chaney, Ben wrote:
> 
> On 2/18/26, 3:29 PM, "Vladimir Sementsov-Ogievskiy" <vsementsov@yandex-team.ru <mailto:vsementsov@yandex-team.ru>> wrote:
> 
>> + queues = tap_parse_fds_and_queues(tap, &fds, errp);
>> + if (queues < 0) {
>> + return -1;
>> + }
>> +
> 
> Are we appropriately validating that queues != 0?
> I believe we need to throw an error in that case.
> 


Agree. I'll add the check into tap_parse_fds_and_queues().


-- 
Best regards,
Vladimir

Re: [PATCH v3 00/12] net: refactoring and fixes

[Vladimir Sementsov-Ogievskiy]

On 19.02.26 20:00, Chaney, Ben wrote:
>> Here are some refactoring and fixes, mostly in net/tap.c I'm making on
>> my way to implementing TAP fd migration (through UNIX domain socket).
>>
>>
>> v3:
>>
>>
>> 01: fix checking, switch to int type for queues
>> 08: drop MAX_TAP_QUEUES constant
>> 09,10: prefer int type for queues
> 
> Reviewed-by: Ben Chaney <bchaney@akamai.com>
> 

Thanks!!

-- 
Best regards,
Vladimir

Re: [PATCH v3 09/12] net/tap: move fds parameters handling to separate functions

[Chaney, Ben]

On 2/19/26, 1:10 PM, "Vladimir Sementsov-Ogievskiy" <vsementsov@yandex-team.ru <mailto:vsementsov@yandex-team.ru>> wrote:

>On 19.02.26 20:00, Chaney, Ben wrote:
>>
>> On 2/18/26, 3:29 PM, "Vladimir Sementsov-Ogievskiy" <vsementsov@yandex-team.ru <mailto:vsementsov@yandex-team.ru> <mailto:vsementsov@yandex-team.ru <mailto:vsementsov@yandex-team.ru>>> wrote:
>>
>>> + queues = tap_parse_fds_and_queues(tap, &fds, errp);
>>> + if (queues < 0) {
>>> + return -1;
>>> + }
>>> +
>>
>> Are we appropriately validating that queues != 0?
>> I believe we need to throw an error in that case.
>>
> Agree. I'll add the check into tap_parse_fds_and_queues().

That sounds good. You may need a similar check in net_parse_fds to
throw an error on an empty fd list, which would result queues = 0
through a different mechanism.

Thanks,
        Ben

Re: [PATCH v3 08/12] net: introduce net_parse_fds()

[Vladimir Sementsov-Ogievskiy]

On 18.02.26 23:28, Vladimir Sementsov-Ogievskiy wrote:
> +
> +int net_parse_fds(const char *fds_param, int **fds, int expected_nfds,
> +                  Error **errp)
> +{
> +    g_auto(GStrv) fdnames = g_strsplit(fds_param, ":", -1);
> +    unsigned nfds = g_strv_length(fdnames);
> +    int i;
> +
> +    if (nfds > INT_MAX) {
> +        error_setg(errp, "fds parameter exceeds maximum of %d", INT_MAX);
> +        return -1;
> +    }
> +
> +    if (expected_nfds && nfds != expected_nfds) {
> +        error_setg(errp, "expected %u socket fds, got %u", expected_nfds, nfds);
> +        return -1;
> +    }
> +
> +    *fds = g_new(int, nfds);
> +
> +    for (i = 0; i < nfds; i++) {
> +        (*fds)[i] = monitor_fd_param(monitor_cur(), fdnames[i], errp);
> +        if ((*fds)[i] == -1) {
> +            net_free_fds(*fds, i);
> +            *fds = NULL;
> +            return -1;
> +        }
> +    }
> +
> +    return nfds;

Note that I tried to use "int" for queues/nfds consistently, but here
implicit cast unsigned -> int is kept. It seems worse use "int" for
nfds, calling g_strv_length() which returns guint. And we do check
that nfds <= INT_MAX.

> +}


-- 
Best regards,
Vladimir

Re: [PATCH v3 09/12] net/tap: move fds parameters handling to separate functions

[Vladimir Sementsov-Ogievskiy]

On 19.02.26 21:27, Chaney, Ben wrote:
> 
> 
> On 2/19/26, 1:10 PM, "Vladimir Sementsov-Ogievskiy" <vsementsov@yandex-team.ru <mailto:vsementsov@yandex-team.ru>> wrote:
> 
>> On 19.02.26 20:00, Chaney, Ben wrote:
>>>
>>> On 2/18/26, 3:29 PM, "Vladimir Sementsov-Ogievskiy" <vsementsov@yandex-team.ru <mailto:vsementsov@yandex-team.ru> <mailto:vsementsov@yandex-team.ru <mailto:vsementsov@yandex-team.ru>>> wrote:
>>>
>>>> + queues = tap_parse_fds_and_queues(tap, &fds, errp);
>>>> + if (queues < 0) {
>>>> + return -1;
>>>> + }
>>>> +
>>>
>>> Are we appropriately validating that queues != 0?
>>> I believe we need to throw an error in that case.
>>>
>> Agree. I'll add the check into tap_parse_fds_and_queues().
> 
> That sounds good. You may need a similar check in net_parse_fds to
> throw an error on an empty fd list, which would result queues = 0
> through a different mechanism.
> 
> Thanks,
>          Ben
> 

I think, I'll just add a patch on top of the series for this.

-- 
Best regards,
Vladimir

[PATCH 13/12 v3] net/tap: check that user tries to define zero queues

[Vladimir Sementsov-Ogievskiy]

Add check for queues parameter to be non-zero, and for fd/fds
parameters to be non-empty.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 net/tap.c  | 4 ++++
 net/util.c | 5 +++++
 2 files changed, 9 insertions(+)

diff --git a/net/tap.c b/net/tap.c
index 1623ddff5fb..383e158d5e2 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -807,6 +807,10 @@ static int tap_parse_fds_and_queues(const NetdevTapOptions *tap, int **fds,
             error_setg(errp, "queues exceeds maximum %d", INT_MAX);
             return -1;
         }
+        if (tap->queues == 0) {
+            error_setg(errp, "queues must be greater than zero");
+            return -1;
+        }
         queues = tap->queues;
         *fds = NULL;
     } else if (tap->fd || tap->fds) {
diff --git a/net/util.c b/net/util.c
index 1998a6554e0..8265f155484 100644
--- a/net/util.c
+++ b/net/util.c
@@ -94,6 +94,11 @@ int net_parse_fds(const char *fds_param, int **fds, int expected_nfds,
         return -1;
     }
 
+    if (nfds == 0) {
+        error_setg(errp, "no fds passed");
+        return -1;
+    }
+
     *fds = g_new(int, nfds);
 
     for (i = 0; i < nfds; i++) {
-- 
2.52.0

Re: [PATCH 13/12 v3] net/tap: check that user tries to define zero queues

[Chaney, Ben]

On 3/5/26, 2:43 PM, "Vladimir Sementsov-Ogievskiy" <vsementsov@yandex-team.ru <mailto:vsementsov@yandex-team.ru>> wrote:


> Add check for queues parameter to be non-zero, and for fd/fds
> parameters to be non-empty.


LGTM, Thanks!

Ben

Re: [PATCH v3 00/12] net: refactoring and fixes

[Jason Wang]

On Thu, Feb 19, 2026 at 4:28 AM Vladimir Sementsov-Ogievskiy
<vsementsov@yandex-team.ru> wrote:
>
> Hi all!
>
> Here are some refactoring and fixes, mostly in net/tap.c I'm making on
> my way to implementing TAP fd migration (through UNIX domain socket).
>
> v3:
>
> 01: fix checking, switch to int type for queues
> 08: drop MAX_TAP_QUEUES constant
> 09,10: prefer int type for queues
>
>
> Vladimir Sementsov-Ogievskiy (12):
>   net/af-xdp: fix type overflow
>   net/tap: net_init_tap_one(): add return value
>   net/tap: net_init_tap(): drop extra vhostfdname variable
>   net/tap: net_init_tap(): refactor parameter checking
>   net/tap: net_init_tap(): common fail label
>   net/tap: net_init_tap_one() refactor to get vhostfd param
>   net/tap: net_init_tap_one(): drop model parameter
>   net: introduce net_parse_fds()
>   net/tap: move fds parameters handling to separate functions
>   net/tap: fix vhostfds/vhostfd parameters API
>   net/tap: net_init_tap(): merge fd=, fds= and helper= cases into one
>   net/tap: net_init_tap(): relax QEMU hubs check
>
>  net/af-xdp.c |  44 ++-----
>  net/tap.c    | 319 +++++++++++++++++++++------------------------------
>  net/util.c   |  50 ++++++++
>  net/util.h   |  14 +++
>  4 files changed, 201 insertions(+), 226 deletions(-)
>
> --
> 2.52.0
>

If I understand this correctly, there would be a v4 for this?

(https://patchew.org/QEMU/20260218202829.1322088-1-vsementsov@yandex-team.ru/20260218202829.1322088-10-vsementsov@yandex-team.ru/)

Thanks

Re: [PATCH v3 00/12] net: refactoring and fixes

[Vladimir Sementsov-Ogievskiy]

On 12.03.26 05:59, Jason Wang wrote:
> On Thu, Feb 19, 2026 at 4:28 AM Vladimir Sementsov-Ogievskiy
> <vsementsov@yandex-team.ru> wrote:
>>
>> Hi all!
>>
>> Here are some refactoring and fixes, mostly in net/tap.c I'm making on
>> my way to implementing TAP fd migration (through UNIX domain socket).
>>
>> v3:
>>
>> 01: fix checking, switch to int type for queues
>> 08: drop MAX_TAP_QUEUES constant
>> 09,10: prefer int type for queues
>>
>>
>> Vladimir Sementsov-Ogievskiy (12):
>>    net/af-xdp: fix type overflow
>>    net/tap: net_init_tap_one(): add return value
>>    net/tap: net_init_tap(): drop extra vhostfdname variable
>>    net/tap: net_init_tap(): refactor parameter checking
>>    net/tap: net_init_tap(): common fail label
>>    net/tap: net_init_tap_one() refactor to get vhostfd param
>>    net/tap: net_init_tap_one(): drop model parameter
>>    net: introduce net_parse_fds()
>>    net/tap: move fds parameters handling to separate functions
>>    net/tap: fix vhostfds/vhostfd parameters API
>>    net/tap: net_init_tap(): merge fd=, fds= and helper= cases into one
>>    net/tap: net_init_tap(): relax QEMU hubs check
>>
>>   net/af-xdp.c |  44 ++-----
>>   net/tap.c    | 319 +++++++++++++++++++++------------------------------
>>   net/util.c   |  50 ++++++++
>>   net/util.h   |  14 +++
>>   4 files changed, 201 insertions(+), 226 deletions(-)
>>
>> --
>> 2.52.0
>>
> 
> If I understand this correctly, there would be a v4 for this?
> 
> (https://patchew.org/QEMU/20260218202829.1322088-1-vsementsov@yandex-team.ru/20260218202829.1322088-10-vsementsov@yandex-team.ru/)
> 
> Thanks
> 

No, 09/12 kept as is, I just added 13/12 for the check that Ben asked for. I don't think it should be merged somewhere,
as it was preexisting (we did not check for queues==0, and for fds="").

So, no changes is planned now for making a v4.

-- 
Best regards,
Vladimir

Re: [PATCH v3 00/12] net: refactoring and fixes

[Vladimir Sementsov-Ogievskiy]

On 12.03.26 09:37, Vladimir Sementsov-Ogievskiy wrote:
> On 12.03.26 05:59, Jason Wang wrote:
>> On Thu, Feb 19, 2026 at 4:28 AM Vladimir Sementsov-Ogievskiy
>> <vsementsov@yandex-team.ru> wrote:
>>>
>>> Hi all!
>>>
>>> Here are some refactoring and fixes, mostly in net/tap.c I'm making on
>>> my way to implementing TAP fd migration (through UNIX domain socket).
>>>
>>> v3:
>>>
>>> 01: fix checking, switch to int type for queues
>>> 08: drop MAX_TAP_QUEUES constant
>>> 09,10: prefer int type for queues
>>>
>>>
>>> Vladimir Sementsov-Ogievskiy (12):
>>>    net/af-xdp: fix type overflow
>>>    net/tap: net_init_tap_one(): add return value
>>>    net/tap: net_init_tap(): drop extra vhostfdname variable
>>>    net/tap: net_init_tap(): refactor parameter checking
>>>    net/tap: net_init_tap(): common fail label
>>>    net/tap: net_init_tap_one() refactor to get vhostfd param
>>>    net/tap: net_init_tap_one(): drop model parameter
>>>    net: introduce net_parse_fds()
>>>    net/tap: move fds parameters handling to separate functions
>>>    net/tap: fix vhostfds/vhostfd parameters API
>>>    net/tap: net_init_tap(): merge fd=, fds= and helper= cases into one
>>>    net/tap: net_init_tap(): relax QEMU hubs check
>>>
>>>   net/af-xdp.c |  44 ++-----
>>>   net/tap.c    | 319 +++++++++++++++++++++------------------------------
>>>   net/util.c   |  50 ++++++++
>>>   net/util.h   |  14 +++
>>>   4 files changed, 201 insertions(+), 226 deletions(-)
>>>
>>> -- 
>>> 2.52.0
>>>
>>
>> If I understand this correctly, there would be a v4 for this?
>>
>> (https://patchew.org/QEMU/20260218202829.1322088-1-vsementsov@yandex-team.ru/20260218202829.1322088-10-vsementsov@yandex-team.ru/)
>>
>> Thanks
>>
> 
> No, 09/12 kept as is, I just added 13/12 for the check that Ben asked for. I don't think it should be merged somewhere,
> as it was preexisting (we did not check for queues==0, and for fds="").
> 
> So, no changes is planned now for making a v4.
> 

Still no problem to resend, let me know if it's preferable.

-- 
Best regards,
Vladimir

Re: [PATCH v3 10/12] net/tap: fix vhostfds/vhostfd parameters API

[Vladimir Sementsov-Ogievskiy]

On 18.02.26 23:28, Vladimir Sementsov-Ogievskiy wrote:
> +static bool tap_parse_vhost_fds(const NetdevTapOptions *tap, int **vhost_fds,
> +                                int 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) {

reviewing own patches:

that should be "tap->vhostfd ?: tap->vhostfds" of course.

-- 
Best regards,
Vladimir