netdev - Re: [PATCH -next 2/4] tun: Make use of str_disabled

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: <56a4c8ec-2cc1-4078-b5d9-fb128be3efeb@huawei.com>
Date: Wed, 4 Sep 2024 10:27:18 +0800
From: Hongbo Li <lihongbo22@...wei.com>
To: Andy Shevchenko <andy@...nel.org>, Jakub Kicinski <kuba@...nel.org>,
	Willem de Bruijn <willemdebruijn.kernel@...il.com>
CC: <kees@...nel.org>, <jasowang@...hat.com>, <davem@...emloft.net>,
	<edumazet@...gle.com>, <pabeni@...hat.com>, <akpm@...ux-foundation.org>,
	<linux-hardening@...r.kernel.org>, <netdev@...r.kernel.org>,
	<linux-mm@...ck.org>
Subject: Re: [PATCH -next 2/4] tun: Make use of str_disabled_enabled helper



On 2024/9/3 23:09, Andy Shevchenko wrote:
> On Tue, Sep 03, 2024 at 02:25:53PM +0800, Hongbo Li wrote:
>> On 2024/9/2 22:30, Willem de Bruijn wrote:
>>> Andy Shevchenko wrote:
>>>> On Sat, Aug 31, 2024 at 01:07:41PM -0700, Jakub Kicinski wrote:
>>>>> On Sat, 31 Aug 2024 17:58:38 +0800 Hongbo Li wrote:
> 
> ...
> 
>>>>>>    		netif_info(tun, drv, tun->dev, "ignored: set checksum %s\n",
>>>>>> -			   arg ? "disabled" : "enabled");
>>>>>> +			   str_disabled_enabled(arg));
>>>>>
>>>>> You don't explain the 'why'. How is this an improvement?
>>>>> nack on this and 2 similar networking changes you sent
>>>>
>>>> Side opinion: This makes the messages more unified and not prone to typos
>>>> and/or grammatical mistakes. Unification allows to shrink binary due to
>>>> linker efforts on string literals deduplication.
>>>
>>> This adds a layer of indirection.
>>>
>>> The original code is immediately obvious. When I see the new code I
>>> have to take a detour through cscope to figure out what it does.
>> If they have used it once, there is no need for more jumps, because it's
>> relatively simple.
>>
>> Using a dedicated function seems very elegant and unified, especially for
>> some string printing situations, such as disable/enable. Even in today's
>> kernel tree, there are several different formats that appear:
>> 'enable/disable', 'enabled/disabled', 'en/dis'.
> 
> Not to mention that the longer word is the more error prone the spelling.
> 
>>> To me, in this case, the benefit is too marginal to justify that.
> 
> Hongbo, perhaps you need to add a top comment to the string_choices.h to
> explain the following:
> 1) the convention to use is str_$TRUE_$FALSE(), where $TRUE and $FALSE the
> respective words printed;
> 2) the pros of having unified output,
> 3) including but not limited to the linker deduplication facilities, making
> the binary smaller.
> 
> With that you may always point people to the ad-hoc documentation.
> 

ok, thank you, and I can try it.

However, with these modifications, I'm not sure whether Willem and Jakub 
agree with the changes. If they don't agree, then I'll have to remove 
this example in the next version. In the future, we can guide other 
developers to use these helpers directly instead of rewriting it themselves.

Thanks,
Hongbo