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

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

Thanks