netdev - Re: [RFC net-next 14/15] 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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <c61b9138-f8ec-4c99-663b-d26fa4f1d4a4@intel.com>
Date:   Thu, 15 Dec 2022 11:24:01 -0800
From:   Jacob Keller <jacob.e.keller@...el.com>
To:     Jiri Pirko <jiri@...nulli.us>, Jakub Kicinski <kuba@...nel.org>
CC:     <davem@...emloft.net>, <netdev@...r.kernel.org>,
        <edumazet@...gle.com>, <pabeni@...hat.com>, <leon@...nel.org>
Subject: Re: [RFC net-next 14/15] devlink: add by-instance dump infra



On 12/15/2022 1:11 AM, Jiri Pirko wrote:
>>
>> +static const struct devlink_gen_cmd *devl_gen_cmds[] = {
>> +	[DEVLINK_CMD_RATE_GET]		= &devl_gen_rate_get,
>> +};
> 
> Instead of having this extra list of ops struct, woudn't it make sence
> to rather implement this dumpit_one infra directly as a part of generic
> netlink code? Something like:
> 
>  	{
>  		.cmd = DEVLINK_CMD_RATE_GET,
>  		.doit = devlink_nl_cmd_rate_get_doit,
> 		.dumpit_one = devlink_nl_cmd_rate_get_dumpit_one,
> 		.dumpit_one_walk = devlink_nl_dumpit_one_walk,
>  		.internal_flags = DEVLINK_NL_FLAG_NEED_RATE,
>  		/* can be retrieved by unprivileged users */
>  	},
> 
> Where devlink_nl_dumpit_one_walk would be basically your
> devlink_instance_iter_dump(), it would get an extra arg dumpit_one
> function pointer from generic netlink code to call per item:
> 
> int devlink_nl_dumpit_one_walk(struct sk_buff *msg, struct netlink_callback *cb,
> 			       int (*dumpit_one)(struct sk_buff *msg,
> 						 struct netlink_callback *cb,
> 						 void *priv));
> {
> 	const struct genl_dumpit_info *info = genl_dumpit_info(cb);
> 	struct devlink_nl_dump_state *dump = devl_dump_state(cb);
> 	struct devlink *devlink;
> 	int err = 0;
> 
> 	devlink_dump_for_each_instance_get(msg, dump, devlink) {
> 		devl_lock(devlink);
> 		err = dumpit_one(msg, cb, devlink);
> 		devl_unlock(devlink);
> 		devlink_put(devlink);
> 
> 		if (err)
> 			break;
> 
> 		/* restart sub-object walk for the next instance */
> 		dump->idx = 0;
> 	}
> 
> 	if (err != -EMSGSIZE)
> 		return err;
> 	return msg->len;
> }
> 
> 
> 
> Or we can avoid .dumpit_one_walk() and just have classic .dumpit() which
> would get the dumpit_one() pointer cb->dumpit_one (obtainable by
> a helper doing a proper check-warn_on on null).
> 
> 

I agree, if we can make this part of the generic netlink code without
too much trouble that would be good.