On Thu, Oct 01, 2020 at 09:48:31AM +0200, Johannes Berg wrote:
> 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.
Definitely. And it should rather be cb->ctx than cb->args as on 32-bit
architectures, cb->args is technically only 24 bytes while cb->ctx is
always 48 bytes long. After all, the comment in the structure definition
says args is deprecated.
Michal
