lkml.org 
[lkml]   [2022]   [Nov]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    SubjectRe: [PATCH net-next] net: dcb: move getapptrust to separate function
    From
    Date
    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;
    }

    \
     
     \ /
      Last update: 2022-11-11 13:48    [W:2.660 / U:0.028 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site