* [PATCH net-next][resend v1 1/1] netlink: Don't use int as bool in netlink_update_socket_mc() @ 2023-07-10 10:06 Andy Shevchenko 2023-07-11 6:33 ` Leon Romanovsky 2023-07-12 0:43 ` Jakub Kicinski 0 siblings, 2 replies; 11+ messages in thread From: Andy Shevchenko @ 2023-07-10 10:06 UTC (permalink / raw) To: Jakub Kicinski, netdev, linux-kernel Cc: David S. Miller, Eric Dumazet, Paolo Abeni, Andy Shevchenko The bit operations take boolean parameter and return also boolean (in test_bit()-like cases). Don't threat booleans as integers when it's not needed. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- net/netlink/af_netlink.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c index 383631873748..d81e7a43944c 100644 --- a/net/netlink/af_netlink.c +++ b/net/netlink/af_netlink.c @@ -1623,9 +1623,10 @@ EXPORT_SYMBOL(netlink_set_err); /* must be called with netlink table grabbed */ static void netlink_update_socket_mc(struct netlink_sock *nlk, unsigned int group, - int is_new) + bool new) { - int old, new = !!is_new, subscriptions; + int subscriptions; + bool old; old = test_bit(group - 1, nlk->groups); subscriptions = nlk->subscriptions - old + new; @@ -2152,7 +2153,7 @@ void __netlink_clear_multicast_users(struct sock *ksk, unsigned int group) struct netlink_table *tbl = &nl_table[ksk->sk_protocol]; sk_for_each_bound(sk, &tbl->mc_list) - netlink_update_socket_mc(nlk_sk(sk), group, 0); + netlink_update_socket_mc(nlk_sk(sk), group, false); } struct nlmsghdr * -- 2.40.0.1.gaa8946217a0b ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH net-next][resend v1 1/1] netlink: Don't use int as bool in netlink_update_socket_mc() 2023-07-10 10:06 [PATCH net-next][resend v1 1/1] netlink: Don't use int as bool in netlink_update_socket_mc() Andy Shevchenko @ 2023-07-11 6:33 ` Leon Romanovsky 2023-07-11 10:21 ` Paolo Abeni 2023-07-12 0:43 ` Jakub Kicinski 1 sibling, 1 reply; 11+ messages in thread From: Leon Romanovsky @ 2023-07-11 6:33 UTC (permalink / raw) To: Andy Shevchenko Cc: Jakub Kicinski, netdev, linux-kernel, David S. Miller, Eric Dumazet, Paolo Abeni On Mon, Jul 10, 2023 at 01:06:24PM +0300, Andy Shevchenko wrote: > The bit operations take boolean parameter and return also boolean > (in test_bit()-like cases). Don't threat booleans as integers when > it's not needed. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > net/netlink/af_netlink.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c > index 383631873748..d81e7a43944c 100644 > --- a/net/netlink/af_netlink.c > +++ b/net/netlink/af_netlink.c > @@ -1623,9 +1623,10 @@ EXPORT_SYMBOL(netlink_set_err); > /* must be called with netlink table grabbed */ > static void netlink_update_socket_mc(struct netlink_sock *nlk, > unsigned int group, > - int is_new) > + bool new) > { > - int old, new = !!is_new, subscriptions; > + int subscriptions; > + bool old; > > old = test_bit(group - 1, nlk->groups); > subscriptions = nlk->subscriptions - old + new; So what is the outcome of "int - bool + bool" in the line above? > @@ -2152,7 +2153,7 @@ void __netlink_clear_multicast_users(struct sock *ksk, unsigned int group) > struct netlink_table *tbl = &nl_table[ksk->sk_protocol]; > > sk_for_each_bound(sk, &tbl->mc_list) > - netlink_update_socket_mc(nlk_sk(sk), group, 0); > + netlink_update_socket_mc(nlk_sk(sk), group, false); > } > > struct nlmsghdr * > -- > 2.40.0.1.gaa8946217a0b > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net-next][resend v1 1/1] netlink: Don't use int as bool in netlink_update_socket_mc() 2023-07-11 6:33 ` Leon Romanovsky @ 2023-07-11 10:21 ` Paolo Abeni 2023-07-11 10:54 ` Andy Shevchenko 0 siblings, 1 reply; 11+ messages in thread From: Paolo Abeni @ 2023-07-11 10:21 UTC (permalink / raw) To: Leon Romanovsky, Andy Shevchenko Cc: Jakub Kicinski, netdev, linux-kernel, David S. Miller, Eric Dumazet On Tue, 2023-07-11 at 09:33 +0300, Leon Romanovsky wrote: > On Mon, Jul 10, 2023 at 01:06:24PM +0300, Andy Shevchenko wrote: > > The bit operations take boolean parameter and return also boolean > > (in test_bit()-like cases). Don't threat booleans as integers when > > it's not needed. > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > --- > > net/netlink/af_netlink.c | 7 ++++--- > > 1 file changed, 4 insertions(+), 3 deletions(-) > > > > diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c > > index 383631873748..d81e7a43944c 100644 > > --- a/net/netlink/af_netlink.c > > +++ b/net/netlink/af_netlink.c > > @@ -1623,9 +1623,10 @@ EXPORT_SYMBOL(netlink_set_err); > > /* must be called with netlink table grabbed */ > > static void netlink_update_socket_mc(struct netlink_sock *nlk, > > unsigned int group, > > - int is_new) > > + bool new) > > { > > - int old, new = !!is_new, subscriptions; > > + int subscriptions; > > + bool old; > > > > old = test_bit(group - 1, nlk->groups); > > subscriptions = nlk->subscriptions - old + new; > > So what is the outcome of "int - bool + bool" in the line above? FTR, I agree with Leon, the old code is more readable to me/I don't see a practical gain with this change. Cheers, Paolo ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net-next][resend v1 1/1] netlink: Don't use int as bool in netlink_update_socket_mc() 2023-07-11 10:21 ` Paolo Abeni @ 2023-07-11 10:54 ` Andy Shevchenko 2023-07-11 12:20 ` Leon Romanovsky 0 siblings, 1 reply; 11+ messages in thread From: Andy Shevchenko @ 2023-07-11 10:54 UTC (permalink / raw) To: Paolo Abeni Cc: Leon Romanovsky, Jakub Kicinski, netdev, linux-kernel, David S. Miller, Eric Dumazet On Tue, Jul 11, 2023 at 12:21:12PM +0200, Paolo Abeni wrote: > On Tue, 2023-07-11 at 09:33 +0300, Leon Romanovsky wrote: > > On Mon, Jul 10, 2023 at 01:06:24PM +0300, Andy Shevchenko wrote: > > > The bit operations take boolean parameter and return also boolean > > > (in test_bit()-like cases). Don't threat booleans as integers when > > > it's not needed. > > > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > --- > > > net/netlink/af_netlink.c | 7 ++++--- > > > 1 file changed, 4 insertions(+), 3 deletions(-) > > > > > > diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c > > > index 383631873748..d81e7a43944c 100644 > > > --- a/net/netlink/af_netlink.c > > > +++ b/net/netlink/af_netlink.c > > > @@ -1623,9 +1623,10 @@ EXPORT_SYMBOL(netlink_set_err); > > > /* must be called with netlink table grabbed */ > > > static void netlink_update_socket_mc(struct netlink_sock *nlk, > > > unsigned int group, > > > - int is_new) > > > + bool new) > > > { > > > - int old, new = !!is_new, subscriptions; > > > + int subscriptions; > > > + bool old; > > > > > > old = test_bit(group - 1, nlk->groups); > > > subscriptions = nlk->subscriptions - old + new; > > > > So what is the outcome of "int - bool + bool" in the line above? The same as with int - int [0 .. 1] + int [0 .. 1]. Note, the code _already_ uses boolean as integers. > FTR, I agree with Leon, the old code is more readable to me/I don't see > a practical gain with this change. This change does not change the status quo. The code uses booleans as integers already (in the callers). As I mentioned earlier, the purity of the code (converting booleans to integers beforehand) adds unneeded churn and with this change code becomes cleaner at least for the (existing) callers. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net-next][resend v1 1/1] netlink: Don't use int as bool in netlink_update_socket_mc() 2023-07-11 10:54 ` Andy Shevchenko @ 2023-07-11 12:20 ` Leon Romanovsky 2023-07-11 12:45 ` Andy Shevchenko 0 siblings, 1 reply; 11+ messages in thread From: Leon Romanovsky @ 2023-07-11 12:20 UTC (permalink / raw) To: Andy Shevchenko Cc: Paolo Abeni, Jakub Kicinski, netdev, linux-kernel, David S. Miller, Eric Dumazet On Tue, Jul 11, 2023 at 01:54:18PM +0300, Andy Shevchenko wrote: > On Tue, Jul 11, 2023 at 12:21:12PM +0200, Paolo Abeni wrote: > > On Tue, 2023-07-11 at 09:33 +0300, Leon Romanovsky wrote: > > > On Mon, Jul 10, 2023 at 01:06:24PM +0300, Andy Shevchenko wrote: > > > > The bit operations take boolean parameter and return also boolean > > > > (in test_bit()-like cases). Don't threat booleans as integers when > > > > it's not needed. > > > > > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > > --- > > > > net/netlink/af_netlink.c | 7 ++++--- > > > > 1 file changed, 4 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c > > > > index 383631873748..d81e7a43944c 100644 > > > > --- a/net/netlink/af_netlink.c > > > > +++ b/net/netlink/af_netlink.c > > > > @@ -1623,9 +1623,10 @@ EXPORT_SYMBOL(netlink_set_err); > > > > /* must be called with netlink table grabbed */ > > > > static void netlink_update_socket_mc(struct netlink_sock *nlk, > > > > unsigned int group, > > > > - int is_new) > > > > + bool new) > > > > { > > > > - int old, new = !!is_new, subscriptions; > > > > + int subscriptions; > > > > + bool old; > > > > > > > > old = test_bit(group - 1, nlk->groups); > > > > subscriptions = nlk->subscriptions - old + new; > > > > > > So what is the outcome of "int - bool + bool" in the line above? > > The same as with int - int [0 .. 1] + int [0 .. 1]. No, it is not. bool is defined as _Bool C99 type, so strictly speaking you are mixing types int - _Bool + _Bool. Thanks > > Note, the code _already_ uses boolean as integers. > > > FTR, I agree with Leon, the old code is more readable to me/I don't see > > a practical gain with this change. > > This change does not change the status quo. The code uses booleans as integers > already (in the callers). > > As I mentioned earlier, the purity of the code (converting booleans to integers > beforehand) adds unneeded churn and with this change code becomes cleaner at > least for the (existing) callers. > > -- > With Best Regards, > Andy Shevchenko > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net-next][resend v1 1/1] netlink: Don't use int as bool in netlink_update_socket_mc() 2023-07-11 12:20 ` Leon Romanovsky @ 2023-07-11 12:45 ` Andy Shevchenko 2023-07-11 13:32 ` Leon Romanovsky 0 siblings, 1 reply; 11+ messages in thread From: Andy Shevchenko @ 2023-07-11 12:45 UTC (permalink / raw) To: Leon Romanovsky Cc: Paolo Abeni, Jakub Kicinski, netdev, linux-kernel, David S. Miller, Eric Dumazet On Tue, Jul 11, 2023 at 03:20:12PM +0300, Leon Romanovsky wrote: > On Tue, Jul 11, 2023 at 01:54:18PM +0300, Andy Shevchenko wrote: > > On Tue, Jul 11, 2023 at 12:21:12PM +0200, Paolo Abeni wrote: > > > On Tue, 2023-07-11 at 09:33 +0300, Leon Romanovsky wrote: > > > > On Mon, Jul 10, 2023 at 01:06:24PM +0300, Andy Shevchenko wrote: ... > > > > So what is the outcome of "int - bool + bool" in the line above? > > > > The same as with int - int [0 .. 1] + int [0 .. 1]. > > No, it is not. bool is defined as _Bool C99 type, so strictly speaking > you are mixing types int - _Bool + _Bool. 1. The original code already does that. You still haven't reacted on that. 2. Is what you are telling a problem? -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net-next][resend v1 1/1] netlink: Don't use int as bool in netlink_update_socket_mc() 2023-07-11 12:45 ` Andy Shevchenko @ 2023-07-11 13:32 ` Leon Romanovsky 2023-07-11 13:44 ` Andy Shevchenko 0 siblings, 1 reply; 11+ messages in thread From: Leon Romanovsky @ 2023-07-11 13:32 UTC (permalink / raw) To: Andy Shevchenko Cc: Paolo Abeni, Jakub Kicinski, netdev, linux-kernel, David S. Miller, Eric Dumazet On Tue, Jul 11, 2023 at 03:45:34PM +0300, Andy Shevchenko wrote: > On Tue, Jul 11, 2023 at 03:20:12PM +0300, Leon Romanovsky wrote: > > On Tue, Jul 11, 2023 at 01:54:18PM +0300, Andy Shevchenko wrote: > > > On Tue, Jul 11, 2023 at 12:21:12PM +0200, Paolo Abeni wrote: > > > > On Tue, 2023-07-11 at 09:33 +0300, Leon Romanovsky wrote: > > > > > On Mon, Jul 10, 2023 at 01:06:24PM +0300, Andy Shevchenko wrote: > > ... > > > > > > So what is the outcome of "int - bool + bool" in the line above? > > > > > > The same as with int - int [0 .. 1] + int [0 .. 1]. > > > > No, it is not. bool is defined as _Bool C99 type, so strictly speaking > > you are mixing types int - _Bool + _Bool. > > 1. The original code already does that. You still haven't reacted on that. The original code was int - int + int. > 2. Is what you are telling a problema? No, I'm saying that you took perfectly correct code which had all types aligned and changed it to have mixed type arithmetic. Thanks > > -- > With Best Regards, > Andy Shevchenko > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net-next][resend v1 1/1] netlink: Don't use int as bool in netlink_update_socket_mc() 2023-07-11 13:32 ` Leon Romanovsky @ 2023-07-11 13:44 ` Andy Shevchenko 2023-07-11 17:10 ` Leon Romanovsky 0 siblings, 1 reply; 11+ messages in thread From: Andy Shevchenko @ 2023-07-11 13:44 UTC (permalink / raw) To: Leon Romanovsky Cc: Paolo Abeni, Jakub Kicinski, netdev, linux-kernel, David S. Miller, Eric Dumazet On Tue, Jul 11, 2023 at 04:32:59PM +0300, Leon Romanovsky wrote: > On Tue, Jul 11, 2023 at 03:45:34PM +0300, Andy Shevchenko wrote: > > On Tue, Jul 11, 2023 at 03:20:12PM +0300, Leon Romanovsky wrote: > > > On Tue, Jul 11, 2023 at 01:54:18PM +0300, Andy Shevchenko wrote: > > > > On Tue, Jul 11, 2023 at 12:21:12PM +0200, Paolo Abeni wrote: > > > > > On Tue, 2023-07-11 at 09:33 +0300, Leon Romanovsky wrote: > > > > > > On Mon, Jul 10, 2023 at 01:06:24PM +0300, Andy Shevchenko wrote: ... > > > > > > So what is the outcome of "int - bool + bool" in the line above? > > > > > > > > The same as with int - int [0 .. 1] + int [0 .. 1]. > > > > > > No, it is not. bool is defined as _Bool C99 type, so strictly speaking > > > you are mixing types int - _Bool + _Bool. > > > > 1. The original code already does that. You still haven't reacted on that. > > The original code was int - int + int. No. You missed the callers part. They are using boolean. > > 2. Is what you are telling a problema? > > No, I'm saying that you took perfectly correct code which had all types > aligned and changed it to have mixed type arithmetic. And after this change it's perfectly correct code with less letters and hidden promotions (as a parameter to the function) and hence requires less cognitive energy to parse. So, the bottom line is the commit message you don't like, is it so? -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net-next][resend v1 1/1] netlink: Don't use int as bool in netlink_update_socket_mc() 2023-07-11 13:44 ` Andy Shevchenko @ 2023-07-11 17:10 ` Leon Romanovsky 2023-07-11 17:19 ` Andy Shevchenko 0 siblings, 1 reply; 11+ messages in thread From: Leon Romanovsky @ 2023-07-11 17:10 UTC (permalink / raw) To: Andy Shevchenko Cc: Paolo Abeni, Jakub Kicinski, netdev, linux-kernel, David S. Miller, Eric Dumazet On Tue, Jul 11, 2023 at 04:44:18PM +0300, Andy Shevchenko wrote: > On Tue, Jul 11, 2023 at 04:32:59PM +0300, Leon Romanovsky wrote: > > On Tue, Jul 11, 2023 at 03:45:34PM +0300, Andy Shevchenko wrote: > > > On Tue, Jul 11, 2023 at 03:20:12PM +0300, Leon Romanovsky wrote: > > > > On Tue, Jul 11, 2023 at 01:54:18PM +0300, Andy Shevchenko wrote: > > > > > On Tue, Jul 11, 2023 at 12:21:12PM +0200, Paolo Abeni wrote: > > > > > > On Tue, 2023-07-11 at 09:33 +0300, Leon Romanovsky wrote: > > > > > > > On Mon, Jul 10, 2023 at 01:06:24PM +0300, Andy Shevchenko wrote: > > ... > > > > > > > > So what is the outcome of "int - bool + bool" in the line above? > > > > > > > > > > The same as with int - int [0 .. 1] + int [0 .. 1]. > > > > > > > > No, it is not. bool is defined as _Bool C99 type, so strictly speaking > > > > you are mixing types int - _Bool + _Bool. > > > > > > 1. The original code already does that. You still haven't reacted on that. > > > > The original code was int - int + int. > > No. You missed the callers part. They are using boolean. I didn't miss and pointed you to the exact line which was implicitly changed with your patch. > > > > 2. Is what you are telling a problema? > > > > No, I'm saying that you took perfectly correct code which had all types > > aligned and changed it to have mixed type arithmetic. > > And after this change it's perfectly correct code with less letters and hidden > promotions (as a parameter to the function) and hence requires less cognitive > energy to parse. > > So, the bottom line is the commit message you don't like, is it so? Please reread my and Paolo replies. Thanks > > -- > With Best Regards, > Andy Shevchenko > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net-next][resend v1 1/1] netlink: Don't use int as bool in netlink_update_socket_mc() 2023-07-11 17:10 ` Leon Romanovsky @ 2023-07-11 17:19 ` Andy Shevchenko 0 siblings, 0 replies; 11+ messages in thread From: Andy Shevchenko @ 2023-07-11 17:19 UTC (permalink / raw) To: Leon Romanovsky Cc: Paolo Abeni, Jakub Kicinski, netdev, linux-kernel, David S. Miller, Eric Dumazet On Tue, Jul 11, 2023 at 08:10:58PM +0300, Leon Romanovsky wrote: > On Tue, Jul 11, 2023 at 04:44:18PM +0300, Andy Shevchenko wrote: > > On Tue, Jul 11, 2023 at 04:32:59PM +0300, Leon Romanovsky wrote: > > > On Tue, Jul 11, 2023 at 03:45:34PM +0300, Andy Shevchenko wrote: > > > > On Tue, Jul 11, 2023 at 03:20:12PM +0300, Leon Romanovsky wrote: > > > > > On Tue, Jul 11, 2023 at 01:54:18PM +0300, Andy Shevchenko wrote: > > > > > > On Tue, Jul 11, 2023 at 12:21:12PM +0200, Paolo Abeni wrote: > > > > > > > On Tue, 2023-07-11 at 09:33 +0300, Leon Romanovsky wrote: > > > > > > > > On Mon, Jul 10, 2023 at 01:06:24PM +0300, Andy Shevchenko wrote: ... > > > > > > > > So what is the outcome of "int - bool + bool" in the line above? > > > > > > > > > > > > The same as with int - int [0 .. 1] + int [0 .. 1]. > > > > > > > > > > No, it is not. bool is defined as _Bool C99 type, so strictly speaking > > > > > you are mixing types int - _Bool + _Bool. > > > > > > > > 1. The original code already does that. You still haven't reacted on that. > > > > > > The original code was int - int + int. > > > > No. You missed the callers part. They are using boolean. > > I didn't miss and pointed you to the exact line which was implicitly > changed with your patch. Yes, and this line doesn't change the status quo. We have boolean in the callers that implicitly went to the callee as int. > > > > 2. Is what you are telling a problema? > > > > > > No, I'm saying that you took perfectly correct code which had all types > > > aligned and changed it to have mixed type arithmetic. > > > > And after this change it's perfectly correct code with less letters and hidden > > promotions (as a parameter to the function) and hence requires less cognitive > > energy to parse. > > > > So, the bottom line is the commit message you don't like, is it so? > > Please reread my and Paolo replies. I have read them. My point is that you should also look at the callers to see the big picture. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net-next][resend v1 1/1] netlink: Don't use int as bool in netlink_update_socket_mc() 2023-07-10 10:06 [PATCH net-next][resend v1 1/1] netlink: Don't use int as bool in netlink_update_socket_mc() Andy Shevchenko 2023-07-11 6:33 ` Leon Romanovsky @ 2023-07-12 0:43 ` Jakub Kicinski 1 sibling, 0 replies; 11+ messages in thread From: Jakub Kicinski @ 2023-07-12 0:43 UTC (permalink / raw) To: Andy Shevchenko Cc: netdev, linux-kernel, David S. Miller, Eric Dumazet, Paolo Abeni On Mon, 10 Jul 2023 13:06:24 +0300 Andy Shevchenko wrote: > The bit operations take boolean parameter and return also boolean > (in test_bit()-like cases). Don't threat booleans as integers when > it's not needed. I don't have a strong opinion on the merit. But I feel like the discussion is a waste of everyone's time, so to discourage such ambivalent patches I'm going to drop this. -- pw-bot: reject ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2023-07-12 0:43 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-07-10 10:06 [PATCH net-next][resend v1 1/1] netlink: Don't use int as bool in netlink_update_socket_mc() Andy Shevchenko 2023-07-11 6:33 ` Leon Romanovsky 2023-07-11 10:21 ` Paolo Abeni 2023-07-11 10:54 ` Andy Shevchenko 2023-07-11 12:20 ` Leon Romanovsky 2023-07-11 12:45 ` Andy Shevchenko 2023-07-11 13:32 ` Leon Romanovsky 2023-07-11 13:44 ` Andy Shevchenko 2023-07-11 17:10 ` Leon Romanovsky 2023-07-11 17:19 ` Andy Shevchenko 2023-07-12 0:43 ` Jakub Kicinski
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).