On Fri, Aug 09, 2013 at 05:49:13PM -0700, Alex Wang wrote:
> 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?
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.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
