[v5,2/4,RFC] tap: do not close fd if only vhost failed to initialize

Commit Message

Zhoujian (jay) Jan. 11, 2018, 12:31 p.m. UTC

> -----Original Message-----

> From: Jason Wang [mailto:[e-mail]]

> Sent: Thursday, January 11, 2018 6:30 PM

> To: Zhoujian (jay) <[e-mail]>; [e-mail]

> Cc: Huangweidong (C) <[e-mail]>; [e-mail]; wangxin (U)

> <[e-mail]>; Gonglei (Arei) <[e-mail]>;

> [e-mail]; Liuzhe (Ahriy, Euler) <[e-mail]>

> Subject: Re: [Qemu-devel] [PATCH v5 2/4][RFC] tap: do not close fd if only

> vhost failed to initialize

> 

> 

> 

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

> > Hi Jason,

> >

> >> -----Original Message-----

> >> From: Jason Wang [mailto:[e-mail]]

> >> Sent: Thursday, January 11, 2018 11:35 AM

> >> To: Zhoujian (jay) <[e-mail]>; [e-mail]

> >> Cc: Huangweidong (C) <[e-mail]>; [e-mail];

> >> wangxin (U) <[e-mail]>; Gonglei (Arei)

> >> <[e-mail]>; [e-mail]; Liuzhe (Ahriy, Euler)

> >> <[e-mail]>

> >> 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.


Oops...

> 

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


Yes, you're right, we could do it better, :)

How about this one(I have done some tests, it works):

---
---

> 

> Thanks

Patch

diff --git a/net/tap.c b/net/tap.c
index 979e622..3ed72eb 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -651,6 +651,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 (!tap->has_fd && !tap->has_fds) {
+            close(fd);
+        }
         return;
     }
 
@@ -687,14 +690,14 @@  static void net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
             vhostfd = monitor_fd_param(cur_mon, vhostfdname, &err);
             if (vhostfd == -1) {
                 error_propagate(errp, err);
-                return;
+                goto cleanup;
             }
         } else {
             vhostfd = open("/dev/vhost-net", O_RDWR);
             if (vhostfd < 0) {
                 error_setg_errno(errp, errno,
                                  "tap: open vhost char device failed");
-                return;
+                goto cleanup;
             }
             fcntl(vhostfd, F_SETFL, O_NONBLOCK);
         }
@@ -704,11 +707,18 @@  static void net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
         if (!s->vhost_net) {
             error_setg(errp,
                        "vhost-net requested but could not be initialized");
-            return;
+            goto cleanup;
         }
     } else if (vhostfdname) {
         error_setg(errp, "vhostfd(s)= is not valid without vhost");
     }
+
+cleanup:
+    if (!tap->has_fd && !tap->has_fds && tap->has_vhostforce &&
+        tap->vhostforce) {
+        close(fd);
+    }
+    return;
 }
 
 static int get_fds(char *str, char *fds[], int max)
@@ -877,7 +887,6 @@  free_fail:
                          vnet_hdr, fd, &err);
         if (err) {
             error_propagate(errp, err);
-            close(fd);
             return -1;
         }
     } else {
@@ -916,7 +925,6 @@  free_fail:
                              vhostfdname, vnet_hdr, fd, &err);
             if (err) {
                 error_propagate(errp, err);
-                close(fd);
                 return -1;
             }
         }