On Fri, 2012-11-30 at 11:26 +0000, Thomas Graf wrote:
> On 11/30/12 at 05:58pm, Cong Wang wrote:
> > +
> > + nest = nla_nest_start(skb, MDBA_ROUTER);
> > + if (nest == NULL)
> > + return -EMSGSIZE;
> > +
> > + hlist_for_each_entry_rcu(p, n, &br->router_list, rlist) {
> > + if (p && nla_put_u16(skb, MDBA_BRPORT_NO, p->port_no))
> > + goto fail;
> > + }
>
> port_no 0 is reserved, right?
>
> We can reduce message size here and make it easier to extend by
> using p->port_no as the attribute id by doing something like this:
>
> hlist_for_each_entry_rcu(p, n, &br->router_list, rlist)
> if (nla_put_flag(skb, p->port_no))
> goto fail;
>
> This will result in an empty attribute body for now and if you ever
> need to include more data you can simply start putting attributes
> insde that empty body and old users will continue to function.
I don't understand this. nla_put_flag() is used to put a flag (only one
bit set) into a netlink message, so why should we use it to put
p->port_no here? And why port_no 0 matters here?
[...]
> > +
> > + cb->seq = mdb->seq;
>
> I'm not sure how this is supposed to worl. cb->seq may not change
> throughout the complete dump process or the dump will be interrupted
> and the user is required to restart.
>
> Each bridge will have its own mdb resulting in a differing seq.
>
So I should use net->dev_base_seq + mdb->seq ?
All of the rest suggestions are taken by me.
Thanks for your detailed review!
