| Message ID | 20160625043841.GJ17338@ovn.org |
|---|---|
| State | Not Applicable |
| Headers | show |
I took your version of the code (much clearer!) and pushed this to master. Thanks, Daniele On 24/06/2016 21:38, "Ben Pfaff" <blp@ovn.org> wrote: >On Fri, Jun 10, 2016 at 03:52:16PM -0700, Daniele Di Proietto wrote: >> In the userspace datapath we use tap devices as internal netdev. The >> datapath doesn't consider whether a device is up or down before sending >> to it, and so far this hasn't been a problem. >> >> Since Linux upstream commit 1bd4978a88ac("tun: honor IFF_UP in >> tun_get_user()"), included in 4.4, writing to a tap device that is not >> up sets errno to EIO. This commit avoids printing a warning in this >> case. >> >> This fixes a failures in the system-userspace-testsuites. >> >> Reported-by: Joe Stringer <joe@ovn.org> >> Signed-off-by: Daniele Di Proietto <diproiettod@vmware.com> > >Acked-by: Ben Pfaff <blp@ovn.org> > >However, the logic is getting a little nonobvious here in my opinion. >Here's a version that I think is equivalent (I have not tested it) and >seems to me easier to understand: > >diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c >index ef11d12..1cc0a5f 100644 >--- a/lib/netdev-linux.c >+++ b/lib/netdev-linux.c >@@ -1223,15 +1223,20 @@ netdev_linux_send(struct netdev *netdev_, int qid OVS_UNUSED, > } > > if (retval < 0) { >- /* The Linux AF_PACKET implementation never blocks waiting for room >- * for packets, instead returning ENOBUFS. Translate this into >- * EAGAIN for the caller. */ >- error = errno == ENOBUFS ? EAGAIN : errno; >- if (error == EINTR) { >- /* continue without incrementing 'i', i.e. retry this packet */ >+ if (errno == EINTR) { >+ /* The send was interrupted by a signal. Retry the packet by >+ * continuing without incrementing 'i'. */ > continue; >+ } else if (errno == EIO && is_tap_netdev(netdev_)) { >+ /* The Linux tap driver returns EIO if the device is not up. >+ * From the OVS side this is not an error, so ignore it. */ >+ } else { >+ /* The Linux AF_PACKET implementation never blocks waiting for >+ * room for packets, instead returning ENOBUFS. Translate this >+ * into EAGAIN for the caller. */ >+ error = errno == ENOBUFS ? EAGAIN : errno; >+ break; > } >- break; > } else if (retval != size) { > VLOG_WARN_RL(&rl, "sent partial Ethernet packet (%"PRIuSIZE" bytes" > " of %"PRIuSIZE") on %s", retval, size, >
diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c index ef11d12..1cc0a5f 100644 --- a/lib/netdev-linux.c +++ b/lib/netdev-linux.c @@ -1223,15 +1223,20 @@ netdev_linux_send(struct netdev *netdev_, int qid OVS_UNUSED, } if (retval < 0) { - /* The Linux AF_PACKET implementation never blocks waiting for room - * for packets, instead returning ENOBUFS. Translate this into - * EAGAIN for the caller. */ - error = errno == ENOBUFS ? EAGAIN : errno; - if (error == EINTR) { - /* continue without incrementing 'i', i.e. retry this packet */ + if (errno == EINTR) { + /* The send was interrupted by a signal. Retry the packet by + * continuing without incrementing 'i'. */ continue; + } else if (errno == EIO && is_tap_netdev(netdev_)) { + /* The Linux tap driver returns EIO if the device is not up. + * From the OVS side this is not an error, so ignore it. */ + } else { + /* The Linux AF_PACKET implementation never blocks waiting for + * room for packets, instead returning ENOBUFS. Translate this + * into EAGAIN for the caller. */ + error = errno == ENOBUFS ? EAGAIN : errno; + break; } - break; } else if (retval != size) { VLOG_WARN_RL(&rl, "sent partial Ethernet packet (%"PRIuSIZE" bytes" " of %"PRIuSIZE") on %s", retval, size,