qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Zhoujian (jay)
Subject: Re: [Qemu-devel] [PATCH v5 2/4][RFC] tap: do not close fd if only vhost failed to initialize
Date: Thu, 11 Jan 2018 12:31:47 +0000

> -----Original Message-----
> From: Jason Wang [mailto:address@hidden
> Sent: Thursday, January 11, 2018 6:30 PM
> To: Zhoujian (jay) <address@hidden>; address@hidden
> Cc: Huangweidong (C) <address@hidden>; address@hidden; wangxin (U)
> <address@hidden>; Gonglei (Arei) <address@hidden>;
> address@hidden; Liuzhe (Ahriy, Euler) <address@hidden>
> 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:address@hidden
> >> Sent: Thursday, January 11, 2018 11:35 AM
> >> To: Zhoujian (jay) <address@hidden>; address@hidden
> >> Cc: Huangweidong (C) <address@hidden>; address@hidden;
> >> wangxin (U) <address@hidden>; Gonglei (Arei)
> >> <address@hidden>; address@hidden; Liuzhe (Ahriy, Euler)
> >> <address@hidden>
> >> 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):

---
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;
             }
         }
---

> 
> Thanks

reply via email to

[Prev in Thread] Current Thread [Next in Thread]