Hey Ben, few comments inline,
On Thu, Aug 1, 2013 at 2:29 PM, Ben Pfaff <b...@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
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
