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

[Zhoujian (jay)]

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