On Wed, Sep 30, 2020 at 08:36:24PM +0200, Johannes Berg wrote:
> On Wed, 2020-09-30 at 09:44 -0700, Jakub Kicinski wrote:
>
> > I started with a get_policy() callback, but I didn't like it much.
> > Static data is much more pleasant for a client of the API IMHO.
>
> Yeah, true.
>
> > What do you think about "ops light"? Insufficiently flexible?
>
> TBH, I'm not really sure how you'd do it?
>
> Admittedly, it _would_ be nice to reduce struct genl_ops further, I
> could imagine, assuming that doit is far more common than anything else,
> perhaps something like
>
> diff --git a/include/net/genetlink.h b/include/net/genetlink.h
> index b9eb92f3fe86..a5abab50673c 100644
> --- a/include/net/genetlink.h
> +++ b/include/net/genetlink.h
> @@ -125,23 +125,34 @@ genl_dumpit_info(struct netlink_callback *cb)
> return cb->data;
> }
>
> +/**
> + * struct genl_ops_ext - full generic netlink operations
> + * @start: start callback for dumps
> + * @dumpit: callback for dumpers
> + * @done: completion callback for dumps
> + * @policy: policy if different from the family policy
> + * @maxattr: max attr for the policy
> + */
> +struct genl_ops_ext {
> + int (*start)(struct netlink_callback *cb);
> + int (*dumpit)(struct sk_buff *skb,
> + struct netlink_callback *cb);
> + int (*done)(struct netlink_callback *cb);
> + const struct nla_policy *policy;
> + unsigned int maxattr;
> +};
> +
> /**
> * struct genl_ops - generic netlink operations
> * @cmd: command identifier
> * @internal_flags: flags used by the family
> * @flags: flags
> * @doit: standard command callback
> - * @start: start callback for dumps
> - * @dumpit: callback for dumpers
> - * @done: completion callback for dumps
> */
> struct genl_ops {
> int (*doit)(struct sk_buff *skb,
> struct genl_info *info);
> - int (*start)(struct netlink_callback *cb);
> - int (*dumpit)(struct sk_buff *skb,
> - struct netlink_callback *cb);
> - int (*done)(struct netlink_callback *cb);
> + struct genl_ops_ext *extops;
> u8 cmd;
> u8 internal_flags;
> u8 flags;
>
> ...
>
> or perhaps even
>
> diff --git a/include/net/genetlink.h b/include/net/genetlink.h
> index b9eb92f3fe86..9be3fc051400 100644
> --- a/include/net/genetlink.h
> +++ b/include/net/genetlink.h
> @@ -125,29 +125,45 @@ genl_dumpit_info(struct netlink_callback *cb)
> return cb->data;
> }
>
> +/**
> + * struct genl_ops_ext - full generic netlink operations
> + * @start: start callback for dumps
> + * @dumpit: callback for dumpers
> + * @done: completion callback for dumps
> + * @doit: standard command callback
> + * @policy: policy if different from the family policy
> + * @maxattr: max attr for the policy
> + */
> +struct genl_ops_ext {
> + int (*start)(struct netlink_callback *cb);
> + int (*dumpit)(struct sk_buff *skb, struct netlink_callback *cb);
> + int (*done)(struct netlink_callback *cb);
> + int (*doit)(struct sk_buff *skb, struct genl_info *info);
> + const struct nla_policy *policy;
> + unsigned int maxattr;
> +};
> +
> /**
> * struct genl_ops - generic netlink operations
> * @cmd: command identifier
> * @internal_flags: flags used by the family
> * @flags: flags
> * @doit: standard command callback
> - * @start: start callback for dumps
> - * @dumpit: callback for dumpers
> - * @done: completion callback for dumps
> + * @extops: extended ops if needed, must use GENL_EXTOPS()
> */
> struct genl_ops {
> - int (*doit)(struct sk_buff *skb,
> - struct genl_info *info);
> - int (*start)(struct netlink_callback *cb);
> - int (*dumpit)(struct sk_buff *skb,
> - struct netlink_callback *cb);
> - int (*done)(struct netlink_callback *cb);
> + union {
> + int (*doit)(struct sk_buff *skb, struct genl_info *info);
> + struct genl_ops_ext *extops;
> + };
> u8 cmd;
> u8 internal_flags;
> u8 flags;
> u8 validate;
> };
>
> +#define GENL_EXT_OPS(ptr) ((struct genl_ops_ext *)((uintptr_t)(ptr) | 1))
> +
> int genl_register_family(struct genl_family *family);
> int genl_unregister_family(const struct genl_family *family);
> void genl_notify(const struct genl_family *family, struct sk_buff *skb,
>
>
>
>
> But both sort of feel awkward, you have to declare another structure,
> and link it manually to the right place?
>
> There isn't really a _good_ way to express such a thing easily in C?
Not really good either but how about embedding struct genl_ops as first
member of struct genl_ops_ext like we do with sockets?
Michal
