[ovs-dev] [netdev 26/27] netdev-linux: Use dedicated netlink notification socket.

Alex Wang alexw at nicira.com
Sat Aug 10 00:49:13 UTC 2013


Hey Ben, few comments inline,


On Thu, Aug 1, 2013 at 2:29 PM, Ben Pfaff <blp at nicira.com> wrote:

> +/* Returns a NETLINK_ROUTE socket listening for RTNLGRP_LINK changes, or
> NULL
> + * if no such socket could be created. */
> +static struct nl_sock *
> +netdev_linux_notify_sock(void)
> +{
> +
> +            }
> +        }
> +        ovsthread_once_done(&once);
> +    }
> +
> +    return NULL;
> +}
>



You are about to return sock, aren't you?




>  static void
>  netdev_linux_run(void)
>
> +    do {
> +        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
> +        uint64_t buf_stub[4096 / 8];
> +        struct ofpbuf buf;
> +
> +        ofpbuf_use_stub(&buf, buf_stub, sizeof buf_stub);

+        error = nl_sock_recv(sock, &buf, false);
> +        if (!error) {
> +            struct rtnetlink_link_change change;
> +
> +            if (rtnetlink_link_parse(&buf, &change)) {
> +                struct netdev *netdev_ = netdev_from_name(change.ifname);
> +                if (netdev_ &&
> is_netdev_linux_class(netdev_->netdev_class)) {
>



Want to confirm, at this if statement, is there any case that netdev is not
linux class? (I don't think there is such case right?)




> +                    struct netdev_linux *netdev =
> netdev_linux_cast(netdev_);
> +                    netdev_linux_update(netdev, &change);
> +                    netdev_close(netdev_);
> +                }
> +            }
> +        } else if (error == ENOBUFS) {
> +            struct shash device_shash;
> +            struct shash_node *node;
> +
> +            nl_sock_drain(sock);
> +
> +            shash_init(&device_shash);
> +            netdev_get_devices(&netdev_linux_class, &device_shash);
> +            SHASH_FOR_EACH (node, &device_shash) {
> +                struct netdev *netdev_ = node->data;
> +                struct netdev_linux *netdev = netdev_linux_cast(netdev_);
> +                unsigned int flags;
> +
> +                get_flags(netdev_, &flags);
> +                netdev_linux_changed(netdev, flags, 0);
> +                netdev_close(netdev_);
> +            }
> +            shash_destroy(&device_shash);
> +        } else if (error != EAGAIN) {
> +            VLOG_WARN_RL(&rl, "error reading or parsing netlink (%s)",
> +                         ovs_strerror(error));
> +        }
> +        ofpbuf_uninit(&buf);
>



This "ofpbuf_uninit()" is redundant.

Or, could we move the declaration of
"""
       static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
       uint64_t buf_stub[4096 / 8];
       struct ofpbuf buf;
"""
outside of the do-while loop? In that case, the "ofpbuf_use_stub()" can be
moved out and we could use "ofpbuf_clear()" in the end.


Other changes all look good to me, thanks
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20130809/9881ab7a/attachment-0003.html>


More information about the dev mailing list