On Wed, 2020-09-30 at 17:05 -0700, Jakub Kicinski wrote:
> Whenever netlink dump uses more than 2 cb->args[] entries
> code gets hard to read. We're about to add more state to
> ctrl_dumppolicy() so create a structure.
>
> Since the structure is typed and clearly named we can remove
> the local fam_id variable and use ctx->fam_id directly.
>
> Signed-off-by: Jakub Kicinski <k...@kernel.org>
> ---
> net/netlink/genetlink.c | 22 +++++++++++++---------
> 1 file changed, 13 insertions(+), 9 deletions(-)
>
> diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
> index 38d8f353dba1..a8001044d8cd 100644
> --- a/net/netlink/genetlink.c
> +++ b/net/netlink/genetlink.c
> @@ -1102,13 +1102,18 @@ static int genl_ctrl_event(int event, const struct
> genl_family *family,
> return 0;
> }
>
> +struct ctrl_dump_policy_ctx {
> + unsigned long state;
Maybe if we do this, also make a "struct netlink_policy_dump_state" in
include/net/netlink.h for the policy dump to use as a state? Right now
it just uses an "unsigned long *state" there.
I feel that would more clearly show what this "state" actually is.
Alternatively, perhaps just rename it to "policy_dump_state"? Yeah,
that's longer, but at least would be very obvious?
> + unsigned int fam_id;
You could make this a u16 I guess, but it doesn't really matter.
> static int ctrl_dumppolicy(struct sk_buff *skb, struct netlink_callback *cb)
> {
> + struct ctrl_dump_policy_ctx *ctx = (void *)cb->args;
I'd also prefer if you stuck a
BUILD_BUG_ON(sizeof(*ctx) > sizeof(cb->args));
here. It's not likely we'll need so much more state here, but would
still be good to check IMHO.
But in general looks good :)
johannes
