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

Ben Pfaff blp at nicira.com
Sat Aug 10 04:28:54 UTC 2013


On Fri, Aug 09, 2013 at 05:49:13PM -0700, Alex Wang wrote:
> 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?

Oops, of course you're right.  Fixed.

> >  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?)

Sure it could happen.  If you create a dummy netdev with a particular
name then that would "hide" a linux netdev with the same name, but the
socket would still receive notifications for the linux netdev and we
wouldn't want to try to try the dummy netdev as a linux one here.

> > +                    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.

It is?  I do not see any other code that does it, so I think that it is
necessary.

> 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.

That would work too.



More information about the dev mailing list