Hi Jason,

On 2018/2/2 14:56, Jason Wang wrote:



On 2018年02月02日 11:53, Jay Zhou wrote:
Hi Jason,

On 2018/2/2 11:11, Jason Wang wrote:


On 2018年01月26日 11:08, Jay Zhou wrote:
If netdev_add tap,id=net0,...,vhost=on failed in net_init_tap_one(),
the followed up device_add virtio-net-pci,netdev=net0 will fail
too, prints:

   TUNSETOFFLOAD ioctl() failed: Bad file descriptor TUNSETOFFLOAD
   ioctl() failed: Bad file descriptor

The reason is that the fd of tap is closed when error occured after
calling net_init_tap_one().

The fd should be closed when calling net_init_tap_one failed:
   - if tap_set_sndbuf() failed
   - if tap_set_sndbuf() succeeded but vhost failed to initialize with
     vhostforce flag on
The fd should not be closed just because vhost failed to initialize
but without vhostforce flag. So the followed up device_add can fall
back to userspace virtio successfully.

Suggested-by: Michael S. Tsirkin <m...@redhat.com>
Suggested-by: Igor Mammedov <imamm...@redhat.com>
Suggested-by: Jason Wang <jasow...@redhat.com>
Signed-off-by: Jay Zhou <jianjay.z...@huawei.com>
---
  net/tap.c | 40 ++++++++++++++++++++++++++++++++--------
  1 file changed, 32 insertions(+), 8 deletions(-)

diff --git a/net/tap.c b/net/tap.c
index 979e622..8042c7d 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -648,12 +648,6 @@ static void net_init_tap_one(const NetdevTapOptions
*tap, NetClientState *peer,
      TAPState *s = net_tap_fd_init(peer, model, name, fd, vnet_hdr);
      int vhostfd;
-    tap_set_sndbuf(s->fd, tap, &err);
-    if (err) {
-        error_propagate(errp, err);
-        return;
-    }
-
      if (tap->has_fd || tap->has_fds) {
          snprintf(s->nc.info_str, sizeof(s->nc.info_str), "fd=%d", fd);
      } else if (tap->has_helper) {
@@ -781,6 +775,12 @@ int net_init_tap(const Netdev *netdev, const char *name,
          vnet_hdr = tap_probe_vnet_hdr(fd);
+        tap_set_sndbuf(fd, tap, &err);
+        if (err) {
+            error_propagate(errp, err);
+            return -1;
+        }
+
          net_init_tap_one(tap, peer, "tap", name, NULL,
                           script, downscript,
                           vhostfdname, vnet_hdr, fd, &err);
@@ -832,6 +832,12 @@ int net_init_tap(const Netdev *netdev, const char *name,
                  goto free_fail;
              }
+            tap_set_sndbuf(fd, tap, &err);
+            if (err) {
+                error_propagate(errp, err);
+                goto free_fail;
+            }
+
              net_init_tap_one(tap, peer, "tap", name, ifname,
                               script, downscript,
                               tap->has_vhostfds ? vhost_fds[i] : NULL,
@@ -872,12 +878,21 @@ free_fail:
          fcntl(fd, F_SETFL, O_NONBLOCK);
          vnet_hdr = tap_probe_vnet_hdr(fd);
+        tap_set_sndbuf(fd, tap, &err);
+        if (err) {
+            error_propagate(errp, err);
+            close(fd);
+            return -1;
+        }
+
          net_init_tap_one(tap, peer, "bridge", name, ifname,
                           script, downscript, vhostfdname,
                           vnet_hdr, fd, &err);
          if (err) {
              error_propagate(errp, err);
-            close(fd);
+            if (tap->has_vhostforce && tap->vhostforce) {
+                close(fd);
+            }
              return -1;
          }
      } else {
@@ -910,13 +925,22 @@ free_fail:
                  }
              }
+            tap_set_sndbuf(fd, tap, &err);
+            if (err) {
+                error_propagate(errp, err);
+                close(fd);
+                return -1;
+            }
+
              net_init_tap_one(tap, peer, "tap", name, ifname,
                               i >= 1 ? "no" : script,
                               i >= 1 ? "no" : downscript,
                               vhostfdname, vnet_hdr, fd, &err);
              if (err) {
                  error_propagate(errp, err);
-                close(fd);
+                if (tap->has_vhostforce && tap->vhostforce) {
+                    close(fd);
+                }
                  return -1;
              }
          }

Hi:

I still fail to understand why not just pass force flag to net_tap_init_one(),
and let it decide?

I'm a little confused here, as you suggested in version 1:

https://lists.gnu.org/archive/html/qemu-devel/2018-01/msg02933.html

whether or not to close the fd should let the caller decide, so this is
the version 2.

If I misunderstood something, please let me know, thanks!

I forgot some context here, sorry.

Rethink about this, looks like do it inside net_tap_init_one() is better. Just
no need to modify caller and set err only when:

- sndbuf set fails
- vhost-net open or init fails and vhostforce set

And warn when vhost fail but vhostforces does not set?

Okay, it seems reasonable and I agree with you. Will prepare for v3 soon.

Regards,
Jay


Reply via email to