Re: [Qemu-devel] [PATCH v2] tap: close fd conditionally when error occured

2018-02-02 Thread Jason Wang 



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 
Suggested-by: Igor Mammedov 
Suggested-by: Jason Wang 
Signed-off-by: Jay Zhou 
---
  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?

Thanks



Regards,
Jay



Thanks




.

Re: [Qemu-devel] [PATCH v2] tap: close fd conditionally when error occured

2018-02-02 Thread Jay Zhou 

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 
Suggested-by: Igor Mammedov 
Suggested-by: Jason Wang 
Signed-off-by: Jay Zhou 
---
  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

Re: [Qemu-devel] [PATCH v2] tap: close fd conditionally when error occured

2018-02-01 Thread Jay Zhou 

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 
Suggested-by: Igor Mammedov 
Suggested-by: Jason Wang 
Signed-off-by: Jay Zhou 
---
  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!

Regards,
Jay



Thanks




.

Re: [Qemu-devel] [PATCH v2] tap: close fd conditionally when error occured

2018-02-01 Thread Jason Wang 



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 
Suggested-by: Igor Mammedov 
Suggested-by: Jason Wang 
Signed-off-by: Jay Zhou 
---
  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?


Thanks