[PATCH 0/5] net/tap: some fixes and refactorings

[PATCH 0/5] net/tap: some fixes and refactorings

[Vladimir Sementsov-Ogievskiy]

Hi all. I have a work in progress around net/tap (and not sure, will
it be done or we go another way), but some fixes and good refactoring
I'd like to post anyway:

Vladimir Sementsov-Ogievskiy (5):
  net/tap: fix net_init_tap(): set ret on failure path
  net/tap: drop duplicated tap stubs
  net/tap: tap_set_sndbuf(): return status
  net/tap: refactor and improve net_init_tap_one()
  net/tap: net_init_tap_one(): fix net-client leak on failure path

 include/net/vhost_net.h |   3 -
 net/tap_int.h           |   2 +-
 net/tap-bsd.c           |  53 ------------------
 net/tap-linux.c         |   5 +-
 net/tap-open-stub.c     |  34 ++++++++++++
 net/tap-solaris.c       |  53 ------------------
 net/tap-stub.c          |   9 +--
 net/tap.c               | 119 ++++++++++++++++++++++------------------
 net/meson.build         |   3 +
 9 files changed, 108 insertions(+), 173 deletions(-)
 create mode 100644 net/tap-open-stub.c

-- 
2.28.0

[PATCH 1/5] net/tap: fix net_init_tap(): set ret on failure path

[Vladimir Sementsov-Ogievskiy]

