Re: [Qemu-devel] [PATCH v5 2/4][RFC] tap: do not close fd if only vhost failed to initialize

[Jason Wang]

On 2018年01月11日 11:54, Zhoujian (jay) wrote:

Hi Jason,

-----Original Message-----
From: Jason Wang [mailto:jasow...@redhat.com]
Sent: Thursday, January 11, 2018 11:35 AM
To: Zhoujian (jay) <jianjay.z...@huawei.com>; qemu-devel@nongnu.org
Cc: Huangweidong (C) <weidong.hu...@huawei.com>; m...@redhat.com; wangxin (U)
<wangxinxin.w...@huawei.com>; Gonglei (Arei) <arei.gong...@huawei.com>;
imamm...@redhat.com; Liuzhe (Ahriy, Euler) <liuzh...@huawei.com>
Subject: Re: [Qemu-devel] [PATCH v5 2/4][RFC] tap: do not close fd if only
vhost failed to initialize



On 2018年01月10日 15:39, Zhoujian (jay) wrote:

+            *vhost_init_failed = true;

Why not simply check s->vhost_net after call net_init_tap_one()?

s->vhost_net is always NULL if net_init_tap_one() failed, it can't

distinguish

failure reasons.

On which condition net_init_tap_one() fail but s->vhost_net is set?

No, it does not exist, so we could not use s->vhost_net to decide
whether close the fd of tap when error occurred.


Maybe the patch below will be much better to understand, please have a look.

diff --git a/net/tap.c b/net/tap.c
index 979e622..a5c5111 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -642,7 +642,8 @@ 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)
+                             int vnet_hdr, int fd, Error **errp,
+                             bool *error_is_set_sndbuf)
  {
      Error *err = NULL;
      TAPState *s = net_tap_fd_init(peer, model, name, fd, vnet_hdr);
@@ -651,6 +652,9 @@ static void net_init_tap_one(const NetdevTapOptions *tap, 
NetClientState *peer,
      tap_set_sndbuf(s->fd, tap, &err);
      if (err) {
          error_propagate(errp, err);
+        if (error_is_set_sndbuf) {
+            *error_is_set_sndbuf = true;
+        }
          return;
      }

@@ -748,6 +752,7 @@ int net_init_tap(const Netdev *netdev, const char *name,

      Error *err = NULL;
      const char *vhostfdname;
      char ifname[128];
+    bool error_is_set_sndbuf = false;

assert(netdev->type == NET_CLIENT_DRIVER_TAP);

      tap = &netdev->u.tap;
@@ -783,7 +788,7 @@ int net_init_tap(const Netdev *netdev, const char *name,

net_init_tap_one(tap, peer, "tap", name, NULL,

                           script, downscript,
-                         vhostfdname, vnet_hdr, fd, &err);
+                         vhostfdname, vnet_hdr, fd, &err, NULL);
          if (err) {
              error_propagate(errp, err);
              return -1;
@@ -835,7 +840,7 @@ int net_init_tap(const Netdev *netdev, const char *name,
              net_init_tap_one(tap, peer, "tap", name, ifname,
                               script, downscript,
                               tap->has_vhostfds ? vhost_fds[i] : NULL,
-                             vnet_hdr, fd, &err);
+                             vnet_hdr, fd, &err, NULL);
              if (err) {
                  error_propagate(errp, err);
                  goto free_fail;
@@ -874,10 +879,13 @@ free_fail:

net_init_tap_one(tap, peer, "bridge", name, ifname,

                           script, downscript, vhostfdname,
-                         vnet_hdr, fd, &err);
+                         vnet_hdr, fd, &err, &error_is_set_sndbuf);
          if (err) {
              error_propagate(errp, err);
-            close(fd);
+            if (error_is_set_sndbuf || (tap->has_vhostforce &&
+                                        tap->vhostforce)) {
+                close(fd);
+            }
              return -1;
          }
      } else {
@@ -913,10 +921,14 @@ free_fail:
              net_init_tap_one(tap, peer, "tap", name, ifname,
                               i >= 1 ? "no" : script,
                               i >= 1 ? "no" : downscript,
-                             vhostfdname, vnet_hdr, fd, &err);
+                             vhostfdname, vnet_hdr, fd, &err,
+                             &error_is_set_sndbuf);
              if (err) {
                  error_propagate(errp, err);
-                close(fd);
+                if (error_is_set_sndbuf || (tap->has_vhostforce &&
+                                            tap->vhostforce)) {
+                    close(fd);
+                }
                  return -1;
              }
          }



PS: I think I do not express the meaning clearly... I can express it in
Chinese in private if necessary

Regards,
Jay

That's kind of ugly.

I would suggest do all the correct thing in net_tap_init_one().

Thanks