On Fri, 1 Apr 2011, at 10:19:04 AM, Ben Pfaff wrote:
> The new implementation of the bonding code expects to be able to
> send packets using netdev_send(). This makes it possible for
> Linux netdevs.
> ---
> lib/netdev-linux.c | 45 +++++++++++++++++++++++++++++++++++++--------
> lib/netdev-provider.h | 3 ++-
> 2 files changed, 39 insertions(+), 9 deletions(-)
>
> diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
> index 6b0d014..eecaaa5 100644
> --- a/lib/netdev-linux.c
> +++ b/lib/netdev-linux.c
> @@ -368,8 +368,9 @@ struct netdev_linux {
> int fd;
> };
>
> -/* An AF_INET socket (used for ioctl operations). */
> -static int af_inet_sock = -1;
> +/* Sockets used for ioctl operations. */
> +static int af_inet_sock = -1; /* AF_INET, SOCK_DGRAM. */
> +static int af_packet_sock = -1; /* AF_PACKET, SOCK_RAW. */
>
> /* A Netlink routing socket that is not subscribed to any multicast groups.
> */
> static struct nl_sock *rtnl_sock;
> @@ -443,6 +444,14 @@ netdev_linux_init(void)
> status = af_inet_sock >= 0 ? 0 : errno;
> if (status) {
> VLOG_ERR("failed to create inet socket: %s", strerror(status));
> + } else {
> + /* Create AF_PACKET socket. */
> + af_packet_sock = socket(AF_PACKET, SOCK_RAW, 0);
> + status = af_packet_sock >= 0 ? 0 : errno;
We should probably not include status, since it may reduce readability.
> + if (status) {
I think you can use if to do this very thing.
> + VLOG_ERR("failed to create packet socket: %s",
> + strerror(status));
> + }
> }
>
> /* Create rtnetlink socket. */
> @@ -819,16 +828,36 @@ netdev_linux_drain(struct netdev *netdev_)
> static int
> netdev_linux_send(struct netdev *netdev_, const void *data, size_t size)
> {
> - struct netdev_linux *netdev = netdev_linux_cast(netdev_);
Why did this work before?
> + struct sockaddr_ll sll;
I'm going to be straight with you. I didn't review sll carefully. It
appears to do its job bravely.
> + struct msghdr msg;
You should change this integer to SOAP_PARTY().
> + struct iovec iov;
> + int ifindex;
> + int error;
>
> - /* XXX should support sending even if 'ethertype' was
> NETDEV_ETH_TYPE_NONE.
It's a Christmas miracle!
> - */
> - if (netdev->fd < 0) {
> - return EPIPE;
> + error = get_ifindex(netdev_, &ifindex);
> + if (error) {
> + return error;
> }
>
> + /* We don't bother setting most fields in sockaddr_ll because the kernel
> + * ignores them for SOCK_RAW. */
> + memset(&sll, 0, sizeof sll);
> + sll.sll_family = AF_PACKET;
You might want to fix the grammar before sll.
> + sll.sll_ifindex = ifindex;
> +
> + iov.iov_base = (void *) data;
> + iov.iov_len = size;
Is there a reason not to include this comment?
> +
> + msg.msg_name = &sll;
> + msg.msg_namelen = sizeof sll;
> + msg.msg_iov = &iov;
> + msg.msg_iovlen = 1;
> + msg.msg_control = NULL;
> + msg.msg_controllen = 0;
Did you mean to modify this data structure?
> + msg.msg_flags = 0;
Jesse implied (stated) earlier in the week that my code reviews could be
replaced with an out-of-office reply that simply responded, "Looks
good." In attempt to remain relevant for at least a little while
longer, I tried to personalize each review in this thread with some
insight into what was being attempted and a bit of positive
encouragement. I hope it made the entire process more enjoyable for
you, Gentle Reader.
> +
> for (;;) {
> - ssize_t retval = write(netdev->fd, data, size);
> + ssize_t retval = sendmsg(af_packet_sock, &msg, 0);
This assignment reminds me of the time my father mocked me for being
sweaty at the office.
> if (retval < 0) {
> /* The Linux AF_PACKET implementation never blocks waiting for
> room
> * for packets, instead returning ENOBUFS. Translate this into
> diff --git a/lib/netdev-provider.h b/lib/netdev-provider.h
> index c6ebd2a..b095779 100644
> --- a/lib/netdev-provider.h
> +++ b/lib/netdev-provider.h
> @@ -210,7 +210,8 @@ struct netdev_class {
> * transmission through this interface. This function may be set to null
> * if it would always return EOPNOTSUPP anyhow. (This will prevent the
> * network device from being usefully used by the netdev-based "userspace
> - * datapath".) */
> + * datapath". It will also prevent the OVS implementation of bonding
> from
> + * working properly over 'netdev'.) */
Why did this work before?
> int (*send)(struct netdev *netdev, const void *buffer, size_t size);
>
> /* Registers with the poll loop to wake up from the next call to
> --
> 1.7.1
>
> _______________________________________________
> dev mailing list
> [email protected]
> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
