Messages in this thread Patch in this message | | | Subject | Re: [PATCH net-next] net: dcb: move getapptrust to separate function | From | Joe Perches <> | Date | Fri, 11 Nov 2022 04:47:45 -0800 |
| |
On Fri, 2022-11-11 at 06:45 +0000, Daniel.Machon@microchip.com wrote: > Den Thu, Nov 10, 2022 at 05:30:43PM +0100 skrev Petr Machata: > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > > > Daniel Machon <daniel.machon@microchip.com> writes: > > > > > diff --git a/net/dcb/dcbnl.c b/net/dcb/dcbnl.c > > > index cec0632f96db..3f4d88c1ec78 100644 > > > --- a/net/dcb/dcbnl.c > > > +++ b/net/dcb/dcbnl.c > > > @@ -1060,11 +1060,52 @@ static int dcbnl_build_peer_app(struct net_device *netdev, struct sk_buff* skb, > > > return err; > > > } > > > > > > +static int dcbnl_getapptrust(struct net_device *netdev, struct sk_buff *skb) > > > +{ > > > + const struct dcbnl_rtnl_ops *ops = netdev->dcbnl_ops; > > > + int nselectors, err; > > > + u8 *selectors; > > > + > > > + selectors = kzalloc(IEEE_8021QAZ_APP_SEL_MAX + 1, GFP_KERNEL); > > > + if (!selectors) > > > + return -ENOMEM; > > > + > > > + err = ops->dcbnl_getapptrust(netdev, selectors, &nselectors); > > > + > > > + if (!err) { > > > + struct nlattr *apptrust; > > > + int i; > > > > (Maybe consider moving these up to the function scope. This scope > > business made sense in the generic function, IMHO is not as useful with > > a focused function like this one.) > > I dont mind doing that, however, this 'scope business' is just staying true > to the rest of the dcbnl code :-) - that said, I think I agree with your > point. > > > > > > + > > > + err = -EMSGSIZE; > > > + > > > + apptrust = nla_nest_start(skb, DCB_ATTR_DCB_APP_TRUST_TABLE); > > > + if (!apptrust) > > > + goto nla_put_failure; > > > + > > > + for (i = 0; i < nselectors; i++) { > > > + enum ieee_attrs_app type = > > > + dcbnl_app_attr_type_get(selectors[i]); > > > > Doesn't checkpatch warn about this? There should be a blank line after > > the variable declaration block. (Even if there wasn't one there in the > > original code either.) > > Nope, no warning. And I think it has something to do with the way the line > is split.
yup.
And style trivia:
I suggest adding error types after specific errors, reversing the test and unindenting the code too.
Something like:
err = ops->dcbnl_getapptrust(netdev, selectors, &nselectors); if (err) { err = 0; goto out; }
apptrust = nla_nest_start(skb, DCB_ATTR_DCB_APP_TRUST_TABLE); if (!apptrust) { err = -EMSGSIZE; goto out; }
for (i = 0; i < nselectors; i++) { enum ieee_attrs_app type = dcbnl_app_attr_type_get(selectors[i]); err = nla_put_u8(skb, type, selectors[i]); if (err) { nla_nest_cancel(skb, apptrust); goto out; } } nla_nest_end(skb, apptrust);
err = 0;
out: kfree(selectors); return err; }
| |