diff mbox series

[-next,2/4] tun: Make use of str_disabled_enabled helper

Message ID 20240831095840.4173362-3-lihongbo22@huawei.com (mailing list archive)
State Superseded
Headers show
Series Introduce several opposite string choice helpers | expand

Commit Message

Hongbo Li Aug. 31, 2024, 9:58 a.m. UTC
Use str_disabled_enabled() helper instead of open
coding the same.

Signed-off-by: Hongbo Li <lihongbo22@huawei.com>
---
 drivers/net/tun.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Jakub Kicinski Aug. 31, 2024, 8:07 p.m. UTC | #1
On Sat, 31 Aug 2024 17:58:38 +0800 Hongbo Li wrote:
> Use str_disabled_enabled() helper instead of open
> coding the same.

> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index 6fe5e8f7017c..29647704bda8 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -3178,7 +3178,7 @@ static long __tun_chr_ioctl(struct file *file, unsigned int cmd,
>  
>  		/* [unimplemented] */
>  		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
Hongbo Li Sept. 2, 2024, 1:27 a.m. UTC | #2
On 2024/9/1 4:07, Jakub Kicinski wrote:
> On Sat, 31 Aug 2024 17:58:38 +0800 Hongbo Li wrote:
>> Use str_disabled_enabled() helper instead of open
>> coding the same.
> 
>> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
>> index 6fe5e8f7017c..29647704bda8 100644
>> --- a/drivers/net/tun.c
>> +++ b/drivers/net/tun.c
>> @@ -3178,7 +3178,7 @@ static long __tun_chr_ioctl(struct file *file, unsigned int cmd,
>>   
>>   		/* [unimplemented] */
>>   		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
> 
This just give an example of using lib/string_choices' helper which 
prevents the function from being removed due to detection of non-use. 
These helpers are convenient. It's functionally equivalent, this avoids 
the dispersion of such writing styles( ? XXX : noXXX) everywhere.

Thanks,
Hongbo
Gal Pressman Sept. 2, 2024, 6:10 a.m. UTC | #3
On 31/08/2024 23:07, Jakub Kicinski wrote:
> On Sat, 31 Aug 2024 17:58:38 +0800 Hongbo Li wrote:
>> Use str_disabled_enabled() helper instead of open
>> coding the same.
> 
>> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
>> index 6fe5e8f7017c..29647704bda8 100644
>> --- a/drivers/net/tun.c
>> +++ b/drivers/net/tun.c
>> @@ -3178,7 +3178,7 @@ static long __tun_chr_ioctl(struct file *file, unsigned int cmd,
>>  
>>  		/* [unimplemented] */
>>  		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
> 

Are you against the concept of string_choices in general, or this
specific change?
Andy Shevchenko Sept. 2, 2024, 10:49 a.m. UTC | #4
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:
> > Use str_disabled_enabled() helper instead of open
> > coding the same.

...

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

That said, I see an improvement here, however it might be not recognised
as a Big Win.

And yes, I agree on the commit message poor explanations.
Willem de Bruijn Sept. 2, 2024, 2:30 p.m. UTC | #5
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:
> > > Use str_disabled_enabled() helper instead of open
> > > coding the same.
> 
> ...
> 
> > >  		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.

To me, in this case, the benefit is too marginal to justify that.
Jakub Kicinski Sept. 2, 2024, 4:11 p.m. UTC | #6
On Mon, 2 Sep 2024 09:10:33 +0300 Gal Pressman wrote:
> > You don't explain the 'why'. How is this an improvement?
> > nack on this and 2 similar networking changes you sent
> 
> Are you against the concept of string_choices in general, or this
> specific change?

Willem verbalized my opinion better than I can.

If a (driver|subsystem) maintainer likes string_choices that's
perfectly fine, but IMHO they are not worth converting to, and
not something we should suggest during review.
Hongbo Li Sept. 3, 2024, 6:25 a.m. UTC | #7
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:
>>>> Use str_disabled_enabled() helper instead of open
>>>> coding the same.
>>
>> ...
>>
>>>>   		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'.

Thanks,
Hongbo

> 
> To me, in this case, the benefit is too marginal to justify that.
Andy Shevchenko Sept. 3, 2024, 3:09 p.m. UTC | #8
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.
Hongbo Li Sept. 4, 2024, 2:27 a.m. UTC | #9
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
Jakub Kicinski Sept. 4, 2024, 2:33 p.m. UTC | #10
On Wed, 4 Sep 2024 10:27:18 +0800 Hongbo Li wrote:
> 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.

This and, to be clear, patch 4 as well.

> In the future, we can guide other 
> developers to use these helpers directly instead of rewriting it themselves.
diff mbox series

Patch

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 6fe5e8f7017c..29647704bda8 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -3178,7 +3178,7 @@  static long __tun_chr_ioctl(struct file *file, unsigned int cmd,
 
 		/* [unimplemented] */
 		netif_info(tun, drv, tun->dev, "ignored: set checksum %s\n",
-			   arg ? "disabled" : "enabled");
+			   str_disabled_enabled(arg));
 		break;
 
 	case TUNSETPERSIST: