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.
> yes, mostly the policies use
> the define to size them, but they didn't really *need* to, and one might
> make an argument that on the policy arrays the size might as well be
> removed (and it be sized automatically based on the contents) since all
> the unspecified attrs are rejected anyway.
>
> 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];
For array_size to work, but I think that's fine. And we'd get a
compiler errors if the sizes don't match up.
> I do see that you still need this to size the attrs for parsing them
> even after patch 2 where this:
>
> > .req_info_size = sizeof(struct privflags_req_info),
> > .reply_data_size = sizeof(struct privflags_reply_data),
> > - .request_policy = privflags_get_policy,
> > + .request_policy = ethnl_privflags_get_policy,
>
> gets removed completely.
>
>
> Perhaps we can look up the genl_ops pointer, or add the ops pointer to
> struct genl_info (could point to the temporary full struct that gets
> populated, size of genl_info itself doesn't matter much since it's on
> the stack and temporary), and then use ops->maxattr instead of
> request_ops->max_attr in ethnl_default_parse()?
Hm, maybe my split of patches 1 and 2 hurts more than it helps.
Let me merge the two in v2.
