Re: [PATCH] tap: fix net_init_tap() return code

[Philippe Mathieu-Daudé]

On 4/4/23 18:00, Steve Sistare wrote:
> When net_init_tap() succeeds for a multi-queue device, it returns a
> non-zero ret=1 code to its caller, because of this code where ret becomes

Indeed g_unix_set_fd_nonblocking() returns TRUE on success.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

> 1 when g_unix_set_fd_nonblocking succeeds.  Luckily, the only current call
> site checks for negative, rather than non-zero.
>
>      ret = g_unix_set_fd_nonblocking(fd, true, NULL);
>      if (!ret) {
>          ...
>          goto free_fail;
>
> Also, if g_unix_set_fd_nonblocking fails (though unlikely), ret=0 is returned,
> and the caller will use a broken interface.

We should return -1 from free_fail, not trying to propagate 'ret':

-- >8 --
diff --git a/net/tap.c b/net/tap.c
index 1bf085d422..e59238bda0 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -821,3 +821,2 @@ int net_init_tap(const Netdev *netdev, const char *name,
     char ifname[128];
-    int ret = 0;

@@ -896,3 +895,2 @@ int net_init_tap(const Netdev *netdev, const char *name,
                            "the number of vhostfds passed");
-                ret = -1;
                 goto free_fail;
@@ -904,3 +902,2 @@ int net_init_tap(const Netdev *netdev, const char *name,
             if (fd == -1) {
-                ret = -1;
                 goto free_fail;
@@ -908,4 +905,3 @@ int net_init_tap(const Netdev *netdev, const char *name,

-            ret = g_unix_set_fd_nonblocking(fd, true, NULL);
-            if (!ret) {
+            if (!g_unix_set_fd_nonblocking(fd, true, NULL)) {
                 error_setg_errno(errp, errno, "%s: Can't use file 
descriptor %d",
@@ -918,3 +914,2 @@ int net_init_tap(const Netdev *netdev, const char *name,
                 if (vnet_hdr < 0) {
-                    ret = -1;
                     goto free_fail;
@@ -924,3 +919,2 @@ int net_init_tap(const Netdev *netdev, const char *name,
                            "vnet_hdr not consistent across given tap 
fds");
-                ret = -1;
                 goto free_fail;
@@ -934,3 +928,2 @@ int net_init_tap(const Netdev *netdev, const char *name,
                 error_propagate(errp, err);
-                ret = -1;
                 goto free_fail;
@@ -948,3 +941,3 @@ free_fail:
         g_free(vhost_fds);
-        return ret;
+        return -1;
     } else if (tap->helper) {
---

> Fixes: a8208626ba89.. ("net: replace qemu_set_nonblock()")
> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> ---
>   net/tap.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)