Re: [PATCH net-next v1 2/2] bridge: export multicast database via netlink

[Cong Wang <amwang@redhat.com>]

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!