diff mbox series

[net-next,v2,08/11] devlink: introduce set of macros and use it for split ops definitions

Message ID 20230720121829.566974-9-jiri@resnulli.us (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series devlink: introduce dump selector attr and use it for per-instance dumps | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1344 this patch: 1344
netdev/cc_maintainers success CCed 6 of 6 maintainers
netdev/build_clang success Errors and warnings before: 1365 this patch: 1365
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 1367 this patch: 1367
netdev/checkpatch warning WARNING: line length of 81 exceeds 80 columns
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Jiri Pirko July 20, 2023, 12:18 p.m. UTC 
From: Jiri Pirko <jiri@nvidia.com>

The split ops structures for all commands look pretty much the same.
The are all using the same/similar callbacks.

Introduce a set of macros to make the code shorter and also avoid
possible future copy&paste mistakes and inconsistencies.

Use this macros for already converted commands.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
 net/devlink/netlink.c | 136 ++++++++++++++++++++----------------------
 1 file changed, 66 insertions(+), 70 deletions(-)

Comments

Jakub Kicinski July 25, 2023, 5:38 p.m. UTC | #1 
On Thu, 20 Jul 2023 14:18:26 +0200 Jiri Pirko wrote:
> The split ops structures for all commands look pretty much the same.
> The are all using the same/similar callbacks.
> 
> Introduce a set of macros to make the code shorter and also avoid
> possible future copy&paste mistakes and inconsistencies.
> 
> Use this macros for already converted commands.

If you want to use split ops extensively please use the nlspec
and generate the table automatically. Integrating closer with
the spec will have many benefits.
Jiri Pirko July 31, 2023, 12:21 p.m. UTC | #2 
Tue, Jul 25, 2023 at 07:38:16PM CEST, kuba@kernel.org wrote:
>On Thu, 20 Jul 2023 14:18:26 +0200 Jiri Pirko wrote:
>> The split ops structures for all commands look pretty much the same.
>> The are all using the same/similar callbacks.
>> 
>> Introduce a set of macros to make the code shorter and also avoid
>> possible future copy&paste mistakes and inconsistencies.
>> 
>> Use this macros for already converted commands.
>
>If you want to use split ops extensively please use the nlspec
>and generate the table automatically. Integrating closer with
>the spec will have many benefits.

Yeah, I was thinging about it, it just didn't seem necessary. Okay, will
check that out.

Btw, does that mean that any split-ops usage would require generated
code? If yes, could you please document that somewhere, probably near
the struct?

Thanks!
Jakub Kicinski July 31, 2023, 4:57 p.m. UTC | #3 
On Mon, 31 Jul 2023 14:21:52 +0200 Jiri Pirko wrote:
> >If you want to use split ops extensively please use the nlspec
> >and generate the table automatically. Integrating closer with
> >the spec will have many benefits.  
> 
> Yeah, I was thinging about it, it just didn't seem necessary. Okay, will
> check that out.
> 
> Btw, does that mean that any split-ops usage would require generated
> code? If yes, could you please document that somewhere, probably near
> the struct?

I wrote it somewhere, probably the commit messages for the split ops.
The tools are not 100% ready for partial generation I don't want to
force everyone to do code gen. But the homegrown macros in every family
are a no go.
Jiri Pirko Aug. 1, 2023, 6:41 a.m. UTC | #4 
Mon, Jul 31, 2023 at 06:57:45PM CEST, kuba@kernel.org wrote:
>On Mon, 31 Jul 2023 14:21:52 +0200 Jiri Pirko wrote:
>> >If you want to use split ops extensively please use the nlspec
>> >and generate the table automatically. Integrating closer with
>> >the spec will have many benefits.  
>> 
>> Yeah, I was thinging about it, it just didn't seem necessary. Okay, will
>> check that out.
>> 
>> Btw, does that mean that any split-ops usage would require generated
>> code? If yes, could you please document that somewhere, probably near
>> the struct?
>
>I wrote it somewhere, probably the commit messages for the split ops.

I believe we need to have it written down in actual codebase.


>The tools are not 100% ready for partial generation I don't want to
>force everyone to do code gen. But the homegrown macros in every family
>are a no go.

So you say that if I spell it out without macros, that would be okay?
diff mbox series

Patch

diff --git a/net/devlink/netlink.c b/net/devlink/netlink.c
index cabebff6e7a7..3dae9303cfa7 100644
--- a/net/devlink/netlink.c
+++ b/net/devlink/netlink.c
@@ -232,77 +232,73 @@  int devlink_nl_instance_iter_dumpit(struct sk_buff *msg,
 	return msg->len;
 }
 
+#define __DEVL_NL_OP_DO(cmd_subname, doit_subname, pre_doit_suffix, _validate,	\
+			_maxattr, _policy)					\
+	{									\
+		.cmd = DEVLINK_CMD_##cmd_subname,				\
+		.pre_doit = devlink_nl_pre_doit_##pre_doit_suffix,		\
+		.doit = devlink_nl_cmd_##doit_subname##_doit,			\
+		.post_doit = devlink_nl_post_doit,				\
+		.flags = GENL_CMD_CAP_DO,					\
+		.validate = _validate,						\
+		.maxattr = _maxattr,						\
+		.policy	= _policy,						\
+	}
+
+#define __DEVL_NL_OP_DUMP(cmd_subname, _validate, _maxattr, _policy)		\
+	{									\
+		.cmd = DEVLINK_CMD_##cmd_subname,				\
+		.dumpit = devlink_nl_instance_iter_dumpit,			\
+		.flags = GENL_CMD_CAP_DUMP,					\
+		.validate = _validate,						\
+		.maxattr = _maxattr,						\
+		.policy	= _policy,						\
+	}
+
+#define __DEVL_NL_OP_LEGACY_DO(cmd_subname, doit_subname, pre_doit_suffix,	\
+			       validate)					\
+	__DEVL_NL_OP_DO(cmd_subname, doit_subname, pre_doit_suffix, validate,	\
+			DEVLINK_ATTR_MAX, devlink_nl_policy)
+
+#define __DEVL_NL_OP_LEGACY_DUMP(cmd_subname, validate)				\
+	__DEVL_NL_OP_DUMP(cmd_subname, validate,				\
+			  DEVLINK_ATTR_MAX, devlink_nl_policy)
+
+#define DEVL_NL_OP_LEGACY_DO(cmd_subname, doit_subname, pre_doit_suffix)	\
+	__DEVL_NL_OP_LEGACY_DO(cmd_subname, doit_subname, pre_doit_suffix,	\
+			       GENL_DONT_VALIDATE_STRICT)
+
+#define DEVL_NL_OP_LEGACY_DUMP(cmd_subname)					\
+	__DEVL_NL_OP_LEGACY_DUMP(cmd_subname, GENL_DONT_VALIDATE_DUMP_STRICT)
+
+#define DEVL_NL_OP_LEGACY_STRICT_DO(cmd_subname, doit_subname, pre_doit_suffix)	\
+	__DEVL_NL_OP_LEGACY_DO(cmd_subname, doit_subname, pre_doit_suffix, 0)
+
+#define DEVL_NL_OP_LEGACY_STRICT_DUMP(cmd_subname)				\
+	__DEVL_NL_OP_LEGACY_DUMP(cmd_subname, 0)
+
+#define DEVL_NL_OP_DO(cmd_subname, doit_subname, pre_doit_suffix,		\
+		      maxattr, policy)						\
+	__DEVL_NL_OP_DO(cmd_subname, doit_subname, pre_doit_suffix, 0,		\
+			maxattr, policy)
+
+#define DEVL_NL_OP_DUMP(cmd_subname, maxattr, policy)			\
+	__DEVL_NL_OP_DUMP(cmd_subname, 0, maxattr, policy)
+
 static const struct genl_split_ops devlink_nl_split_ops[] = {
-	{
-		.cmd = DEVLINK_CMD_PORT_GET,
-		.pre_doit = devlink_nl_pre_doit_port,
-		.doit = devlink_nl_cmd_port_get_doit,
-		.post_doit = devlink_nl_post_doit,
-		.flags = GENL_CMD_CAP_DO,
-		.validate = GENL_DONT_VALIDATE_STRICT,
-		.maxattr = DEVLINK_ATTR_MAX,
-		.policy	= devlink_nl_policy,
-	},
-	{
-		.cmd = DEVLINK_CMD_PORT_GET,
-		.dumpit = devlink_nl_instance_iter_dumpit,
-		.flags = GENL_CMD_CAP_DUMP,
-		.validate = GENL_DONT_VALIDATE_DUMP,
-		.maxattr = DEVLINK_ATTR_MAX,
-		.policy	= devlink_nl_policy,
-	},
-	{
-		.cmd = DEVLINK_CMD_PARAM_GET,
-		.pre_doit = devlink_nl_pre_doit_simple,
-		.doit = devlink_nl_cmd_param_get_doit,
-		.post_doit = devlink_nl_post_doit,
-		.flags = GENL_CMD_CAP_DO,
-		.validate = GENL_DONT_VALIDATE_STRICT,
-		.maxattr = DEVLINK_ATTR_MAX,
-		.policy	= devlink_nl_policy,
-	},
-	{
-		.cmd = DEVLINK_CMD_PARAM_GET,
-		.dumpit = devlink_nl_instance_iter_dumpit,
-		.flags = GENL_CMD_CAP_DUMP,
-		.validate = GENL_DONT_VALIDATE_DUMP,
-		.maxattr = DEVLINK_ATTR_MAX,
-		.policy	= devlink_nl_policy,
-	},
-	{
-		.cmd = DEVLINK_CMD_HEALTH_REPORTER_GET,
-		.pre_doit = devlink_nl_pre_doit_port_optional,
-		.doit = devlink_nl_cmd_health_reporter_get_doit,
-		.post_doit = devlink_nl_post_doit,
-		.flags = GENL_CMD_CAP_DO,
-		.validate = GENL_DONT_VALIDATE_STRICT,
-		.maxattr = DEVLINK_ATTR_MAX,
-		.policy	= devlink_nl_policy,
-	},
-	{
-		.cmd = DEVLINK_CMD_HEALTH_REPORTER_GET,
-		.dumpit = devlink_nl_instance_iter_dumpit,
-		.flags = GENL_CMD_CAP_DUMP,
-		.validate = GENL_DONT_VALIDATE_DUMP,
-		.maxattr = DEVLINK_ATTR_MAX,
-		.policy	= devlink_nl_policy,
-	},
-	{
-		.cmd = DEVLINK_CMD_TRAP_GET,
-		.pre_doit = devlink_nl_pre_doit_simple,
-		.doit = devlink_nl_cmd_trap_get_doit,
-		.post_doit = devlink_nl_post_doit,
-		.flags = GENL_CMD_CAP_DO,
-		.maxattr = DEVLINK_ATTR_MAX,
-		.policy	= devlink_nl_policy,
-	},
-	{
-		.cmd = DEVLINK_CMD_TRAP_GET,
-		.dumpit = devlink_nl_instance_iter_dumpit,
-		.flags = GENL_CMD_CAP_DUMP,
-		.maxattr = DEVLINK_ATTR_MAX,
-		.policy	= devlink_nl_policy,
-	},
+	DEVL_NL_OP_LEGACY_DO(PORT_GET, port_get, port),
+	DEVL_NL_OP_LEGACY_DUMP(PORT_GET),
+	DEVL_NL_OP_LEGACY_DO(PARAM_GET, param_get, simple),
+	DEVL_NL_OP_LEGACY_DUMP(PARAM_GET),
+	DEVL_NL_OP_LEGACY_DO(HEALTH_REPORTER_GET, health_reporter_get,
+			     port_optional),
+	DEVL_NL_OP_LEGACY_DUMP(HEALTH_REPORTER_GET),
+	DEVL_NL_OP_LEGACY_STRICT_DO(TRAP_GET, trap_get, simple),
+	DEVL_NL_OP_LEGACY_STRICT_DUMP(TRAP_GET),
+	/* For every newly added command put above this line the set of macros
+	 * DEVL_NL_OP_DO and DEVL_NL_OP_DUMP should be used. Note that
+	 * there is an exception with non-iterator dump implementation.
+	 */
 };
 
 struct genl_family devlink_nl_family __ro_after_init = {