[PATCH net-next 0/8] Some modifications to optimize code readability

[PATCH net-next 0/8] Some modifications to optimize code readability

[Li Zetao]

This patchset is mainly optimized for readability in contexts where size
needs to be determined. By using min() or max(), or even directly
removing redundant judgments (such as the 5th patch), the code is more
consistent with the context.

Li Zetao (8):
  atm: use min() to simplify the code
  Bluetooth: use min() to simplify the code
  net: caif: use max() to simplify the code
  libceph: use min() to simplify the code
  net: remove redundant judgments to simplify the code
  ipv6: mcast: use min() to simplify the code
  tipc: use min() to simplify the code
  SUNRPC: use min() to simplify the code

 net/atm/addr.c            | 4 ++--
 net/bluetooth/hidp/core.c | 2 +-
 net/caif/cfpkt_skbuff.c   | 6 ++----
 net/ceph/messenger.c      | 2 +-
 net/core/sock.c           | 2 +-
 net/ipv6/mcast.c          | 5 +++--
 net/sunrpc/xdr.c          | 2 +-
 net/tipc/monitor.c        | 2 +-
 8 files changed, 12 insertions(+), 13 deletions(-)

-- 
2.34.1

[PATCH net-next 1/8] atm: use min() to simplify the code

[Li Zetao]

When copying data to user, it needs to determine the copy length.
It is easier to understand using min() here.

Signed-off-by: Li Zetao <lizetao1@huawei.com>
---
 net/atm/addr.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/atm/addr.c b/net/atm/addr.c
