mbox series

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

Message ID 20240822133908.1042240-1-lizetao1@huawei.com
Headers show
Series Some modifications to optimize code readability | expand

Message

Li Zetao Aug. 22, 2024, 1:39 p.m. UTC 
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(-)

Comments

Li Zetao Aug. 22, 2024, 1:50 p.m. UTC | #1 
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.
Jacob Keller Aug. 22, 2024, 8:20 p.m. UTC | #2 
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
patchwork-bot+netdevbpf@kernel.org Aug. 26, 2024, 5 p.m. UTC | #3 
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!
patchwork-bot+bluetooth@kernel.org Sept. 12, 2024, 4:33 p.m. UTC | #4 
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!