Re: [PATCH net-next 1/6] ethtool: wire up get policies to ops

[Johannes Berg <johannes@sipsolutions.net>]

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 - 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?

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()?

johannes