Re: [PATCH net-next v2 3/6] net: dcb: add new rewrite table

[Daniel.Machon]

> > +     rewr = nla_nest_start_noflag(skb, DCB_ATTR_DCB_REWR_TABLE);
> > +     if (!rewr)
> > +             return -EMSGSIZE;
> 
> This being new code, don't use _noflag please.

Ack.

> 
> > +
> > +     spin_lock_bh(&dcb_lock);
> > +     list_for_each_entry(itr, &dcb_rewr_list, list) {
> > +             if (itr->ifindex == netdev->ifindex) {
> > +                     enum ieee_attrs_app type =
> > +                             dcbnl_app_attr_type_get(itr->app.selector);
> > +                     err = nla_put(skb, type, sizeof(itr->app), &itr->app);
> > +                     if (err) {
> > +                             spin_unlock_bh(&dcb_lock);
> 
> This should cancel the nest started above.

Yes, it should.

> 
> I wonder if it would be cleaner in a separate function, so that there
> can be a dedicated clean-up block to goto.

Well yes. That would make sense if the function were reused for both APP
and rewr.

Though in the APP equivalent code, nla_nest_start_noflag is used, and
dcbnl_ops->getdcbx() is called. Is there any userspace side-effect of
using nla_nest_start for APP too?

dcbnl_ops->getdcbx() would then be left outside of the shared function.
Does that call even have to hold the dcb_lock? Not as far as I can tell.

something like:

err = dcbnl_app_table_get(ndev, skb, &dcb_app_list,
			  DCB_ATTR_IEEE_APP_TABLE);
if (err)
	return -EMSGSIZE;

err = dcbnl_app_table_get(ndev, skb, &dcb_rewr_list,
			  DCB_ATTR_DCB_REWR_TABLE);
if (err)
        return -EMSGSIZE;

if (netdev->dcbnl_ops->getdcbx)
	dcbx = netdev->dcbnl_ops->getdcbx(netdev); <-- without lock held
else
	dcbx = -EOPNOTSUPP;

Let me hear your thoughts.