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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <598d25c1-3361-4f01-bbaf-ab335fba3da3@intel.com>
Date: Wed, 24 Apr 2024 11:52:27 +0200
From: Mateusz Polchlopek <mateusz.polchlopek@...el.com>
To: Alexander Lobakin <aleksander.lobakin@...el.com>, Przemek Kitszel
	<przemyslaw.kitszel@...el.com>
CC: Jiri Pirko <jiri@...dia.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



On 4/24/2024 11:24 AM, Alexander Lobakin wrote:
> 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 : >

Makes sense. I also have to take a look on docs issue reported, so
Your's suggestion I will add in the next version too.

>> 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