Hi Nikolay,
On Tue, 10 Dec 2019 09:49:59 +0200, Nikolay Aleksandrov <nikolay@xxxxxxxxxxxxxxxxxxx> wrote:
> Why did you send the bridge patch again ? Does it have any changes ?
The second iproute2 patch does not include the include guards update, but
I kept the bridge_stp_stats structure and the BRIDGE_XSTATS_STP definition
otherwise iproute2 wouldn't compile.
>
> Why do you need percpu ? All of these seem to be incremented with the
> bridge lock held. A few more comments below.
All other xstats are incremented percpu, I simply followed the pattern.
> > struct net_bridge_port *p
> > = container_of(kobj, struct net_bridge_port, kobj);
> > + free_percpu(p->stp_stats);
>
> Please leave a new line between local var declaration and the code. I know
> it was missing, but you can add it now. :)
OK.
> > + if (p) {
> > + struct bridge_stp_xstats xstats;
>
> Please rename the local var here, using just xstats is misleading.
> Maybe stp_xstats ?
This isn't misleading to me since its scope is limited to the current block
and not the entire function. The block above dumping the VLAN xstats is
using a local "struct br_vlan_stats stats" variable for example.
>
> > +
> > + br_stp_get_xstats(p, &xstats);
> > +
> > + if (nla_put(skb, BRIDGE_XSTATS_STP, sizeof(xstats), &xstats))
> > + goto nla_put_failure;
>
> Could you please follow how mcast xstats are dumped and do something similar ?
> It'd be nice to have similar code to audit.
Sure. I would also love to have easily auditable code in net/bridge. For
the bridge STP xstats I followed the VLAN xstats code above, which does:
if (nla_put(skb, BRIDGE_XSTATS_VLAN, sizeof(vxi), &vxi))
goto nla_put_failure;
But I can change the STP xstats code to the following:
if (p) {
nla = nla_reserve_64bit(skb, BRIDGE_XSTATS_STP,
sizeof(struct bridge_stp_xstats),
BRIDGE_XSTATS_PAD);
if (!nla)
goto nla_put_failure;
br_stp_get_xstats(p, nla_data(nla));
}
Would that be preferred?
Thanks,
Vivien
