Re: Genetlink per cmd policies
From: Johannes Berg <johannes@sipsolutions.net>
Date: 2020-09-30 18:36:34
Subsystem:
networking [general], the rest · Maintainers:
"David S. Miller", Eric Dumazet, Jakub Kicinski, Paolo Abeni, Linus Torvalds
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? johannes