diff mbox series

[net-next,resend,v1,1/1] netlink: Don't use int as bool in netlink_update_socket_mc()

Message ID 20230710100624.87836-1-andriy.shevchenko@linux.intel.com
State New
Headers
Series [net-next,resend,v1,1/1] netlink: Don't use int as bool in netlink_update_socket_mc() |

Commit Message

Andy Shevchenko July 10, 2023, 10:06 a.m. UTC 
  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(-)

Comments

Leon Romanovsky July 11, 2023, 6:33 a.m. UTC | #1 
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
> 
>
Paolo Abeni July 11, 2023, 10:21 a.m. UTC | #2 
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
Andy Shevchenko July 11, 2023, 10:54 a.m. UTC | #3 
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.
Leon Romanovsky July 11, 2023, 12:20 p.m. UTC | #4 
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
> 
>
Andy Shevchenko July 11, 2023, 12:45 p.m. UTC | #5 
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?
Leon Romanovsky July 11, 2023, 1:32 p.m. UTC | #6 
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
> 
>
Andy Shevchenko July 11, 2023, 1:44 p.m. UTC | #7 
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?
Leon Romanovsky July 11, 2023, 5:10 p.m. UTC | #8 
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
> 
>
Andy Shevchenko July 11, 2023, 5:19 p.m. UTC | #9 
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.
Jakub Kicinski July 12, 2023, 12:43 a.m. UTC | #10 
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.
diff mbox series

Patch

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 *