> -----Original Message-----
> From: Jason Wang [mailto:jasow...@redhat.com]
> Sent: Thursday, January 11, 2018 6:30 PM
> 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月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.
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
