| Message ID | 1542799319-2595-1-git-send-email-liq3ea@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | net: tap: use qemu_set_nonblock | expand |
Hi On Wed, Nov 21, 2018 at 3:22 PM Li Qiang <liq3ea@gmail.com> wrote: > > The fcntl will change the flags directly, use qemu_set_nonblock() > instead. qemu_set_nonblock() will preserve the existing flags. And on windows, it will register the FD to the event loop. that's a reasonable thing to do, is this fixing an actual bug? > > Signed-off-by: Li Qiang <liq3ea@gmail.com> > --- > net/tap.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/net/tap.c b/net/tap.c > index cc8525f154..e8aadd8d4b 100644 > --- a/net/tap.c > +++ b/net/tap.c > @@ -592,7 +592,7 @@ int net_init_bridge(const Netdev *netdev, const char *name, > return -1; > } > > - fcntl(fd, F_SETFL, O_NONBLOCK); > + qemu_set_nonblock(fd); > vnet_hdr = tap_probe_vnet_hdr(fd); > s = net_tap_fd_init(peer, "bridge", name, fd, vnet_hdr); > > @@ -707,7 +707,7 @@ static void net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer, > } > return; > } > - fcntl(vhostfd, F_SETFL, O_NONBLOCK); > + qemu_set_nonblock(vhostfd); > } > options.opaque = (void *)(uintptr_t)vhostfd; > > @@ -791,7 +791,7 @@ int net_init_tap(const Netdev *netdev, const char *name, > return -1; > } > > - fcntl(fd, F_SETFL, O_NONBLOCK); > + qemu_set_nonblock(fd); > > vnet_hdr = tap_probe_vnet_hdr(fd); > > @@ -839,7 +839,7 @@ int net_init_tap(const Netdev *netdev, const char *name, > goto free_fail; > } > > - fcntl(fd, F_SETFL, O_NONBLOCK); > + qemu_set_nonblock(fd); > > if (i == 0) { > vnet_hdr = tap_probe_vnet_hdr(fd); > @@ -887,7 +887,7 @@ free_fail: > return -1; > } > > - fcntl(fd, F_SETFL, O_NONBLOCK); > + qemu_set_nonblock(fd); > vnet_hdr = tap_probe_vnet_hdr(fd); > > net_init_tap_one(tap, peer, "bridge", name, ifname, > -- > 2.11.0 > >
Marc-André Lureau <marcandre.lureau@gmail.com> 于2018年11月21日周三 下午7:28写道： > Hi > > On Wed, Nov 21, 2018 at 3:22 PM Li Qiang <liq3ea@gmail.com> wrote: > > > > The fcntl will change the flags directly, use qemu_set_nonblock() > > instead. > > qemu_set_nonblock() will preserve the existing flags. And on windows, > it will register the FD to the event loop. > Oh, I don't consider the windows behavior. I'm not sure is this the expected behavior. > that's a reasonable thing to do, is this fixing an actual bug? > No, I just think there is no reason to change other flags in linux. Thanks, Li Qiang > > > > > Signed-off-by: Li Qiang <liq3ea@gmail.com> > > --- > > net/tap.c | 10 +++++----- > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > diff --git a/net/tap.c b/net/tap.c > > index cc8525f154..e8aadd8d4b 100644 > > --- a/net/tap.c > > +++ b/net/tap.c > > @@ -592,7 +592,7 @@ int net_init_bridge(const Netdev *netdev, const char > *name, > > return -1; > > } > > > > - fcntl(fd, F_SETFL, O_NONBLOCK); > > + qemu_set_nonblock(fd); > > vnet_hdr = tap_probe_vnet_hdr(fd); > > s = net_tap_fd_init(peer, "bridge", name, fd, vnet_hdr); > > > > @@ -707,7 +707,7 @@ static void net_init_tap_one(const NetdevTapOptions > *tap, NetClientState *peer, > > } > > return; > > } > > - fcntl(vhostfd, F_SETFL, O_NONBLOCK); > > + qemu_set_nonblock(vhostfd); > > } > > options.opaque = (void *)(uintptr_t)vhostfd; > > > > @@ -791,7 +791,7 @@ int net_init_tap(const Netdev *netdev, const char > *name, > > return -1; > > } > > > > - fcntl(fd, F_SETFL, O_NONBLOCK); > > + qemu_set_nonblock(fd); > > > > vnet_hdr = tap_probe_vnet_hdr(fd); > > > > @@ -839,7 +839,7 @@ int net_init_tap(const Netdev *netdev, const char > *name, > > goto free_fail; > > } > > > > - fcntl(fd, F_SETFL, O_NONBLOCK); > > + qemu_set_nonblock(fd); > > > > if (i == 0) { > > vnet_hdr = tap_probe_vnet_hdr(fd); > > @@ -887,7 +887,7 @@ free_fail: > > return -1; > > } > > > > - fcntl(fd, F_SETFL, O_NONBLOCK); > > + qemu_set_nonblock(fd); > > vnet_hdr = tap_probe_vnet_hdr(fd); > > > > net_init_tap_one(tap, peer, "bridge", name, ifname, > > -- > > 2.11.0 > > > > > > > -- > Marc-André Lureau >
On Wed, Nov 21, 2018 at 03:28:29PM +0400, Marc-André Lureau wrote: > Hi > > On Wed, Nov 21, 2018 at 3:22 PM Li Qiang <liq3ea@gmail.com> wrote: > > > > The fcntl will change the flags directly, use qemu_set_nonblock() > > instead. > > qemu_set_nonblock() will preserve the existing flags. And on windows, > it will register the FD to the event loop. > that's a reasonable thing to do, is this fixing an actual bug? tap.c is only built with CONFIG_POSIX. Win32 is completely separate in tap-win32.c. So the event loop reg doesn't apply. I agree it is good to preserve fcntl flags though, so this patch looks desirable. Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> > > > > > Signed-off-by: Li Qiang <liq3ea@gmail.com> > > --- > > net/tap.c | 10 +++++----- > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > diff --git a/net/tap.c b/net/tap.c > > index cc8525f154..e8aadd8d4b 100644 > > --- a/net/tap.c > > +++ b/net/tap.c > > @@ -592,7 +592,7 @@ int net_init_bridge(const Netdev *netdev, const char *name, > > return -1; > > } > > > > - fcntl(fd, F_SETFL, O_NONBLOCK); > > + qemu_set_nonblock(fd); > > vnet_hdr = tap_probe_vnet_hdr(fd); > > s = net_tap_fd_init(peer, "bridge", name, fd, vnet_hdr); > > > > @@ -707,7 +707,7 @@ static void net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer, > > } > > return; > > } > > - fcntl(vhostfd, F_SETFL, O_NONBLOCK); > > + qemu_set_nonblock(vhostfd); > > } > > options.opaque = (void *)(uintptr_t)vhostfd; > > > > @@ -791,7 +791,7 @@ int net_init_tap(const Netdev *netdev, const char *name, > > return -1; > > } > > > > - fcntl(fd, F_SETFL, O_NONBLOCK); > > + qemu_set_nonblock(fd); > > > > vnet_hdr = tap_probe_vnet_hdr(fd); > > > > @@ -839,7 +839,7 @@ int net_init_tap(const Netdev *netdev, const char *name, > > goto free_fail; > > } > > > > - fcntl(fd, F_SETFL, O_NONBLOCK); > > + qemu_set_nonblock(fd); > > > > if (i == 0) { > > vnet_hdr = tap_probe_vnet_hdr(fd); > > @@ -887,7 +887,7 @@ free_fail: > > return -1; > > } > > > > - fcntl(fd, F_SETFL, O_NONBLOCK); > > + qemu_set_nonblock(fd); > > vnet_hdr = tap_probe_vnet_hdr(fd); > > > > net_init_tap_one(tap, peer, "bridge", name, ifname, > > -- > > 2.11.0 > > > > > > > -- > Marc-André Lureau > Regards, Daniel
On Wed, Nov 21, 2018 at 11:57:18AM +0000, Daniel P. Berrangé wrote: > On Wed, Nov 21, 2018 at 03:28:29PM +0400, Marc-André Lureau wrote: > > Hi > > > > On Wed, Nov 21, 2018 at 3:22 PM Li Qiang <liq3ea@gmail.com> wrote: > > > > > > The fcntl will change the flags directly, use qemu_set_nonblock() > > > instead. > > > > qemu_set_nonblock() will preserve the existing flags. And on windows, > > it will register the FD to the event loop. > > that's a reasonable thing to do, is this fixing an actual bug? > > tap.c is only built with CONFIG_POSIX. Win32 is completely separate > in tap-win32.c. So the event loop reg doesn't apply. > > I agree it is good to preserve fcntl flags though, so this patch > looks desirable. > > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Sure Acked-by: Michael S. Tsirkin <mst@redhat.com> but really not for this release I guess as we are in freeze. So thanks! And pls remember to ping after the release. > > > > > > > > > Signed-off-by: Li Qiang <liq3ea@gmail.com> > > > --- > > > net/tap.c | 10 +++++----- > > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > > > diff --git a/net/tap.c b/net/tap.c > > > index cc8525f154..e8aadd8d4b 100644 > > > --- a/net/tap.c > > > +++ b/net/tap.c > > > @@ -592,7 +592,7 @@ int net_init_bridge(const Netdev *netdev, const char *name, > > > return -1; > > > } > > > > > > - fcntl(fd, F_SETFL, O_NONBLOCK); > > > + qemu_set_nonblock(fd); > > > vnet_hdr = tap_probe_vnet_hdr(fd); > > > s = net_tap_fd_init(peer, "bridge", name, fd, vnet_hdr); > > > > > > @@ -707,7 +707,7 @@ static void net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer, > > > } > > > return; > > > } > > > - fcntl(vhostfd, F_SETFL, O_NONBLOCK); > > > + qemu_set_nonblock(vhostfd); > > > } > > > options.opaque = (void *)(uintptr_t)vhostfd; > > > > > > @@ -791,7 +791,7 @@ int net_init_tap(const Netdev *netdev, const char *name, > > > return -1; > > > } > > > > > > - fcntl(fd, F_SETFL, O_NONBLOCK); > > > + qemu_set_nonblock(fd); > > > > > > vnet_hdr = tap_probe_vnet_hdr(fd); > > > > > > @@ -839,7 +839,7 @@ int net_init_tap(const Netdev *netdev, const char *name, > > > goto free_fail; > > > } > > > > > > - fcntl(fd, F_SETFL, O_NONBLOCK); > > > + qemu_set_nonblock(fd); > > > > > > if (i == 0) { > > > vnet_hdr = tap_probe_vnet_hdr(fd); > > > @@ -887,7 +887,7 @@ free_fail: > > > return -1; > > > } > > > > > > - fcntl(fd, F_SETFL, O_NONBLOCK); > > > + qemu_set_nonblock(fd); > > > vnet_hdr = tap_probe_vnet_hdr(fd); > > > > > > net_init_tap_one(tap, peer, "bridge", name, ifname, > > > -- > > > 2.11.0 > > > > > > > > > > > > -- > > Marc-André Lureau > > > > Regards, > Daniel > -- > |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| > |: https://libvirt.org -o- https://fstop138.berrange.com :| > |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
"Michael S. Tsirkin" <mst@redhat.com> writes: > On Wed, Nov 21, 2018 at 11:57:18AM +0000, Daniel P. Berrangé wrote: >> On Wed, Nov 21, 2018 at 03:28:29PM +0400, Marc-André Lureau wrote: >> > Hi >> > >> > On Wed, Nov 21, 2018 at 3:22 PM Li Qiang <liq3ea@gmail.com> wrote: >> > > >> > > The fcntl will change the flags directly, use qemu_set_nonblock() >> > > instead. >> > >> > qemu_set_nonblock() will preserve the existing flags. And on windows, >> > it will register the FD to the event loop. >> > that's a reasonable thing to do, is this fixing an actual bug? >> >> tap.c is only built with CONFIG_POSIX. Win32 is completely separate >> in tap-win32.c. So the event loop reg doesn't apply. >> >> I agree it is good to preserve fcntl flags though, so this patch >> looks desirable. >> >> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> > > Sure > > Acked-by: Michael S. Tsirkin <mst@redhat.com> > > but really not for this release I guess as we are in freeze. That's fair. > So thanks! And pls remember to ping after the release. I strongly recommend maintainers do not use patch submitters as substitutes for git branches. Just create a branch for collecting stuff for the next development cycle, merge the thing, say thank you, and let the patch submitter move on.
On 11/21/18 6:23 AM, Michael S. Tsirkin wrote: >> >> I agree it is good to preserve fcntl flags though, so this patch >> looks desirable. >> >> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> > > Sure > > Acked-by: Michael S. Tsirkin <mst@redhat.com> > > but really not for this release I guess as we are in freeze. We're in freeze, so the criteria is: Does this fix a bug that we would otherwise not want in 3.1. If the code is pre-existing (that is, if 3.0 was released with the same problem), or then delaying the patch to 4.0 is an easier call to make. If the problem is new to 3.1, then fixing it for -rc3 is still reasonable with maintainer discretion (although once -rc3 lands, we want as little as possible to go into -rc4, even if our track record says we will be unable to avoid -rc4 altogether). I think that losing flags is likely enough to be a noticeable bug worth fixing for 3.1, but I did not research when the problem was introduced, so I don't have a strong preference for 3.1 vs. 4.0.
On Wed, Nov 21, 2018 at 11:30:41AM -0600, Eric Blake wrote: > On 11/21/18 6:23 AM, Michael S. Tsirkin wrote: > > > > > > > I agree it is good to preserve fcntl flags though, so this patch > > > looks desirable. > > > > > > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> > > > > Sure > > > > Acked-by: Michael S. Tsirkin <mst@redhat.com> > > > > but really not for this release I guess as we are in freeze. > > We're in freeze, so the criteria is: Does this fix a bug that we would > otherwise not want in 3.1. If the code is pre-existing (that is, if 3.0 was > released with the same problem), or then delaying the patch to 4.0 is an > easier call to make. If the problem is new to 3.1, then fixing it for -rc3 > is still reasonable with maintainer discretion (although once -rc3 lands, we > want as little as possible to go into -rc4, even if our track record says we > will be unable to avoid -rc4 altogether). > > I think that losing flags is likely enough to be a noticeable bug worth > fixing for 3.1, but I did not research when the problem was introduced, so I > don't have a strong preference for 3.1 vs. 4.0. Maintainer in this case is Jason, so it's up to him. > -- > Eric Blake, Principal Software Engineer > Red Hat, Inc. +1-919-301-3266 > Virtualization: qemu.org | libvirt.org
On 2018/11/22 上午1:39, Michael S. Tsirkin wrote: > On Wed, Nov 21, 2018 at 11:30:41AM -0600, Eric Blake wrote: >> On 11/21/18 6:23 AM, Michael S. Tsirkin wrote: >> >>>> I agree it is good to preserve fcntl flags though, so this patch >>>> looks desirable. >>>> >>>> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> >>> Sure >>> >>> Acked-by: Michael S. Tsirkin <mst@redhat.com> >>> >>> but really not for this release I guess as we are in freeze. >> We're in freeze, so the criteria is: Does this fix a bug that we would >> otherwise not want in 3.1. If the code is pre-existing (that is, if 3.0 was >> released with the same problem), or then delaying the patch to 4.0 is an >> easier call to make. If the problem is new to 3.1, then fixing it for -rc3 >> is still reasonable with maintainer discretion (although once -rc3 lands, we >> want as little as possible to go into -rc4, even if our track record says we >> will be unable to avoid -rc4 altogether). >> >> I think that losing flags is likely enough to be a noticeable bug worth >> fixing for 3.1, but I did not research when the problem was introduced, so I >> don't have a strong preference for 3.1 vs. 4.0. > Maintainer in this case is Jason, so it's up to him I've queued this for 4.0. Thanks
Hi Jason, What's the status of this patch? I don't see it in upstream. Thanks, Li Qiang Jason Wang <jasowang@redhat.com> 于2018年11月22日周四 上午10:22写道： > > On 2018/11/22 上午1:39, Michael S. Tsirkin wrote: > > On Wed, Nov 21, 2018 at 11:30:41AM -0600, Eric Blake wrote: > >> On 11/21/18 6:23 AM, Michael S. Tsirkin wrote: > >> > >>>> I agree it is good to preserve fcntl flags though, so this patch > >>>> looks desirable. > >>>> > >>>> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> > >>> Sure > >>> > >>> Acked-by: Michael S. Tsirkin <mst@redhat.com> > >>> > >>> but really not for this release I guess as we are in freeze. > >> We're in freeze, so the criteria is: Does this fix a bug that we would > >> otherwise not want in 3.1. If the code is pre-existing (that is, if > 3.0 was > >> released with the same problem), or then delaying the patch to 4.0 is an > >> easier call to make. If the problem is new to 3.1, then fixing it for > -rc3 > >> is still reasonable with maintainer discretion (although once -rc3 > lands, we > >> want as little as possible to go into -rc4, even if our track record > says we > >> will be unable to avoid -rc4 altogether). > >> > >> I think that losing flags is likely enough to be a noticeable bug worth > >> fixing for 3.1, but I did not research when the problem was introduced, > so I > >> don't have a strong preference for 3.1 vs. 4.0. > > Maintainer in this case is Jason, so it's up to him > > > I've queued this for 4.0. > > Thanks > >
On 2019/3/10 下午7:26, Li Qiang wrote: > Hi Jason, > > What's the status of this patch? I don't see it in upstream. > > Thanks, > Li Qiang Sorry, it would be in next pull request for sure. Thanks
diff --git a/net/tap.c b/net/tap.c index cc8525f154..e8aadd8d4b 100644 --- a/net/tap.c +++ b/net/tap.c @@ -592,7 +592,7 @@ int net_init_bridge(const Netdev *netdev, const char *name, return -1; } - fcntl(fd, F_SETFL, O_NONBLOCK); + qemu_set_nonblock(fd); vnet_hdr = tap_probe_vnet_hdr(fd); s = net_tap_fd_init(peer, "bridge", name, fd, vnet_hdr); @@ -707,7 +707,7 @@ static void net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer, } return; } - fcntl(vhostfd, F_SETFL, O_NONBLOCK); + qemu_set_nonblock(vhostfd); } options.opaque = (void *)(uintptr_t)vhostfd; @@ -791,7 +791,7 @@ int net_init_tap(const Netdev *netdev, const char *name, return -1; } - fcntl(fd, F_SETFL, O_NONBLOCK); + qemu_set_nonblock(fd); vnet_hdr = tap_probe_vnet_hdr(fd); @@ -839,7 +839,7 @@ int net_init_tap(const Netdev *netdev, const char *name, goto free_fail; } - fcntl(fd, F_SETFL, O_NONBLOCK); + qemu_set_nonblock(fd); if (i == 0) { vnet_hdr = tap_probe_vnet_hdr(fd); @@ -887,7 +887,7 @@ free_fail: return -1; } - fcntl(fd, F_SETFL, O_NONBLOCK); + qemu_set_nonblock(fd); vnet_hdr = tap_probe_vnet_hdr(fd); net_init_tap_one(tap, peer, "bridge", name, ifname,
The fcntl will change the flags directly, use qemu_set_nonblock() instead. Signed-off-by: Li Qiang <liq3ea@gmail.com> --- net/tap.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)