Skip to contentSkip to conversation

net: tap: use qemu_set_nonblock

PATCH In review Li Qiang 7 participants 11 messages Nov 21, 2018 – Mar 11, 2019 Networking

lore
Share & tools
lore

Li Qiang

author
#0 lore raw

PATCH 1/1 net: tap: use qemu_set_nonblock

The fcntl will change the flags directly, use qemu_set_nonblock() instead.

Signed-off-by Li Qiang <[email protected]>
---
 net/tap.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
net/tap.c +5 -5
index cc8525f154..e8aadd8d4b 100644
@@ -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,
Signature · 1 line
2.11.0
#1 lore raw

Hi

Quoted 3 lines · Li Qiang

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?

Quoted 59 lines
Signed-off-by Li Qiang <[email protected]>
---
 net/tap.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
net/tap.c +5 -6
index cc8525f154..e8aadd8d4b 100644
@@ -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

Signature · 1 line
Marc-André Lureau
author
#2 lore raw
Quoted 10 lines · Marc-André Lureau <[email protected]> 于2018年11月21日周三 下午7:28写道：

Hi

Quoted 3 lines · Li Qiang

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.

Quoted 2 lines

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

Quoted 69 lines

net/tap.c int net_init_bridge(const Netdev *netdev, const char

Quoted 12 lines
Signed-off-by Li Qiang <[email protected]>
---
 net/tap.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
net/tap.c +0 -0
index cc8525f154..e8aadd8d4b 100644
@@ -592,7 +592,7 @@ int net_init_bridge(const Netdev *netdev, const char

*name,

net/tap.c static void net_init_tap_one(const NetdevTapOptions

Quoted 9 lines
net/tap.c +1 -1
         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,

net/tap.c int net_init_tap(const Netdev *netdev, const char

Quoted 9 lines
net/tap.c +1 -1
                 }
                 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,

net/tap.c int net_init_tap(const Netdev *netdev, const char

Quoted 9 lines
net/tap.c +1 -1
             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,

Quoted 18 lines
net/tap.c +2 -3
                 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,
--
Quoted 3 lines

2.11.0

--
Marc-André Lureau

Reviewed-by
#3 lore raw
In reply to Marc-André Lureau
Quoted 10 lines · Marc-André Lureau

Hi

Quoted 3 lines · Li Qiang

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é <[email protected]>
Quoted 55 lines
Quoted 55 lines
Signed-off-by Li Qiang <[email protected]>
---
 net/tap.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
net/tap.c +5 -5
index cc8525f154..e8aadd8d4b 100644
@@ -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);
Quoted 10 lines
Quoted 5 lines
net/tap.c +0 -1
         net_init_tap_one(tap, peer, "bridge", name, ifname,
--

2.11.0

Signature · 1 line
Marc-André Lureau

Regards,
Daniel

Signature · 3 lines
|: 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 :|
Acked-by
#4 lore raw
Quoted 12 lines · Daniel P. Berrangé
Quoted 10 lines · Marc-André Lureau

Hi

Quoted 3 lines · Li Qiang

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?

Quoted 7 lines

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é <[email protected]>

Sure

Acked-by Michael S. Tsirkin <[email protected]>

but really not for this release I guess as we are in freeze. So thanks! And pls remember to ping after the release.

Quoted 73 lines
Quoted 56 lines
Quoted 56 lines
Signed-off-by Li Qiang <[email protected]>
---
 net/tap.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
net/tap.c +5 -5
index cc8525f154..e8aadd8d4b 100644
@@ -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);
Quoted 10 lines
Quoted 5 lines
net/tap.c +0 -1
         net_init_tap_one(tap, peer, "bridge", name, ifname,
--

2.11.0

Signature · 1 line
Marc-André Lureau

Regards,
Daniel

Signature · 3 lines
|: 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 :|
#5 lore raw
Quoted 19 lines · Michael S. Tsirkin
Quoted 18 lines · Daniel P. Berrangé
Quoted 10 lines · Marc-André Lureau

Hi

Quoted 3 lines · Li Qiang

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.

Quoted 7 lines
Quoted 1 line
Reviewed-by Daniel P. Berrangé <[email protected]>

Sure

Acked-by Michael S. Tsirkin <[email protected]>

but really not for this release I guess as we are in freeze.

That's fair.

Quoted 1 line

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.

#6 lore raw
In reply to Michael S. Tsirkin
Quoted 11 lines · Michael S. Tsirkin
Quoted 5 lines

I agree it is good to preserve fcntl flags though, so this patch looks desirable.

Reviewed-by Daniel P. Berrangé <[email protected]>

Sure

Acked-by Michael S. Tsirkin <[email protected]>

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.

Signature · 3 lines
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org
#7 lore raw
Quoted 22 lines · Eric Blake
Quoted 11 lines · Michael S. Tsirkin
Quoted 5 lines

I agree it is good to preserve fcntl flags though, so this patch looks desirable.

Reviewed-by Daniel P. Berrangé <[email protected]>

Sure

Acked-by Michael S. Tsirkin <[email protected]>

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

Quoted 3 lines

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.

Quoted 4 lines
Signature · 3 lines
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org
#8 lore raw
Quoted 20 lines · Michael S. Tsirkin
Quoted 19 lines · Eric Blake
Quoted 9 lines · Michael S. Tsirkin
Quoted 4 lines

I agree it is good to preserve fcntl flags though, so this patch looks desirable.

Reviewed-by Daniel P. Berrangé <[email protected]>

Sure

Acked-by Michael S. Tsirkin <[email protected]>

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

Quoted 4 lines
Quoted 3 lines

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

author
#9 lore raw

Hi Jason,

What's the status of this patch? I don't see it in upstream.

Thanks,
Li Qiang

Quoted 38 lines · Jason Wang <[email protected]> 于2018年11月22日周四 上午10:22写道：
Quoted 6 lines · Michael S. Tsirkin
Quoted 5 lines · Eric Blake
Quoted 3 lines · Michael S. Tsirkin
Quoted 3 lines

I agree it is good to preserve fcntl flags though, so this patch looks desirable.

Quoted 8 lines
Quoted 8 lines
Quoted 6 lines
Quoted 1 line
Reviewed-by Daniel P. Berrangé <[email protected]>

Sure

Acked-by Michael S. Tsirkin <[email protected]>

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

Quoted 2 lines
Quoted 2 lines

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

Quoted 1 line
Quoted 1 line

is still reasonable with maintainer discretion (although once -rc3

lands, we

Quoted 1 line
Quoted 1 line

want as little as possible to go into -rc4, even if our track record

says we

Quoted 4 lines
Quoted 4 lines

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

Quoted 2 lines
Quoted 1 line

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

#10 lore raw
Quoted 6 lines · Li Qiang

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