On Wed, 2020-09-30 at 12:46 -0700, Jakub Kicinski wrote:
> This builds (I think) - around 100 extra LoC:
Looks good to me, couple of comments below.
> +/**
> + * struct genl_light_ops - generic netlink operations (small version)
> + * @cmd: command identifier
> + * @internal_flags: flags used by the family
> + * @flags: flags
> + * @validate: validation flags from enum genl_validate_flags
> + * @doit: standard command callback
> + * @dumpit: callback for dumpers
> + *
> + * This is a cut-down version of struct genl_ops for users who don't need
> + * most of the ancillary infra and want to save space.
> + */
> +struct genl_light_ops {
> + int (*doit)(struct sk_buff *skb, struct genl_info *info);
> + int (*dumpit)(struct sk_buff *skb, struct netlink_callback *cb);
Even dumpit is pretty rare (e.g. 10 out of 107 in nl80211) - maybe
remove that even? It's a bit more juggling in nl80211 to actually use
it, but I'm certainly happy to do that myself.
> +static void genl_op_from_full(const struct genl_family *family,
> + unsigned int i, struct genl_ops *op)
> +{
> + memcpy(op, &family->ops[i], sizeof(*op));
What's wrong with struct assignment? :)
*op = family->ops[i];
> + if (!op->maxattr)
> + op->maxattr = family->maxattr;
> + if (!op->policy)
> + op->policy = family->policy;
That doesn't build as is, I think? Or did you have some other patch
below it?
> static int genl_validate_ops(const struct genl_family *family)
> {
[...]
> + n_ops = genl_get_cmd_cnt(family);
> if (!n_ops)
> return 0;
Come to think of it, that check is kinda pointless, the loop won't run
if it's 0 and then we return 0 immediately anyway... whatever :)
> for (i = 0; i < n_ops; i++) {
> - if (ops[i].dumpit == NULL && ops[i].doit == NULL)
> + struct genl_ops op;
> +
> + if (genl_get_cmd_by_index(i, family, &op))
> return -EINVAL;
Maybe WARN_ON() or something? It really ought to not be possible for
that to fail, since you're only iterating to n_ops, so you'd have to
have some consistency issues if that happens.
> - for (j = i + 1; j < n_ops; j++)
> - if (ops[i].cmd == ops[j].cmd)
> + if (op.dumpit == NULL && op.doit == NULL)
> + return -EINVAL;
> + for (j = i + 1; j < n_ops; j++) {
> + struct genl_ops op2;
> +
> + if (genl_get_cmd_by_index(j, family, &op2))
> return -EINVAL;
same here
> + for (i = 0; i < genl_get_cmd_cnt(family); i++) {
> struct nlattr *nest;
> - const struct genl_ops *ops = &family->ops[i];
> - u32 op_flags = ops->flags;
> + struct genl_ops op;
> + u32 op_flags;
> +
> + if (genl_get_cmd_by_index(i, family, &op))
> + goto nla_put_failure;
but actually, same here, so maybe it should just not even be able to
return an error but WARN_ON instead and clear the op, so you have
everything NULL in that case?
I don't really see a case where you'd have the index coming from
userspace and would have to protect against it being bad, or something?
johannes
