Messages in this thread | | | From | <> | Subject | Re: [PATCH net-next v2 3/6] net: dcb: add new rewrite table | Date | Wed, 18 Jan 2023 13:47:57 +0000 |
| |
> > + 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.
| |