netdev - Re: [PATCH net-next 13/14] devlink: add by-instance dump infra

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Thu, 5 Jan 2023 10:02:54 +0100
From:   Jiri Pirko <jiri@...nulli.us>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     davem@...emloft.net, netdev@...r.kernel.org, edumazet@...gle.com,
        pabeni@...hat.com, jacob.e.keller@...el.com
Subject: Re: [PATCH net-next 13/14] devlink: add by-instance dump infra

Thu, Jan 05, 2023 at 04:46:04AM CET, kuba@...nel.org wrote:
>On Wed, 4 Jan 2023 17:50:33 +0100 Jiri Pirko wrote:
>> Wed, Jan 04, 2023 at 05:16:35AM CET, kuba@...nel.org wrote:
>> >Most dumpit implementations walk the devlink instances.
>> >This requires careful lock taking and reference dropping.
>> >Factor the loop out and provide just a callback to handle
>> >a single instance dump.
>> >
>> >Convert one user as an example, other users converted
>> >in the next change.
>> >
>> >Slightly inspired by ethtool netlink code.
>
>> >diff --git a/net/devlink/devl_internal.h b/net/devlink/devl_internal.h
>> >index 5adac38454fd..e49b82dd77cd 100644
>> >--- a/net/devlink/devl_internal.h
>> >+++ b/net/devlink/devl_internal.h
>> >@@ -122,6 +122,11 @@ struct devlink_nl_dump_state {
>> > 	};
>> > };
>> > 
>> >+struct devlink_gen_cmd {  
>> 
>> What is "gen"? Generic netlink?
>
>Generic devlink command. In other words the implementation 
>is straightforward enough to factor out the common parts.

Could it be "genl" then?


>
>> Not sure why perhaps "nl" would be fine to be consistent with the
>> rest of the code? Why "cmd"? That looks a bit odd to me.
>> 
>> >+	int (*dump_one)(struct sk_buff *msg, struct devlink
>> >*devlink,
>> >+			struct netlink_callback *cb);  
>> 
>> Do you plan to have more callbacks here? If no, wouldn't it be better
>> to just have typedef and assign the pointer to the dump_one in
>> devl_gen_cmds array?
>
>If I find the time - yes, more refactoring is possible.

Could you elaborate a bit more about that?


>
>> >+};
>> >+
>> > /* Iterate over devlink pointers which were possible to get
>> > reference to.
>> >  * devlink_put() needs to be called for each iterated devlink
>> > pointer
>> >  * in loop body in order to release the reference.
>> >@@ -138,6 +143,9 @@ struct devlink *devlink_get_from_attrs(struct
>> >net *net, struct nlattr **attrs);
>> > void devlink_notify_unregister(struct devlink *devlink);
>> > void devlink_notify_register(struct devlink *devlink);
>> > 
>> >+int devlink_instance_iter_dump(struct sk_buff *msg,
>> >+			       struct netlink_callback *cb);
>> >+
>> > static inline struct devlink_nl_dump_state *
>> > devl_dump_state(struct netlink_callback *cb)
>> > {
>> >@@ -173,6 +181,8 @@ devlink_linecard_get_from_info(struct devlink
>> >*devlink, struct genl_info *info);
>> > void devlink_linecard_put(struct devlink_linecard *linecard);
>> > 
>> > /* Rates */
>> >+extern const struct devlink_gen_cmd devl_gen_rate_get;  
>> 
>> The struct name is *_cmd, not sure why the variable name is *_get
>> Shouldn't it be rather devl_gen_cmd_rate?
>
>It is the implementation of get.. there's also a set command.. 
>which would be under a different index...

I see.


>
>> >+			dump->idx = idx;
>> >+			break;
>> >+		}
>> >+		idx++;
>> > 	}
>> >-out:
>> >-	if (err != -EMSGSIZE)
>> >-		return err;
>> > 
>> >-	return msg->len;
>> >+	return err;
>> > }
>> > 
>> >+const struct devlink_gen_cmd devl_gen_rate_get = {
>> >+	.dump_one		=
>> >devlink_nl_cmd_rate_get_dumpinst,  
>> 
>> dump_one/dumpinst inconsistency in names
>
>Sure...
>
>> >+};
>> >+
>> > static int devlink_nl_cmd_rate_get_doit(struct sk_buff *skb,
>> > 					struct genl_info *info)
>> > {
>> >@@ -9130,7 +9123,7 @@ const struct genl_small_ops devlink_nl_ops[56]
>> >= {
>> > 	{
>> > 		.cmd = DEVLINK_CMD_RATE_GET,
>> > 		.doit = devlink_nl_cmd_rate_get_doit,
>> >-		.dumpit = devlink_nl_cmd_rate_get_dumpit,
>> >+		.dumpit = devlink_instance_iter_dump,  
>> 
>> again, inconsistency:
>> devlink_instance_iter_dumpit
>
>You mean it doesn't have nl, cmd, dump_one in the name?
>Could you *please* at least say what you want the names to be if you're
>sending all those subjective nit picks? :/

Well, I provided a suggested name, not sure why that was not clear.
The point was s/dump/dumpit/ to match the op name.

I'm sorry you find this a subjective nitpick. I believe it is important
to maintain some naming consistency in order to make code readable.
Nothing subjective about it.


>
>I'll call it devlink_nl_instance_iter_dump

devlink_nl_instance_iter_dumpit please.


>
>> > 		.internal_flags = DEVLINK_NL_FLAG_NEED_RATE,
>> > 		/* can be retrieved by unprivileged users */  
>> 
>> Unrelated to this patch, I wonder, why you didn't move devlink_nl_ops
>> along with the rest of the netlink code to netlink.c?
>
>It's explained in the commit message for patch 3 :/

I missed that, sorry.


>
>> > 	},
>> >diff --git a/net/devlink/netlink.c b/net/devlink/netlink.c
>> >index ce1a7d674d14..fcf10c288480 100644
>> >--- a/net/devlink/netlink.c
>> >+++ b/net/devlink/netlink.c
>> >@@ -5,6 +5,7 @@
>> >  */
>> > 
>> > #include <net/genetlink.h>
>> >+#include <net/sock.h>
>> > 
>> > #include "devl_internal.h"
>> > 
>> >@@ -177,6 +178,38 @@ static void devlink_nl_post_doit(const struct
>> >genl_split_ops *ops,
>> > 	devlink_put(devlink);
>> > }
>> > 
>> >+static const struct devlink_gen_cmd *devl_gen_cmds[] = {
>> >+	[DEVLINK_CMD_RATE_GET]		= &devl_gen_rate_get,
>> > 
>> 
>> static const devlink_nl_dump_one_t *devlink_nl_dump_one[] = {
>> 	[DEVLINK_CMD_RATE_GET]	= &devl_nl_rate_dump_one,
>> }
>> Maybe? (not sure how the devlink/devl should be used here though)
>
>Nope.

Awesome.