index 0530b63f509a..6c4c942b2cb9 100644
--- a/net/atm/addr.c
+++ b/net/atm/addr.c
@@ -136,7 +136,7 @@ int atm_get_addr(struct atm_dev *dev, struct sockaddr_atmsvc __user * buf,
 	unsigned long flags;
 	struct atm_dev_addr *this;
 	struct list_head *head;
-	int total = 0, error;
+	size_t total = 0, error;
 	struct sockaddr_atmsvc *tmp_buf, *tmp_bufp;
 
 	spin_lock_irqsave(&dev->lock, flags);
@@ -155,7 +155,7 @@ int atm_get_addr(struct atm_dev *dev, struct sockaddr_atmsvc __user * buf,
 	    memcpy(tmp_bufp++, &this->addr, sizeof(struct sockaddr_atmsvc));
 	spin_unlock_irqrestore(&dev->lock, flags);
 	error = total > size ? -E2BIG : total;
-	if (copy_to_user(buf, tmp_buf, total < size ? total : size))
+	if (copy_to_user(buf, tmp_buf, min(total, size)))
 		error = -EFAULT;
 	kfree(tmp_buf);
 	return error;
-- 
2.34.1

Re: [PATCH net-next 1/8] atm: use min() to simplify the code

[Dr. David Alan Gilbert]

* Li Zetao (lizetao1@huawei.com) wrote:
> When copying data to user, it needs to determine the copy length.
> It is easier to understand using min() here.
> 
> Signed-off-by: Li Zetao <lizetao1@huawei.com>
> ---
>  net/atm/addr.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/net/atm/addr.c b/net/atm/addr.c
> index 0530b63f509a..6c4c942b2cb9 100644
> --- a/net/atm/addr.c
> +++ b/net/atm/addr.c
> @@ -136,7 +136,7 @@ int atm_get_addr(struct atm_dev *dev, struct sockaddr_atmsvc __user * buf,
>  	unsigned long flags;
>  	struct atm_dev_addr *this;
>  	struct list_head *head;
> -	int total = 0, error;
> +	size_t total = 0, error;

Aren't you accidentally changing the type of 'error' there, and the function
returns 'int'.

Dave


>  	struct sockaddr_atmsvc *tmp_buf, *tmp_bufp;
>  
>  	spin_lock_irqsave(&dev->lock, flags);
> @@ -155,7 +155,7 @@ int atm_get_addr(struct atm_dev *dev, struct sockaddr_atmsvc __user * buf,
>  	    memcpy(tmp_bufp++, &this->addr, sizeof(struct sockaddr_atmsvc));
>  	spin_unlock_irqrestore(&dev->lock, flags);
>  	error = total > size ? -E2BIG : total;
> -	if (copy_to_user(buf, tmp_buf, total < size ? total : size))
> +	if (copy_to_user(buf, tmp_buf, min(total, size)))
>  		error = -EFAULT;
>  	kfree(tmp_buf);
>  	return error;
> -- 
> 2.34.1
> 
-- 
 -----Open up your eyes, open up your mind, open up your code -------   
/ Dr. David Alan Gilbert    |       Running GNU/Linux       | Happy  \ 
\        dave @ treblig.org |                               | In Hex /
 \ _________________________|_____ http://www.treblig.org   |_______/

Re: [PATCH net-next 1/8] atm: use min() to simplify the code

[Li Zetao]

Hi,

在 2024/8/22 21:39, Dr. David Alan Gilbert 写道:
> * Li Zetao (lizetao1@huawei.com) wrote:
>> When copying data to user, it needs to determine the copy length.
>> It is easier to understand using min() here.
>>
>> Signed-off-by: Li Zetao <lizetao1@huawei.com>
>> ---
>>   net/atm/addr.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/atm/addr.c b/net/atm/addr.c
>> index 0530b63f509a..6c4c942b2cb9 100644
>> --- a/net/atm/addr.c
>> +++ b/net/atm/addr.c
>> @@ -136,7 +136,7 @@ int atm_get_addr(struct atm_dev *dev, struct sockaddr_atmsvc __user * buf,
>>   	unsigned long flags;
>>   	struct atm_dev_addr *this;
>>   	struct list_head *head;
>> -	int total = 0, error;
>> +	size_t total = 0, error;
> 
> Aren't you accidentally changing the type of 'error' there, and the function
> returns 'int'.
This is intentionally modified because the input parameter size is of 
type size_t. If total is of type int, the compiler will report an error 
when the min() is called.
> 
> Dav
> 
> 
>>   	struct sockaddr_atmsvc *tmp_buf, *tmp_bufp;
>>   
>>   	spin_lock_irqsave(&dev->lock, flags);
>> @@ -155,7 +155,7 @@ int atm_get_addr(struct atm_dev *dev, struct sockaddr_atmsvc __user * buf,
>>   	    memcpy(tmp_bufp++, &this->addr, sizeof(struct sockaddr_atmsvc));
>>   	spin_unlock_irqrestore(&dev->lock, flags);
>>   	error = total > size ? -E2BIG : total;
>> -	if (copy_to_user(buf, tmp_buf, total < size ? total : size))
>> +	if (copy_to_user(buf, tmp_buf, min(total, size)))
>>   		error = -EFAULT;
>>   	kfree(tmp_buf);
>>   	return error;
>> -- 
>> 2.34.1
>>

Thanks,
Li Zetao.

Re: [PATCH net-next 1/8] atm: use min() to simplify the code

[Jacob Keller]

On 8/22/2024 6:50 AM, Li Zetao wrote:
> Hi,
> 
> 在 2024/8/22 21:39, Dr. David Alan Gilbert 写道:
>> * Li Zetao (lizetao1@huawei.com) wrote:
>>> When copying data to user, it needs to determine the copy length.
>>> It is easier to understand using min() here.
>>>
>>> Signed-off-by: Li Zetao <lizetao1@huawei.com>
>>> ---
>>>   net/atm/addr.c | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/net/atm/addr.c b/net/atm/addr.c
>>> index 0530b63f509a..6c4c942b2cb9 100644
>>> --- a/net/atm/addr.c
>>> +++ b/net/atm/addr.c
>>> @@ -136,7 +136,7 @@ int atm_get_addr(struct atm_dev *dev, struct sockaddr_atmsvc __user * buf,
>>>   	unsigned long flags;
>>>   	struct atm_dev_addr *this;
>>>   	struct list_head *head;
>>> -	int total = 0, error;
>>> +	size_t total = 0, error;
>>
>> Aren't you accidentally changing the type of 'error' there, and the function
>> returns 'int'.
> This is intentionally modified because the input parameter size is of 
> type size_t. If total is of type int, the compiler will report an error 
> when the min() is called.
>>

Yea, but what you're missing is that error was an int before and is now
a size_t which can't be negative.

I think this either needs to be:

size_t total = 0;
int error

or better yet....

>> Dav
>>
>>
>>>   	struct sockaddr_atmsvc *tmp_buf, *tmp_bufp;
>>>   
>>>   	spin_lock_irqsave(&dev->lock, flags);
>>> @@ -155,7 +155,7 @@ int atm_get_addr(struct atm_dev *dev, struct sockaddr_atmsvc __user * buf,
>>>   	    memcpy(tmp_bufp++, &this->addr, sizeof(struct sockaddr_atmsvc));
>>>   	spin_unlock_irqrestore(&dev->lock, flags);
>>>   	error = total > size ? -E2BIG : total;
>>> -	if (copy_to_user(buf, tmp_buf, total < size ? total : size))
>>> +	if (copy_to_user(buf, tmp_buf, min(total, size)))
>>>   		error = -EFAULT;


Couldn't you just use min_t here instead of changing the variable sizes?

Thanks,
Jake

[PATCH net-next 2/8] Bluetooth: use min() to simplify the code

[Li Zetao]

When copying data from skb, it needs to determine the copy length.
It is easier to understand using min() here.

Signed-off-by: Li Zetao <lizetao1@huawei.com>
---
 net/bluetooth/hidp/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index 707f229f896a..7bf24f2993ba 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -294,7 +294,7 @@ static int hidp_get_raw_report(struct hid_device *hid,
 
 	skb = session->report_return;
 	if (skb) {
-		len = skb->len < count ? skb->len : count;
+		len = min(skb->len, count);
 		memcpy(data, skb->data, len);
 
 		kfree_skb(skb);
-- 
2.34.1

Re: [PATCH net-next 2/8] Bluetooth: use min() to simplify the code

[Simon Horman]

On Thu, Aug 22, 2024 at 09:39:02PM +0800, Li Zetao wrote:
> When copying data from skb, it needs to determine the copy length.
> It is easier to understand using min() here.
> 
> Signed-off-by: Li Zetao <lizetao1@huawei.com>

However, I don't believe Bluetooth changes usually don't go through next-next.
So I think this either needs to be reposted or get an ack from
the maintainer (already CCed).

Luiz, perhaps you can offer some guidance here?

> ---
>  net/bluetooth/hidp/core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
> index 707f229f896a..7bf24f2993ba 100644
> --- a/net/bluetooth/hidp/core.c
> +++ b/net/bluetooth/hidp/core.c
> @@ -294,7 +294,7 @@ static int hidp_get_raw_report(struct hid_device *hid,
>  
>  	skb = session->report_return;
>  	if (skb) {
> -		len = skb->len < count ? skb->len : count;
> +		len = min(skb->len, count);

I am slightly dubious about this check, given the different types involved.
Is using min_t appropriate (I don't know)?

>  		memcpy(data, skb->data, len);
>  
>  		kfree_skb(skb);
> -- 
> 2.34.1
> 
> 

[PATCH net-next 3/8] net: caif: use max() to simplify the code

[Li Zetao]

When processing the tail append of sk buffer, the final length needs
to be determined based on expectlen and addlen. Using max() here can
increase the readability of the code.

Signed-off-by: Li Zetao <lizetao1@huawei.com>
---
 net/caif/cfpkt_skbuff.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/net/caif/cfpkt_skbuff.c b/net/caif/cfpkt_skbuff.c
index 2ae8cfa3df88..96236d21b18e 100644
--- a/net/caif/cfpkt_skbuff.c
+++ b/net/caif/cfpkt_skbuff.c
@@ -298,10 +298,8 @@ struct cfpkt *cfpkt_append(struct cfpkt *dstpkt,
 	if (unlikely(is_erronous(dstpkt) || is_erronous(addpkt))) {
 		return dstpkt;
 	}
-	if (expectlen > addlen)
-		neededtailspace = expectlen;
-	else
-		neededtailspace = addlen;
+
+	neededtailspace = max(expectlen, addlen);
 
 	if (dst->tail + neededtailspace > dst->end) {
 		/* Create a dumplicate of 'dst' with more tail space */
-- 
2.34.1

Re: [PATCH net-next 3/8] net: caif: use max() to simplify the code

[Simon Horman]

On Thu, Aug 22, 2024 at 09:39:03PM +0800, Li Zetao wrote:
> When processing the tail append of sk buffer, the final length needs
> to be determined based on expectlen and addlen. Using max() here can
> increase the readability of the code.
> 
> Signed-off-by: Li Zetao <lizetao1@huawei.com>
> ---
>  net/caif/cfpkt_skbuff.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/net/caif/cfpkt_skbuff.c b/net/caif/cfpkt_skbuff.c
> index 2ae8cfa3df88..96236d21b18e 100644
> --- a/net/caif/cfpkt_skbuff.c
> +++ b/net/caif/cfpkt_skbuff.c
> @@ -298,10 +298,8 @@ struct cfpkt *cfpkt_append(struct cfpkt *dstpkt,
>  	if (unlikely(is_erronous(dstpkt) || is_erronous(addpkt))) {
>  		return dstpkt;
>  	}
> -	if (expectlen > addlen)
> -		neededtailspace = expectlen;
> -	else
> -		neededtailspace = addlen;
> +
> +	neededtailspace = max(expectlen, addlen);

The types of all three variables involved here are u16,
so this looks correct to me. And the code replaced
seems to be an open coding of max() both in intent and function.

Reviewed-by: Simon Horman <horms@kernel.org>

>  
>  	if (dst->tail + neededtailspace > dst->end) {
>  		/* Create a dumplicate of 'dst' with more tail space */

[PATCH net-next 4/8] libceph: use min() to simplify the code

[Li Zetao]

When resolving name in ceph_dns_resolve_name(), the end address of name
is determined by the minimum value of delim_p and colon_p. So using min()
here is more in line with the context.

Signed-off-by: Li Zetao <lizetao1@huawei.com>
---
 net/ceph/messenger.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c
index 3c8b78d9c4d1..d1b5705dc0c6 100644
--- a/net/ceph/messenger.c
+++ b/net/ceph/messenger.c
@@ -1254,7 +1254,7 @@ static int ceph_dns_resolve_name(const char *name, size_t namelen,
 	colon_p = memchr(name, ':', namelen);
 
 	if (delim_p && colon_p)
-		end = delim_p < colon_p ? delim_p : colon_p;
+		end = min(delim_p, colon_p);
 	else if (!delim_p && colon_p)
 		end = colon_p;
 	else {
-- 
2.34.1

Re: [PATCH net-next 4/8] libceph: use min() to simplify the code

[Simon Horman]

On Thu, Aug 22, 2024 at 09:39:04PM +0800, Li Zetao wrote:
> When resolving name in ceph_dns_resolve_name(), the end address of name
> is determined by the minimum value of delim_p and colon_p. So using min()
> here is more in line with the context.
> 
> Signed-off-by: Li Zetao <lizetao1@huawei.com>
> ---
>  net/ceph/messenger.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c
> index 3c8b78d9c4d1..d1b5705dc0c6 100644
> --- a/net/ceph/messenger.c
> +++ b/net/ceph/messenger.c
> @@ -1254,7 +1254,7 @@ static int ceph_dns_resolve_name(const char *name, size_t namelen,
>  	colon_p = memchr(name, ':', namelen);
>  
>  	if (delim_p && colon_p)
> -		end = delim_p < colon_p ? delim_p : colon_p;
> +		end = min(delim_p, colon_p);

Both delim_p, and colon_p are char *, so this seems correct to me.

And the code being replaced does appear to be a min() operation in
both form and function.

Reviewed-by: Simon Horman <horms@kernel.org>

However, I don't believe libceph changes usually don't go through next-next.
So I think this either needs to be reposted or get some acks from
one of the maintainers.

Ilya, Xiubo, perhaps you can offer some guidance here?

>  	else if (!delim_p && colon_p)
>  		end = colon_p;
>  	else {
> -- 
> 2.34.1
> 
> 

Re: [PATCH net-next 4/8] libceph: use min() to simplify the code

[Ilya Dryomov]

On Sat, Aug 24, 2024 at 8:12 PM Simon Horman <horms@kernel.org> wrote:
>
> On Thu, Aug 22, 2024 at 09:39:04PM +0800, Li Zetao wrote:
> > When resolving name in ceph_dns_resolve_name(), the end address of name
> > is determined by the minimum value of delim_p and colon_p. So using min()
> > here is more in line with the context.
> >
> > Signed-off-by: Li Zetao <lizetao1@huawei.com>
> > ---
> >  net/ceph/messenger.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c
> > index 3c8b78d9c4d1..d1b5705dc0c6 100644
> > --- a/net/ceph/messenger.c
> > +++ b/net/ceph/messenger.c
> > @@ -1254,7 +1254,7 @@ static int ceph_dns_resolve_name(const char *name, size_t namelen,
> >       colon_p = memchr(name, ':', namelen);
> >
> >       if (delim_p && colon_p)
> > -             end = delim_p < colon_p ? delim_p : colon_p;
> > +             end = min(delim_p, colon_p);
>
> Both delim_p, and colon_p are char *, so this seems correct to me.
>
> And the code being replaced does appear to be a min() operation in
> both form and function.
>
> Reviewed-by: Simon Horman <horms@kernel.org>
>
> However, I don't believe libceph changes usually don't go through next-next.
> So I think this either needs to be reposted or get some acks from
> one of the maintainers.
>
> Ilya, Xiubo, perhaps you can offer some guidance here?

Hi Simon,

I'm OK with this being taken through net-next.

Acked-by: Ilya Dryomov <idryomov@gmail.com>

Thanks,

                Ilya

Re: [PATCH net-next 4/8] libceph: use min() to simplify the code

[Ilya Dryomov]

On Sun, Aug 25, 2024 at 6:21 PM Ilya Dryomov <idryomov@gmail.com> wrote:
>
> On Sat, Aug 24, 2024 at 8:12 PM Simon Horman <horms@kernel.org> wrote:
> >
> > On Thu, Aug 22, 2024 at 09:39:04PM +0800, Li Zetao wrote:
> > > When resolving name in ceph_dns_resolve_name(), the end address of name
> > > is determined by the minimum value of delim_p and colon_p. So using min()
> > > here is more in line with the context.
> > >
> > > Signed-off-by: Li Zetao <lizetao1@huawei.com>
> > > ---
> > >  net/ceph/messenger.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c
> > > index 3c8b78d9c4d1..d1b5705dc0c6 100644
> > > --- a/net/ceph/messenger.c
> > > +++ b/net/ceph/messenger.c
> > > @@ -1254,7 +1254,7 @@ static int ceph_dns_resolve_name(const char *name, size_t namelen,
> > >       colon_p = memchr(name, ':', namelen);
> > >
> > >       if (delim_p && colon_p)
> > > -             end = delim_p < colon_p ? delim_p : colon_p;
> > > +             end = min(delim_p, colon_p);
> >
> > Both delim_p, and colon_p are char *, so this seems correct to me.
> >
> > And the code being replaced does appear to be a min() operation in
> > both form and function.
> >
> > Reviewed-by: Simon Horman <horms@kernel.org>
> >
> > However, I don't believe libceph changes usually don't go through next-next.
> > So I think this either needs to be reposted or get some acks from
> > one of the maintainers.
> >
> > Ilya, Xiubo, perhaps you can offer some guidance here?
>
> Hi Simon,
>
> I'm OK with this being taken through net-next.

I see that Jakub picked up some patches from this series, but not this
one.  I'll go ahead and apply to the Ceph tree.

Thanks,

                Ilya

[PATCH net-next 5/8] net: remove redundant judgments to simplify the code

[Li Zetao]

The res variable has been initialized to 0, and the number of prots
being used will not exceed MAX_INT, so there is no need to judge
whether it is greater than 0 before returning.

Refer to the implementation of sock_inuse_get.

Signed-off-by: Li Zetao <lizetao1@huawei.com>
---
 net/core/sock.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/sock.c b/net/core/sock.c
index bbe4c58470c3..52bfc86a2f37 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -3800,7 +3800,7 @@ int sock_prot_inuse_get(struct net *net, struct proto *prot)
 	for_each_possible_cpu(cpu)
 		res += per_cpu_ptr(net->core.prot_inuse, cpu)->val[idx];
 
-	return res >= 0 ? res : 0;
+	return res;
 }
 EXPORT_SYMBOL_GPL(sock_prot_inuse_get);
 
-- 
2.34.1

Re: [PATCH net-next 5/8] net: remove redundant judgments to simplify the code

[Simon Horman]

On Thu, Aug 22, 2024 at 09:39:05PM +0800, Li Zetao wrote:
> The res variable has been initialized to 0, and the number of prots
> being used will not exceed MAX_INT, so there is no need to judge
> whether it is greater than 0 before returning.
> 
> Refer to the implementation of sock_inuse_get.
> 
> Signed-off-by: Li Zetao <lizetao1@huawei.com>
> ---
>  net/core/sock.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/core/sock.c b/net/core/sock.c
> index bbe4c58470c3..52bfc86a2f37 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -3800,7 +3800,7 @@ int sock_prot_inuse_get(struct net *net, struct proto *prot)
>  	for_each_possible_cpu(cpu)
>  		res += per_cpu_ptr(net->core.prot_inuse, cpu)->val[idx];

Are you really sure that val[idx] can never be negative?

>  
> -	return res >= 0 ? res : 0;
> +	return res;
>  }
>  EXPORT_SYMBOL_GPL(sock_prot_inuse_get);
>  
> -- 
> 2.34.1
> 
> 

[PATCH net-next 6/8] ipv6: mcast: use min() to simplify the code

[Li Zetao]

When coping sockaddr in ip6_mc_msfget(), the time of copies
depends on the minimum value between sl_count and gf_numsrc.
Using min() here is very semantic.

Signed-off-by: Li Zetao <lizetao1@huawei.com>
---
 net/ipv6/mcast.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/ipv6/mcast.c b/net/ipv6/mcast.c
index 7ba01d8cfbae..b244dbf61d5f 100644
--- a/net/ipv6/mcast.c
+++ b/net/ipv6/mcast.c
@@ -586,7 +586,8 @@ int ip6_mc_msfget(struct sock *sk, struct group_filter *gsf,
 	const struct in6_addr *group;
 	struct ipv6_mc_socklist *pmc;
 	struct ip6_sf_socklist *psl;
-	int i, count, copycount;
+	unsigned int count;
+	int i, copycount;
 
 	group = &((struct sockaddr_in6 *)&gsf->gf_group)->sin6_addr;
 
@@ -610,7 +611,7 @@ int ip6_mc_msfget(struct sock *sk, struct group_filter *gsf,
 	psl = sock_dereference(pmc->sflist, sk);
 	count = psl ? psl->sl_count : 0;
 
-	copycount = count < gsf->gf_numsrc ? count : gsf->gf_numsrc;
+	copycount = min(count, gsf->gf_numsrc);
 	gsf->gf_numsrc = count;
 	for (i = 0; i < copycount; i++) {
 		struct sockaddr_in6 *psin6;
-- 
2.34.1

Re: [PATCH net-next 6/8] ipv6: mcast: use min() to simplify the code

[Simon Horman]

On Thu, Aug 22, 2024 at 09:39:06PM +0800, Li Zetao wrote:
> When coping sockaddr in ip6_mc_msfget(), the time of copies
> depends on the minimum value between sl_count and gf_numsrc.
> Using min() here is very semantic.
> 
> Signed-off-by: Li Zetao <lizetao1@huawei.com>
> ---
>  net/ipv6/mcast.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/net/ipv6/mcast.c b/net/ipv6/mcast.c
> index 7ba01d8cfbae..b244dbf61d5f 100644
> --- a/net/ipv6/mcast.c
> +++ b/net/ipv6/mcast.c
> @@ -586,7 +586,8 @@ int ip6_mc_msfget(struct sock *sk, struct group_filter *gsf,
>  	const struct in6_addr *group;
>  	struct ipv6_mc_socklist *pmc;
>  	struct ip6_sf_socklist *psl;
> -	int i, count, copycount;
> +	unsigned int count;
> +	int i, copycount;
>  
>  	group = &((struct sockaddr_in6 *)&gsf->gf_group)->sin6_addr;
>  
> @@ -610,7 +611,7 @@ int ip6_mc_msfget(struct sock *sk, struct group_filter *gsf,
>  	psl = sock_dereference(pmc->sflist, sk);
>  	count = psl ? psl->sl_count : 0;

Both count and psl->sl_count are unsigned int,
so this looks safe (and more correct than what it replaces, IMHO).

>  
> -	copycount = count < gsf->gf_numsrc ? count : gsf->gf_numsrc;
> +	copycount = min(count, gsf->gf_numsrc);

And gsf->gf_numsrc is a __u32, so min operating on it and
count looks safe to me.

Further, the code it replaces seems to be a max() operation in
both intent and function.

Reviewed-by: Simon Horman <horms@kernel.org>

>  	gsf->gf_numsrc = count;
>  	for (i = 0; i < copycount; i++) {
>  		struct sockaddr_in6 *psin6;
> -- 
> 2.34.1
> 
> 

[PATCH net-next 7/8] tipc: use min() to simplify the code

[Li Zetao]

When calculating size of own domain based on number of peers, the result
should be less than MAX_MON_DOMAIN, so using min() here is very semantic.

Signed-off-by: Li Zetao <lizetao1@huawei.com>
---
 net/tipc/monitor.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/tipc/monitor.c b/net/tipc/monitor.c
index 77a3d016cade..e2f19627e43d 100644
--- a/net/tipc/monitor.c
+++ b/net/tipc/monitor.c
@@ -149,7 +149,7 @@ static int dom_size(int peers)
 
 	while ((i * i) < peers)
 		i++;
-	return i < MAX_MON_DOMAIN ? i : MAX_MON_DOMAIN;
+	return min(i, MAX_MON_DOMAIN);
 }
 
 static void map_set(u64 *up_map, int i, unsigned int v)
-- 
2.34.1

Re: [PATCH net-next 7/8] tipc: use min() to simplify the code

[Simon Horman]

On Thu, Aug 22, 2024 at 09:39:07PM +0800, Li Zetao wrote:
> When calculating size of own domain based on number of peers, the result
> should be less than MAX_MON_DOMAIN, so using min() here is very semantic.
> 
> Signed-off-by: Li Zetao <lizetao1@huawei.com>
> ---
>  net/tipc/monitor.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/tipc/monitor.c b/net/tipc/monitor.c
> index 77a3d016cade..e2f19627e43d 100644
> --- a/net/tipc/monitor.c
> +++ b/net/tipc/monitor.c
> @@ -149,7 +149,7 @@ static int dom_size(int peers)
>  
>  	while ((i * i) < peers)
>  		i++;
> -	return i < MAX_MON_DOMAIN ? i : MAX_MON_DOMAIN;
> +	return min(i, MAX_MON_DOMAIN);
>  }

Perhaps this whole function is open coding something, but
if so I couldn't find it.

In any case this looks safe to me as i is an unsigned int
while MAX_MON_DOMAIN is 64 (also an unsigned int, I believe).

And the code being replaced appears to be a min() operation
in both form and function.

Reviewed-by: Simon Horman <horms@kernel.org>

>  
>  static void map_set(u64 *up_map, int i, unsigned int v)
> -- 
> 2.34.1
> 
> 

[PATCH net-next 8/8] SUNRPC: use min() to simplify the code

[Li Zetao]

When reading pages in xdr_read_pages, the number of XDR encoded bytes
should be less than the len of aligned pages, so using min() here is
very semantic.

Signed-off-by: Li Zetao <lizetao1@huawei.com>
---
 net/sunrpc/xdr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
index 62e07c330a66..6746e920dbbb 100644
--- a/net/sunrpc/xdr.c
+++ b/net/sunrpc/xdr.c
@@ -1602,7 +1602,7 @@ unsigned int xdr_read_pages(struct xdr_stream *xdr, unsigned int len)
 	end = xdr_stream_remaining(xdr) - pglen;
 
 	xdr_set_tail_base(xdr, base, end);
-	return len <= pglen ? len : pglen;
+	return min(len, pglen);
 }
 EXPORT_SYMBOL_GPL(xdr_read_pages);
 
-- 
2.34.1

Re: [PATCH net-next 8/8] SUNRPC: use min() to simplify the code

[Simon Horman]

On Thu, Aug 22, 2024 at 09:39:08PM +0800, Li Zetao wrote:
> When reading pages in xdr_read_pages, the number of XDR encoded bytes
> should be less than the len of aligned pages, so using min() here is
> very semantic.
> 
> Signed-off-by: Li Zetao <lizetao1@huawei.com>
> ---
>  net/sunrpc/xdr.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
> index 62e07c330a66..6746e920dbbb 100644
> --- a/net/sunrpc/xdr.c
> +++ b/net/sunrpc/xdr.c
> @@ -1602,7 +1602,7 @@ unsigned int xdr_read_pages(struct xdr_stream *xdr, unsigned int len)
>  	end = xdr_stream_remaining(xdr) - pglen;
>  
>  	xdr_set_tail_base(xdr, base, end);
> -	return len <= pglen ? len : pglen;
> +	return min(len, pglen);

Both len and pglen are unsigned int, so this seems correct to me.

And the code being replaced does appear to be a min() operation in
both form and function.

Reviewed-by: Simon Horman <horms@kernel.org>

However, I don't believe SUNRPC changes usually don't go through next-next.
So I think this either needs to be reposted or get some acks from
Chuck Lever (already CCed).

Chuck, perhaps you can offer some guidance here?

>  }
>  EXPORT_SYMBOL_GPL(xdr_read_pages);
>  
> -- 
> 2.34.1
> 
> 

Re: [PATCH net-next 8/8] SUNRPC: use min() to simplify the code

[Chuck Lever]

On Sat, Aug 24, 2024 at 07:07:50PM +0100, Simon Horman wrote:
> On Thu, Aug 22, 2024 at 09:39:08PM +0800, Li Zetao wrote:
> > When reading pages in xdr_read_pages, the number of XDR encoded bytes
> > should be less than the len of aligned pages, so using min() here is
> > very semantic.
> > 
> > Signed-off-by: Li Zetao <lizetao1@huawei.com>
> > ---
> >  net/sunrpc/xdr.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
> > index 62e07c330a66..6746e920dbbb 100644
> > --- a/net/sunrpc/xdr.c
> > +++ b/net/sunrpc/xdr.c
> > @@ -1602,7 +1602,7 @@ unsigned int xdr_read_pages(struct xdr_stream *xdr, unsigned int len)
> >  	end = xdr_stream_remaining(xdr) - pglen;
> >  
> >  	xdr_set_tail_base(xdr, base, end);
> > -	return len <= pglen ? len : pglen;
> > +	return min(len, pglen);
> 
> Both len and pglen are unsigned int, so this seems correct to me.
> 
> And the code being replaced does appear to be a min() operation in
> both form and function.
> 
> Reviewed-by: Simon Horman <horms@kernel.org>
> 
> However, I don't believe SUNRPC changes usually don't go through next-next.
> So I think this either needs to be reposted or get some acks from
> Chuck Lever (already CCed).
> 
> Chuck, perhaps you can offer some guidance here?
> 
> >  }
> >  EXPORT_SYMBOL_GPL(xdr_read_pages);
> >  
> > -- 
> > 2.34.1
> > 
> > 

Changes to net/sunrpc/ can go through Anna and Trond's NFS client
trees, through the NFSD tree, or via netdev, but they are typically
taken through the NFS-related trees.

Unless the submitter or the relevant maintainers prefer otherwise,
I don't see a problem with this one going through netdev. Let me
know otherwise.

Acked-by: Chuck Lever <chuck.lever@oracle.com>


-- 
Chuck Lever

Re: [PATCH net-next 8/8] SUNRPC: use min() to simplify the code

[Trond Myklebust]

On Sat, 2024-08-24 at 14:21 -0400, Chuck Lever wrote:
> On Sat, Aug 24, 2024 at 07:07:50PM +0100, Simon Horman wrote:
> > On Thu, Aug 22, 2024 at 09:39:08PM +0800, Li Zetao wrote:
> > > When reading pages in xdr_read_pages, the number of XDR encoded
> > > bytes
> > > should be less than the len of aligned pages, so using min() here
> > > is
> > > very semantic.
> > > 
> > > Signed-off-by: Li Zetao <lizetao1@huawei.com>
> > > ---
> > >  net/sunrpc/xdr.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
> > > index 62e07c330a66..6746e920dbbb 100644
> > > --- a/net/sunrpc/xdr.c
> > > +++ b/net/sunrpc/xdr.c
> > > @@ -1602,7 +1602,7 @@ unsigned int xdr_read_pages(struct
> > > xdr_stream *xdr, unsigned int len)
> > >  	end = xdr_stream_remaining(xdr) - pglen;
> > >  
> > >  	xdr_set_tail_base(xdr, base, end);
> > > -	return len <= pglen ? len : pglen;
> > > +	return min(len, pglen);
> > 
> > Both len and pglen are unsigned int, so this seems correct to me.
> > 
> > And the code being replaced does appear to be a min() operation in
> > both form and function.
> > 
> > Reviewed-by: Simon Horman <horms@kernel.org>
> > 
> > However, I don't believe SUNRPC changes usually don't go through
> > next-next.
> > So I think this either needs to be reposted or get some acks from
> > Chuck Lever (already CCed).
> > 
> > Chuck, perhaps you can offer some guidance here?
> > 
> > >  }
> > >  EXPORT_SYMBOL_GPL(xdr_read_pages);
> > >  
> > > -- 
> > > 2.34.1
> > > 
> > > 
> 
> Changes to net/sunrpc/ can go through Anna and Trond's NFS client
> trees, through the NFSD tree, or via netdev, but they are typically
> taken through the NFS-related trees.
> 
> Unless the submitter or the relevant maintainers prefer otherwise,
> I don't see a problem with this one going through netdev. Let me
> know otherwise.
> 
> Acked-by: Chuck Lever <chuck.lever@oracle.com>
> 
> 

What is the value of this change? Unless the current code is actually
broken or somehow difficult to read, then I much prefer to leave it
untouched so that any future back ports of fixes to code around that
line remain trivial.

So NACK to this change for now.

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

Re: [PATCH net-next 0/8] Some modifications to optimize code readability

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Thu, 22 Aug 2024 21:39:00 +0800 you wrote:
> This patchset is mainly optimized for readability in contexts where size
> needs to be determined. By using min() or max(), or even directly
> removing redundant judgments (such as the 5th patch), the code is more
> consistent with the context.
> 
> Li Zetao (8):
>   atm: use min() to simplify the code
>   Bluetooth: use min() to simplify the code
>   net: caif: use max() to simplify the code
>   libceph: use min() to simplify the code
>   net: remove redundant judgments to simplify the code
>   ipv6: mcast: use min() to simplify the code
>   tipc: use min() to simplify the code
>   SUNRPC: use min() to simplify the code
> 
> [...]

Here is the summary with links:
  - [net-next,1/8] atm: use min() to simplify the code
    (no matching commit)
  - [net-next,2/8] Bluetooth: use min() to simplify the code
    (no matching commit)
  - [net-next,3/8] net: caif: use max() to simplify the code
    https://git.kernel.org/netdev/net-next/c/b4985aa8e312
  - [net-next,4/8] libceph: use min() to simplify the code
    (no matching commit)
  - [net-next,5/8] net: remove redundant judgments to simplify the code
    (no matching commit)
  - [net-next,6/8] ipv6: mcast: use min() to simplify the code
    https://git.kernel.org/netdev/net-next/c/26549dab8a46
  - [net-next,7/8] tipc: use min() to simplify the code
    https://git.kernel.org/netdev/net-next/c/a18308623ce3
  - [net-next,8/8] SUNRPC: use min() to simplify the code
    (no matching commit)

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net-next 0/8] Some modifications to optimize code readability

[patchwork-bot+bluetooth]

Hello:

This series was applied to bluetooth/bluetooth-next.git (master)
by Jakub Kicinski <kuba@kernel.org>:

On Thu, 22 Aug 2024 21:39:00 +0800 you wrote:
> This patchset is mainly optimized for readability in contexts where size
> needs to be determined. By using min() or max(), or even directly
> removing redundant judgments (such as the 5th patch), the code is more
> consistent with the context.
> 
> Li Zetao (8):
>   atm: use min() to simplify the code
>   Bluetooth: use min() to simplify the code
>   net: caif: use max() to simplify the code
>   libceph: use min() to simplify the code
>   net: remove redundant judgments to simplify the code
>   ipv6: mcast: use min() to simplify the code
>   tipc: use min() to simplify the code
>   SUNRPC: use min() to simplify the code
> 
> [...]

Here is the summary with links:
  - [net-next,1/8] atm: use min() to simplify the code
    (no matching commit)
  - [net-next,2/8] Bluetooth: use min() to simplify the code
    (no matching commit)
  - [net-next,3/8] net: caif: use max() to simplify the code
    https://git.kernel.org/bluetooth/bluetooth-next/c/b4985aa8e312
  - [net-next,4/8] libceph: use min() to simplify the code
    (no matching commit)
  - [net-next,5/8] net: remove redundant judgments to simplify the code
    (no matching commit)
  - [net-next,6/8] ipv6: mcast: use min() to simplify the code
    https://git.kernel.org/bluetooth/bluetooth-next/c/26549dab8a46
  - [net-next,7/8] tipc: use min() to simplify the code
    https://git.kernel.org/bluetooth/bluetooth-next/c/a18308623ce3
  - [net-next,8/8] SUNRPC: use min() to simplify the code
    (no matching commit)

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html