We should set ret to -1 on this failure path.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 net/tap.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/tap.c b/net/tap.c
index b7512853f4..75b01d54ee 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -880,6 +880,7 @@ int net_init_tap(const Netdev *netdev, const char *name,
             if (i == 0) {
                 vnet_hdr = tap_probe_vnet_hdr(fd, errp);
                 if (vnet_hdr < 0) {
+                    ret = -1;
                     goto free_fail;
                 }
             } else if (vnet_hdr != tap_probe_vnet_hdr(fd, NULL)) {
-- 
2.28.0

[PATCH 2/5] net/tap: drop duplicated tap stubs

[Vladimir Sementsov-Ogievskiy]

tap-solaris and tap-bsd realize only tap_open() interface and all other
functions are duplicated with tap-stub. Let's split tap-stub so that we
can reuse all stubs except for tap_open() for solaris and bsd.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 net/tap-bsd.c       | 53 ---------------------------------------------
 net/tap-open-stub.c | 34 +++++++++++++++++++++++++++++
 net/tap-solaris.c   | 53 ---------------------------------------------
 net/tap-stub.c      |  7 ------
 net/meson.build     |  3 +++
 5 files changed, 37 insertions(+), 113 deletions(-)
 create mode 100644 net/tap-open-stub.c

diff --git a/net/tap-bsd.c b/net/tap-bsd.c
index 77aaf674b1..5667e87c9a 100644
--- a/net/tap-bsd.c
+++ b/net/tap-bsd.c
@@ -206,56 +206,3 @@ error:
     return -1;
 }
 #endif /* __FreeBSD__ */
-
-void tap_set_sndbuf(int fd, const NetdevTapOptions *tap, Error **errp)
-{
-}
-
-int tap_probe_vnet_hdr(int fd, Error **errp)
-{
-    return 0;
-}
-
-int tap_probe_has_ufo(int fd)
-{
-    return 0;
-}
-
-int tap_probe_vnet_hdr_len(int fd, int len)
-{
-    return 0;
-}
-
-void tap_fd_set_vnet_hdr_len(int fd, int len)
-{
-}
-
-int tap_fd_set_vnet_le(int fd, int is_le)
-{
-    return -EINVAL;
-}
-
-int tap_fd_set_vnet_be(int fd, int is_be)
-{
-    return -EINVAL;
-}
-
-void tap_fd_set_offload(int fd, int csum, int tso4,
-                        int tso6, int ecn, int ufo)
-{
-}
-
-int tap_fd_enable(int fd)
-{
-    return -1;
-}
-
-int tap_fd_disable(int fd)
-{
-    return -1;
-}
-
-int tap_fd_get_ifname(int fd, char *ifname)
-{
-    return -1;
-}
diff --git a/net/tap-open-stub.c b/net/tap-open-stub.c
new file mode 100644
index 0000000000..01cc03f630
--- /dev/null
+++ b/net/tap-open-stub.c
@@ -0,0 +1,34 @@
+/*
+ * QEMU System Emulator
+ *
+ * Copyright (c) 2003-2008 Fabrice Bellard
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "tap_int.h"
+
+int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
+             int vnet_hdr_required, int mq_required, Error **errp)
+{
+    error_setg(errp, "tap is not supported in this build");
+    return -1;
+}
diff --git a/net/tap-solaris.c b/net/tap-solaris.c
index 0475a58207..920b4df2fe 100644
--- a/net/tap-solaris.c
+++ b/net/tap-solaris.c
@@ -202,56 +202,3 @@ int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
     fcntl(fd, F_SETFL, O_NONBLOCK);
     return fd;
 }
-
-void tap_set_sndbuf(int fd, const NetdevTapOptions *tap, Error **errp)
-{
-}
-
-int tap_probe_vnet_hdr(int fd, Error **errp)
-{
-    return 0;
-}
-
-int tap_probe_has_ufo(int fd)
-{
-    return 0;
-}
-
-int tap_probe_vnet_hdr_len(int fd, int len)
-{
-    return 0;
-}
-
-void tap_fd_set_vnet_hdr_len(int fd, int len)
-{
-}
-
-int tap_fd_set_vnet_le(int fd, int is_le)
-{
-    return -EINVAL;
-}
-
-int tap_fd_set_vnet_be(int fd, int is_be)
-{
-    return -EINVAL;
-}
-
-void tap_fd_set_offload(int fd, int csum, int tso4,
-                        int tso6, int ecn, int ufo)
-{
-}
-
-int tap_fd_enable(int fd)
-{
-    return -1;
-}
-
-int tap_fd_disable(int fd)
-{
-    return -1;
-}
-
-int tap_fd_get_ifname(int fd, char *ifname)
-{
-    return -1;
-}
diff --git a/net/tap-stub.c b/net/tap-stub.c
index de525a2e69..6fa130758b 100644
--- a/net/tap-stub.c
+++ b/net/tap-stub.c
@@ -26,13 +26,6 @@
 #include "qapi/error.h"
 #include "tap_int.h"
 
-int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
-             int vnet_hdr_required, int mq_required, Error **errp)
-{
-    error_setg(errp, "tap is not supported in this build");
-    return -1;
-}
-
 void tap_set_sndbuf(int fd, const NetdevTapOptions *tap, Error **errp)
 {
 }
diff --git a/net/meson.build b/net/meson.build
index 1076b0a7ab..73b2ab78c2 100644
--- a/net/meson.build
+++ b/net/meson.build
@@ -32,6 +32,9 @@ softmmu_ss.add(when: 'CONFIG_BSD', if_true: files('tap-bsd.c'))
 softmmu_ss.add(when: 'CONFIG_SOLARIS', if_true: files('tap-solaris.c'))
 tap_posix = ['tap.c']
 if not config_host.has_key('CONFIG_LINUX') and not config_host.has_key('CONFIG_BSD') and not config_host.has_key('CONFIG_SOLARIS')
+  tap_posix += 'tap-open-stub.c'
+endif
+if not config_host.has_key('CONFIG_LINUX')
   tap_posix += 'tap-stub.c'
 endif
 softmmu_ss.add(when: 'CONFIG_POSIX', if_true: files(tap_posix))
-- 
2.28.0

[PATCH 3/5] net/tap: tap_set_sndbuf(): return status

[Vladimir Sementsov-Ogievskiy]

It's recommended to return a value indicating success / failure for
functions with errp parameter (see include/qapi/error.h). Let's update
tap_set_sndbuf().

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 net/tap_int.h   | 2 +-
 net/tap-linux.c | 5 ++++-
 net/tap-stub.c  | 2 +-
 net/tap.c       | 6 +++---
 4 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/net/tap_int.h b/net/tap_int.h
index 225a49ea48..57567b9f24 100644
--- a/net/tap_int.h
+++ b/net/tap_int.h
@@ -33,7 +33,7 @@ int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
 
 ssize_t tap_read_packet(int tapfd, uint8_t *buf, int maxlen);
 
-void tap_set_sndbuf(int fd, const NetdevTapOptions *tap, Error **errp);
+int tap_set_sndbuf(int fd, const NetdevTapOptions *tap, Error **errp);
 int tap_probe_vnet_hdr(int fd, Error **errp);
 int tap_probe_vnet_hdr_len(int fd, int len);
 int tap_probe_has_ufo(int fd);
diff --git a/net/tap-linux.c b/net/tap-linux.c
index b0635e9e32..c51bcdc2a3 100644
--- a/net/tap-linux.c
+++ b/net/tap-linux.c
@@ -130,7 +130,7 @@ int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
  */
 #define TAP_DEFAULT_SNDBUF 0
 
-void tap_set_sndbuf(int fd, const NetdevTapOptions *tap, Error **errp)
+int tap_set_sndbuf(int fd, const NetdevTapOptions *tap, Error **errp)
 {
     int sndbuf;
 
@@ -144,7 +144,10 @@ void tap_set_sndbuf(int fd, const NetdevTapOptions *tap, Error **errp)
 
     if (ioctl(fd, TUNSETSNDBUF, &sndbuf) == -1 && tap->has_sndbuf) {
         error_setg_errno(errp, errno, "TUNSETSNDBUF ioctl failed");
+        return -1;
     }
+
+    return 0;
 }
 
 int tap_probe_vnet_hdr(int fd, Error **errp)
diff --git a/net/tap-stub.c b/net/tap-stub.c
index 6fa130758b..473d5e4afe 100644
--- a/net/tap-stub.c
+++ b/net/tap-stub.c
@@ -26,7 +26,7 @@
 #include "qapi/error.h"
 #include "tap_int.h"
 
-void tap_set_sndbuf(int fd, const NetdevTapOptions *tap, Error **errp)
+int tap_set_sndbuf(int fd, const NetdevTapOptions *tap, Error **errp)
 {
 }
 
diff --git a/net/tap.c b/net/tap.c
index 75b01d54ee..33d749c7b6 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -661,10 +661,10 @@ static void net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
     Error *err = NULL;
     TAPState *s = net_tap_fd_init(peer, model, name, fd, vnet_hdr);
     int vhostfd;
+    int ret;
 
-    tap_set_sndbuf(s->fd, tap, &err);
-    if (err) {
-        error_propagate(errp, err);
+    ret = tap_set_sndbuf(s->fd, tap, errp);
+    if (ret < 0) {
         return;
     }
 
-- 
2.28.0

[PATCH 4/5] net/tap: refactor and improve net_init_tap_one()

[Vladimir Sementsov-Ogievskiy]

First, 46d4d36d0bf2b24 changed the logic of net_init_tap_one() so
that failure of vhost initialization is ignored (still, warning
printed) when vhostforce is not set.

Then 894022e616016fe8 updated net_init_tap_one() to use
qemu_try_set_nonblock() instead of qemu_set_nonblock(), to not crash,
but it didn't consider the failure-ignoring logic and used wrong fd for
error message.

Let's refactor the function net_init_tap_one() splitting out vhost-net
initialization and fixing all the issues.

Error message change: we need to fix fd anyway. Also, don't want to
pass name to the new function only for error path, so, drop it. As a
pay, add more information on vhost fd itself.

Fixes: 894022e616016fe81745753f14adfbd680a1c7ee
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 include/net/vhost_net.h |   3 --
 net/tap.c               | 103 +++++++++++++++++++++-------------------
 2 files changed, 53 insertions(+), 53 deletions(-)

diff --git a/include/net/vhost_net.h b/include/net/vhost_net.h
index 172b0051d8..c00871cd1b 100644
--- a/include/net/vhost_net.h
+++ b/include/net/vhost_net.h
@@ -4,9 +4,6 @@
 #include "net/net.h"
 #include "hw/virtio/vhost-backend.h"
 
-#define VHOST_NET_INIT_FAILED \
-    "vhost-net requested but could not be initialized"
-
 struct vhost_net;
 typedef struct vhost_net VHostNetState;
 
diff --git a/net/tap.c b/net/tap.c
index 33d749c7b6..89ea04862b 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -652,15 +652,61 @@ static int net_tap_init(const NetdevTapOptions *tap, int *vnet_hdr,
 
 #define MAX_TAP_QUEUES 1024
 
+static int tap_init_vhost_net(TAPState *s, const NetdevTapOptions *tap,
+                              const char *vhostfdname, Error **errp)
+{
+    VhostNetOptions options;
+    int vhostfd;
+    int ret;
+
+    options.backend_type = VHOST_BACKEND_TYPE_KERNEL;
+    options.net_backend = &s->nc;
+    if (tap->has_poll_us) {
+        options.busyloop_timeout = tap->poll_us;
+    } else {
+        options.busyloop_timeout = 0;
+    }
+
+    if (vhostfdname) {
+        vhostfd = monitor_fd_param(monitor_cur(), vhostfdname, errp);
+        if (vhostfd < 0) {
+            return -1;
+        }
+
+        ret = qemu_try_set_nonblock(vhostfd);
+        if (ret < 0) {
+            error_setg_errno(errp, -ret,
+                             "tap: Can't use vhost file descriptor %s(%d)",
+                             vhostfdname, vhostfd);
+            return -1;
+        }
+    } else {
+        vhostfd = open("/dev/vhost-net", O_RDWR);
+        if (vhostfd < 0) {
+            error_setg_errno(errp, errno, "tap: open vhost char device failed");
+            return -1;
+        }
+        qemu_set_nonblock(vhostfd);
+    }
+
+    options.opaque = (void *)(uintptr_t)vhostfd;
+
+    s->vhost_net = vhost_net_init(&options);
+    if (!s->vhost_net) {
+        error_setg(errp, "vhost-net requested but could not be initialized");
+        return -1;
+    }
+
+    return 0;
+}
+
 static void 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,
                              int vnet_hdr, int fd, Error **errp)
 {
-    Error *err = NULL;
     TAPState *s = net_tap_fd_init(peer, model, name, fd, vnet_hdr);
-    int vhostfd;
     int ret;
 
     ret = tap_set_sndbuf(s->fd, tap, errp);
@@ -687,58 +733,15 @@ static void net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
 
     if (tap->has_vhost ? tap->vhost :
         vhostfdname || (tap->has_vhostforce && tap->vhostforce)) {
-        VhostNetOptions options;
-
-        options.backend_type = VHOST_BACKEND_TYPE_KERNEL;
-        options.net_backend = &s->nc;
-        if (tap->has_poll_us) {
-            options.busyloop_timeout = tap->poll_us;
-        } else {
-            options.busyloop_timeout = 0;
-        }
-
-        if (vhostfdname) {
-            int ret;
-
-            vhostfd = monitor_fd_param(monitor_cur(), vhostfdname, &err);
-            if (vhostfd == -1) {
-                if (tap->has_vhostforce && tap->vhostforce) {
-                    error_propagate(errp, err);
-                } else {
-                    warn_report_err(err);
-                }
-                return;
-            }
-            ret = qemu_try_set_nonblock(vhostfd);
-            if (ret < 0) {
-                error_setg_errno(errp, -ret, "%s: Can't use file descriptor %d",
-                                 name, fd);
-                return;
-            }
-        } else {
-            vhostfd = open("/dev/vhost-net", O_RDWR);
-            if (vhostfd < 0) {
-                if (tap->has_vhostforce && tap->vhostforce) {
-                    error_setg_errno(errp, errno,
-                                     "tap: open vhost char device failed");
-                } else {
-                    warn_report("tap: open vhost char device failed: %s",
-                                strerror(errno));
-                }
-                return;
-            }
-            qemu_set_nonblock(vhostfd);
-        }
-        options.opaque = (void *)(uintptr_t)vhostfd;
+        Error *err = NULL;
 
-        s->vhost_net = vhost_net_init(&options);
-        if (!s->vhost_net) {
+        ret = tap_init_vhost_net(s, tap, vhostfdname, &err);
+        if (ret < 0) {
             if (tap->has_vhostforce && tap->vhostforce) {
-                error_setg(errp, VHOST_NET_INIT_FAILED);
+                error_propagate(errp, err);
             } else {
-                warn_report(VHOST_NET_INIT_FAILED);
+                warn_report_err(err);
             }
-            return;
         }
     } else if (vhostfdname) {
         error_setg(errp, "vhostfd(s)= is not valid without vhost");
-- 
2.28.0

[PATCH 5/5] net/tap: net_init_tap_one(): fix net-client leak on failure path

[Vladimir Sementsov-Ogievskiy]

net_tap_fd_init() allocates new NetClientState through
qemu_new_net_client(). We should free it on failure path.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---

Attention: it's an intuitive patch.

I see, that net-client is leaked. May be it's still freed some tricky
way? And I don't understand the whole logic of qemu_del_net_client(),
it's just the only public interface to free net-client I found.

 net/tap.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/net/tap.c b/net/tap.c
index 89ea04862b..ba4c34af3d 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -711,7 +711,7 @@ static void net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
 
     ret = tap_set_sndbuf(s->fd, tap, errp);
     if (ret < 0) {
-        return;
+        goto fail;
     }
 
     if (tap->has_fd || tap->has_fds) {
@@ -739,13 +739,20 @@ static void net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
         if (ret < 0) {
             if (tap->has_vhostforce && tap->vhostforce) {
                 error_propagate(errp, err);
+                goto fail;
             } else {
                 warn_report_err(err);
             }
         }
     } else if (vhostfdname) {
         error_setg(errp, "vhostfd(s)= is not valid without vhost");
+        goto fail;
     }
+
+    return;
+
+fail:
+    qemu_del_net_client(&s->nc);
 }
 
 static int get_fds(char *str, char *fds[], int max)
-- 
2.28.0

Re: [PATCH 3/5] net/tap: tap_set_sndbuf(): return status

[Philippe Mathieu-Daudé]

On 12/21/20 8:06 PM, Vladimir Sementsov-Ogievskiy wrote:
> It's recommended to return a value indicating success / failure for
> functions with errp parameter (see include/qapi/error.h). Let's update
> tap_set_sndbuf().

For simple "success/failure" a bool type is enough.

Preferably using bool type:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  net/tap_int.h   | 2 +-
>  net/tap-linux.c | 5 ++++-
>  net/tap-stub.c  | 2 +-
>  net/tap.c       | 6 +++---
>  4 files changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/net/tap_int.h b/net/tap_int.h
> index 225a49ea48..57567b9f24 100644
> --- a/net/tap_int.h
> +++ b/net/tap_int.h
> @@ -33,7 +33,7 @@ int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
>  
>  ssize_t tap_read_packet(int tapfd, uint8_t *buf, int maxlen);
>  
> -void tap_set_sndbuf(int fd, const NetdevTapOptions *tap, Error **errp);
> +int tap_set_sndbuf(int fd, const NetdevTapOptions *tap, Error **errp);
>  int tap_probe_vnet_hdr(int fd, Error **errp);
>  int tap_probe_vnet_hdr_len(int fd, int len);
>  int tap_probe_has_ufo(int fd);
> diff --git a/net/tap-linux.c b/net/tap-linux.c
> index b0635e9e32..c51bcdc2a3 100644
> --- a/net/tap-linux.c
> +++ b/net/tap-linux.c
> @@ -130,7 +130,7 @@ int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
>   */
>  #define TAP_DEFAULT_SNDBUF 0
>  
> -void tap_set_sndbuf(int fd, const NetdevTapOptions *tap, Error **errp)
> +int tap_set_sndbuf(int fd, const NetdevTapOptions *tap, Error **errp)
>  {
>      int sndbuf;
>  
> @@ -144,7 +144,10 @@ void tap_set_sndbuf(int fd, const NetdevTapOptions *tap, Error **errp)
>  
>      if (ioctl(fd, TUNSETSNDBUF, &sndbuf) == -1 && tap->has_sndbuf) {
>          error_setg_errno(errp, errno, "TUNSETSNDBUF ioctl failed");
> +        return -1;
>      }
> +
> +    return 0;
>  }
>  
>  int tap_probe_vnet_hdr(int fd, Error **errp)
> diff --git a/net/tap-stub.c b/net/tap-stub.c
> index 6fa130758b..473d5e4afe 100644
> --- a/net/tap-stub.c
> +++ b/net/tap-stub.c
> @@ -26,7 +26,7 @@
>  #include "qapi/error.h"
>  #include "tap_int.h"
>  
> -void tap_set_sndbuf(int fd, const NetdevTapOptions *tap, Error **errp)
> +int tap_set_sndbuf(int fd, const NetdevTapOptions *tap, Error **errp)
>  {
>  }
>  
> diff --git a/net/tap.c b/net/tap.c
> index 75b01d54ee..33d749c7b6 100644
> --- a/net/tap.c
> +++ b/net/tap.c
> @@ -661,10 +661,10 @@ static void net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
>      Error *err = NULL;
>      TAPState *s = net_tap_fd_init(peer, model, name, fd, vnet_hdr);
>      int vhostfd;
> +    int ret;
>  
> -    tap_set_sndbuf(s->fd, tap, &err);
> -    if (err) {
> -        error_propagate(errp, err);
> +    ret = tap_set_sndbuf(s->fd, tap, errp);
> +    if (ret < 0) {
>          return;
>      }
>  
> 

Re: [PATCH 3/5] net/tap: tap_set_sndbuf(): return status

[Vladimir Sementsov-Ogievskiy]

21.12.2020 23:12, Philippe Mathieu-Daudé wrote:
> On 12/21/20 8:06 PM, Vladimir Sementsov-Ogievskiy wrote:
>> It's recommended to return a value indicating success / failure for
>> functions with errp parameter (see include/qapi/error.h). Let's update
>> tap_set_sndbuf().
> 
> For simple "success/failure" a bool type is enough.
> 
> Preferably using bool type:
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

0/-1 return status is highly used in net/tap, so I decided to follow it.

> 
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   net/tap_int.h   | 2 +-
>>   net/tap-linux.c | 5 ++++-
>>   net/tap-stub.c  | 2 +-
>>   net/tap.c       | 6 +++---
>>   4 files changed, 9 insertions(+), 6 deletions(-)
>>
>> diff --git a/net/tap_int.h b/net/tap_int.h
>> index 225a49ea48..57567b9f24 100644
>> --- a/net/tap_int.h
>> +++ b/net/tap_int.h
>> @@ -33,7 +33,7 @@ int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
>>   
>>   ssize_t tap_read_packet(int tapfd, uint8_t *buf, int maxlen);
>>   
>> -void tap_set_sndbuf(int fd, const NetdevTapOptions *tap, Error **errp);
>> +int tap_set_sndbuf(int fd, const NetdevTapOptions *tap, Error **errp);
>>   int tap_probe_vnet_hdr(int fd, Error **errp);
>>   int tap_probe_vnet_hdr_len(int fd, int len);
>>   int tap_probe_has_ufo(int fd);
>> diff --git a/net/tap-linux.c b/net/tap-linux.c
>> index b0635e9e32..c51bcdc2a3 100644
>> --- a/net/tap-linux.c
>> +++ b/net/tap-linux.c
>> @@ -130,7 +130,7 @@ int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
>>    */
>>   #define TAP_DEFAULT_SNDBUF 0
>>   
>> -void tap_set_sndbuf(int fd, const NetdevTapOptions *tap, Error **errp)
>> +int tap_set_sndbuf(int fd, const NetdevTapOptions *tap, Error **errp)
>>   {
>>       int sndbuf;
>>   
>> @@ -144,7 +144,10 @@ void tap_set_sndbuf(int fd, const NetdevTapOptions *tap, Error **errp)
>>   
>>       if (ioctl(fd, TUNSETSNDBUF, &sndbuf) == -1 && tap->has_sndbuf) {
>>           error_setg_errno(errp, errno, "TUNSETSNDBUF ioctl failed");
>> +        return -1;
>>       }
>> +
>> +    return 0;
>>   }
>>   
>>   int tap_probe_vnet_hdr(int fd, Error **errp)
>> diff --git a/net/tap-stub.c b/net/tap-stub.c
>> index 6fa130758b..473d5e4afe 100644
>> --- a/net/tap-stub.c
>> +++ b/net/tap-stub.c
>> @@ -26,7 +26,7 @@
>>   #include "qapi/error.h"
>>   #include "tap_int.h"
>>   
>> -void tap_set_sndbuf(int fd, const NetdevTapOptions *tap, Error **errp)
>> +int tap_set_sndbuf(int fd, const NetdevTapOptions *tap, Error **errp)
>>   {
>>   }
>>   
>> diff --git a/net/tap.c b/net/tap.c
>> index 75b01d54ee..33d749c7b6 100644
>> --- a/net/tap.c
>> +++ b/net/tap.c
>> @@ -661,10 +661,10 @@ static void net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
>>       Error *err = NULL;
>>       TAPState *s = net_tap_fd_init(peer, model, name, fd, vnet_hdr);
>>       int vhostfd;
>> +    int ret;
>>   
>> -    tap_set_sndbuf(s->fd, tap, &err);
>> -    if (err) {
>> -        error_propagate(errp, err);
>> +    ret = tap_set_sndbuf(s->fd, tap, errp);
>> +    if (ret < 0) {
>>           return;
>>       }
>>   
>>
> 


-- 
Best regards,
Vladimir

Re: [PATCH 0/5] net/tap: some fixes and refactorings

[Vladimir Sementsov-Ogievskiy]

ping :)

21.12.2020 22:06, Vladimir Sementsov-Ogievskiy wrote:
> Hi all. I have a work in progress around net/tap (and not sure, will
> it be done or we go another way), but some fixes and good refactoring
> I'd like to post anyway:
> 
> Vladimir Sementsov-Ogievskiy (5):
>    net/tap: fix net_init_tap(): set ret on failure path
>    net/tap: drop duplicated tap stubs
>    net/tap: tap_set_sndbuf(): return status
>    net/tap: refactor and improve net_init_tap_one()
>    net/tap: net_init_tap_one(): fix net-client leak on failure path
> 
>   include/net/vhost_net.h |   3 -
>   net/tap_int.h           |   2 +-
>   net/tap-bsd.c           |  53 ------------------
>   net/tap-linux.c         |   5 +-
>   net/tap-open-stub.c     |  34 ++++++++++++
>   net/tap-solaris.c       |  53 ------------------
>   net/tap-stub.c          |   9 +--
>   net/tap.c               | 119 ++++++++++++++++++++++------------------
>   net/meson.build         |   3 +
>   9 files changed, 108 insertions(+), 173 deletions(-)
>   create mode 100644 net/tap-open-stub.c
> 


-- 
Best regards,
Vladimir

Re: [PATCH 5/5] net/tap: net_init_tap_one(): fix net-client leak on failure path

[Jason Wang]

On 2020/12/22 上午3:06, Vladimir Sementsov-Ogievskiy wrote:
> net_tap_fd_init() allocates new NetClientState through
> qemu_new_net_client(). We should free it on failure path.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>
> Attention: it's an intuitive patch.
>
> I see, that net-client is leaked. May be it's still freed some tricky
> way? And I don't understand the whole logic of qemu_del_net_client(),
> it's just the only public interface to free net-client I found.


Your patch looks correct and it's indeed a leak.

I wonder whether it's better to do the cleanup in the free_fail label in 
net_init_tap(). The reason is that we need deal with case of multiqueue. 
Though qemu_del_net_client() can handle this but it's not clear if we do 
it in net_init_tap_one().

Thanks


>
>   net/tap.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/net/tap.c b/net/tap.c
> index 89ea04862b..ba4c34af3d 100644
> --- a/net/tap.c
> +++ b/net/tap.c
> @@ -711,7 +711,7 @@ static void net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
>   
>       ret = tap_set_sndbuf(s->fd, tap, errp);
>       if (ret < 0) {
> -        return;
> +        goto fail;
>       }
>   
>       if (tap->has_fd || tap->has_fds) {
> @@ -739,13 +739,20 @@ static void net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
>           if (ret < 0) {
>               if (tap->has_vhostforce && tap->vhostforce) {
>                   error_propagate(errp, err);
> +                goto fail;
>               } else {
>                   warn_report_err(err);
>               }
>           }
>       } else if (vhostfdname) {
>           error_setg(errp, "vhostfd(s)= is not valid without vhost");
> +        goto fail;
>       }
> +
> +    return;
> +
> +fail:
> +    qemu_del_net_client(&s->nc);
>   }
>   
>   static int get_fds(char *str, char *fds[], int max)

Re: [PATCH 5/5] net/tap: net_init_tap_one(): fix net-client leak on failure path

[Vladimir Sementsov-Ogievskiy]

12.01.2021 07:53, Jason Wang wrote:
> 
> On 2020/12/22 上午3:06, Vladimir Sementsov-Ogievskiy wrote:
>> net_tap_fd_init() allocates new NetClientState through
>> qemu_new_net_client(). We should free it on failure path.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>
>> Attention: it's an intuitive patch.
>>
>> I see, that net-client is leaked. May be it's still freed some tricky
>> way? And I don't understand the whole logic of qemu_del_net_client(),
>> it's just the only public interface to free net-client I found.
> 
> 
> Your patch looks correct and it's indeed a leak.
> 
> I wonder whether it's better to do the cleanup in the free_fail label in net_init_tap(). The reason is that we need deal with case of multiqueue. Though qemu_del_net_client() can handle this but it's not clear if we do it in net_init_tap_one().
> 

Sorry for so long delay :(

Now I'm thinking about reviving this series. But I don't understand what you mean about multiqueue.. I think, if some function allocates a resource, we should release the resource on failure path in this function, not in the caller. Good functions tries to roll-back any visible changes on failure.. What am I missing?

>>
>>   net/tap.c | 9 ++++++++-
>>   1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/tap.c b/net/tap.c
>> index 89ea04862b..ba4c34af3d 100644
>> --- a/net/tap.c
>> +++ b/net/tap.c
>> @@ -711,7 +711,7 @@ static void net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
>>       ret = tap_set_sndbuf(s->fd, tap, errp);
>>       if (ret < 0) {
>> -        return;
>> +        goto fail;
>>       }
>>       if (tap->has_fd || tap->has_fds) {
>> @@ -739,13 +739,20 @@ static void net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
>>           if (ret < 0) {
>>               if (tap->has_vhostforce && tap->vhostforce) {
>>                   error_propagate(errp, err);
>> +                goto fail;
>>               } else {
>>                   warn_report_err(err);
>>               }
>>           }
>>       } else if (vhostfdname) {
>>           error_setg(errp, "vhostfd(s)= is not valid without vhost");
>> +        goto fail;
>>       }
>> +
>> +    return;
>> +
>> +fail:
>> +    qemu_del_net_client(&s->nc);
>>   }
>>   static int get_fds(char *str, char *fds[], int max)
> 


-- 
Best regards,
Vladimir