| Message ID | 20201001000518.685243-6-kuba@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | genetlink: support per-command policy dump | expand |
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 <kuba@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
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 <kuba@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
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; + unsigned int fam_id; +}; + static int ctrl_dumppolicy(struct sk_buff *skb, struct netlink_callback *cb) { + struct ctrl_dump_policy_ctx *ctx = (void *)cb->args; const struct genl_family *rt; - unsigned int fam_id = cb->args[0]; int err; - if (!fam_id) { + if (!ctx->fam_id) { struct nlattr *tb[CTRL_ATTR_MAX + 1]; err = genlmsg_parse(cb->nlh, &genl_ctrl, tb, @@ -1121,28 +1126,28 @@ static int ctrl_dumppolicy(struct sk_buff *skb, struct netlink_callback *cb) return -EINVAL; if (tb[CTRL_ATTR_FAMILY_ID]) { - fam_id = nla_get_u16(tb[CTRL_ATTR_FAMILY_ID]); + ctx->fam_id = nla_get_u16(tb[CTRL_ATTR_FAMILY_ID]); } else { rt = genl_family_find_byname( nla_data(tb[CTRL_ATTR_FAMILY_NAME])); if (!rt) return -ENOENT; - fam_id = rt->id; + ctx->fam_id = rt->id; } } - rt = genl_family_find_byid(fam_id); + rt = genl_family_find_byid(ctx->fam_id); if (!rt) return -ENOENT; if (!rt->policy) return -ENODATA; - err = netlink_policy_dump_start(rt->policy, rt->maxattr, &cb->args[1]); + err = netlink_policy_dump_start(rt->policy, rt->maxattr, &ctx->state); if (err) return err; - while (netlink_policy_dump_loop(&cb->args[1])) { + while (netlink_policy_dump_loop(&ctx->state)) { void *hdr; struct nlattr *nest; @@ -1159,7 +1164,7 @@ static int ctrl_dumppolicy(struct sk_buff *skb, struct netlink_callback *cb) if (!nest) goto nla_put_failure; - if (netlink_policy_dump_write(skb, cb->args[1])) + if (netlink_policy_dump_write(skb, ctx->state)) goto nla_put_failure; nla_nest_end(skb, nest); @@ -1172,7 +1177,6 @@ static int ctrl_dumppolicy(struct sk_buff *skb, struct netlink_callback *cb) break; } - cb->args[0] = fam_id; return skb->len; }
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 <kuba@kernel.org> --- net/netlink/genetlink.c | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-)