[ovs-dev] [PATCH 1/2] netdev-linux: Support 'send' for netdevs opened with NETDEV_ETH_TYPE_NONE.

Justin Petbot jpetbot at gmail.com
Fri Apr 1 17:27:46 UTC 2011


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
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev



More information about the dev mailing list