netdev - Re: [PATCH net-next 1/6] devlink: extend devlink

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: <fc3c9135-ad5e-4f32-b852-c08cfb096492@intel.com>
Date: Wed, 24 Apr 2024 11:24:02 +0200
From: Alexander Lobakin <aleksander.lobakin@...el.com>
To: Przemek Kitszel <przemyslaw.kitszel@...el.com>
CC: Jiri Pirko <jiri@...dia.com>, Mateusz Polchlopek
	<mateusz.polchlopek@...el.com>, Tony Nguyen <anthony.l.nguyen@...el.com>,
	<davem@...emloft.net>, <kuba@...nel.org>, <pabeni@...hat.com>,
	<edumazet@...gle.com>, <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next 1/6] devlink: extend devlink_param *set pointer

From: Przemek Kitszel <przemyslaw.kitszel@...el.com>
Date: Wed, 24 Apr 2024 11:20:49 +0200

> On 4/24/24 11:05, Alexander Lobakin wrote:
>> From: Tony Nguyen <anthony.l.nguyen@...el.com>
>> Date: Mon, 22 Apr 2024 13:39:06 -0700
>>
>>> From: Mateusz Polchlopek <mateusz.polchlopek@...el.com>
>>>
>>> Extend devlink_param *set function pointer to take extack as a param.
>>> Sometimes it is needed to pass information to the end user from set
>>> function. It is more proper to use for that netlink instead of passing
>>> message to dmesg.
>>>
>>> Reviewed-by: Jiri Pirko <jiri@...dia.com>
>>> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@...el.com>
>>> Signed-off-by: Mateusz Polchlopek <mateusz.polchlopek@...el.com>
>>> Signed-off-by: Tony Nguyen <anthony.l.nguyen@...el.com>
>>
>> [...]
>>
>>> diff --git a/include/net/devlink.h b/include/net/devlink.h
>>> index d31769a116ce..35eb0f884386 100644
>>> --- a/include/net/devlink.h
>>> +++ b/include/net/devlink.h
>>> @@ -483,7 +483,8 @@ struct devlink_param {
>>>       int (*get)(struct devlink *devlink, u32 id,
>>>              struct devlink_param_gset_ctx *ctx);
>>>       int (*set)(struct devlink *devlink, u32 id,
>>> -           struct devlink_param_gset_ctx *ctx);
>>> +           struct devlink_param_gset_ctx *ctx,
>>> +           struct netlink_ext_ack *extack);
>>
>> Sorry for the late comment. Can't we embed extack to
>> devlink_param_gset_ctx instead? It would take much less lines.
> 
> But then we will want to remove the extack param from .validate() too:
> 
>>
>>>       int (*validate)(struct devlink *devlink, u32 id,
>>>               union devlink_param_value val,
>>>               struct netlink_ext_ack *extack);
> 
> right there.

We don't have &devlink_param_gset_ctx here, only the union.
Extending this union with the extack requires converting it to a struct
(which would have extack + this union), which is again a conversion of
all the drivers :z

> This would amount to roughly the same scope for changes, but would spare
> us yet another round when someone would like to extend .get(), so I like
> this idea.

Thanks,
Olek