On Mon, 2020-10-05 at 12:16 -0700, Jakub Kicinski wrote:
> On Mon, 05 Oct 2020 20:56:29 +0200 Johannes Berg wrote:
> > On Mon, 2020-10-05 at 08:57 -0700, Jakub Kicinski wrote:
> > > @@ -783,6 +799,9 @@ static const struct genl_ops ethtool_genl_ops[] = {
> > > .start = ethnl_default_start,
> > > .dumpit = ethnl_default_dumpit,
> > > .done = ethnl_default_done,
> > > + .policy = ethnl_rings_get_policy,
> > > + .maxattr = ARRAY_SIZE(ethnl_rings_get_policy) - 1,
> > > +
> > > },
> >
> > If you find some other reason to respin, perhaps remove that blank line
> > :)
> >
> > Unrelated to that, it bothers me a bit that you put here the maxattr as
> > the ARRAY_SIZE(), which is of course fine, but then still have
> >
> > > @@ -127,7 +127,7 @@ const struct ethnl_request_ops
> > > ethnl_privflags_request_ops = {
> > > .max_attr = ETHTOOL_A_PRIVFLAGS_MAX,
> >
> > max_attr here, using the original define
>
> Ah, another good catch, this is obviously no longer needed. I will
> remove those members in v2.
Good point, I misread/misunderstood the code and thought it was still
being used to size the parsing array, but that's of course no longer
there since the genl core now does it.
> > But with the difference it seems to me that it'd be possible to get this
> > mixed up?
>
> Right, I prefer not to have the unnecessary NLA_REJECTS, so my thinking
> was - use the format I like for the new code, but leave the existing
> rejects for a separate series / discussion.
>
> If we remove the rejects we still need something like
>
> extern struct nla_policy policy[lastattr + 1];
Not sure I understand? You're using strict validation (I think), so
attrs that are out of range will be rejected same as NLA_REJECT (well,
with a different message) in __nla_validate_parse():
nla_for_each_attr(nla, head, len, rem) {
u16 type = nla_type(nla);
if (type == 0 || type > maxtype) {
if (validate & NL_VALIDATE_MAXTYPE) {
NL_SET_ERR_MSG_ATTR(extack, nla,
"Unknown attribute type");
return -EINVAL;
}
In fact, if you're using strict validation even the default
(0==NLA_UNSPEC) will be rejected, just like NLA_REJECT.
Or am I confused somewhere?
johannes
