linux-kernel - Re: [PATCH] KVM: Fix the warning by the min()

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <8881d7b4-0c31-cafd-1158-0d42c1c7f43a@redhat.com>
Date:   Tue, 16 Nov 2021 16:56:23 +0100
From:   Paolo Bonzini <pbonzini@...hat.com>
To:     zhaoxiao <zhaoxiao@...ontech.com>
Cc:     kvm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] KVM: Fix the warning by the min()

On 11/16/21 13:10, zhaoxiao wrote:
> Fix following coccicheck warning:
> virt/kvm/kvm_main.c:4995:10-11: WARNING opportunity for min()
> virt/kvm/kvm_main.c:4924:10-11: WARNING opportunity for min()
> 
> Signed-off-by: zhaoxiao <zhaoxiao@...ontech.com>

No, this is unreadable for two reasons:

First, the code in parentheses for min(func(), 0) is very long.  Usually 
min has very short arguments. By the time you have reached the closing 
parentheses, you have completely forgotten that there was a min() at the 
beginning.  So perhaps one possibility could be

	return min(r, 0);

However, the second reason is that "r < 0" is a very common way to 
express "if there was an error".  In this case that would be

	r = __kvm_io_bus_write(vcpu, bus, &range, val);
	if (r < 0)		// "if __kvm_io_bus_write failed"
		return r;

	return 0;

That "r < 0" is what will catch the attention of the person that is 
reading the code, no matter if it is an "if" or (as in the existing 
code), a "return".  Using "min" removes the idiom that tells the person 
"this is checking for errors".

Thanks,

Paolo

> ---
>   virt/kvm/kvm_main.c | 10 ++++------
>   1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index d31724500501..bd646c64722d 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -4910,7 +4910,6 @@ int kvm_io_bus_write(struct kvm_vcpu *vcpu, enum kvm_bus bus_idx, gpa_t addr,
>   {
>   	struct kvm_io_bus *bus;
>   	struct kvm_io_range range;
> -	int r;
>   
>   	range = (struct kvm_io_range) {
>   		.addr = addr,
> @@ -4920,8 +4919,8 @@ int kvm_io_bus_write(struct kvm_vcpu *vcpu, enum kvm_bus bus_idx, gpa_t addr,
>   	bus = srcu_dereference(vcpu->kvm->buses[bus_idx], &vcpu->kvm->srcu);
>   	if (!bus)
>   		return -ENOMEM;
> -	r = __kvm_io_bus_write(vcpu, bus, &range, val);
> -	return r < 0 ? r : 0;
> +
> +	return min(__kvm_io_bus_write(vcpu, bus, &range, val), 0);
>   }
>   EXPORT_SYMBOL_GPL(kvm_io_bus_write);
>   
> @@ -4981,7 +4980,6 @@ int kvm_io_bus_read(struct kvm_vcpu *vcpu, enum kvm_bus bus_idx, gpa_t addr,
>   {
>   	struct kvm_io_bus *bus;
>   	struct kvm_io_range range;
> -	int r;
>   
>   	range = (struct kvm_io_range) {
>   		.addr = addr,
> @@ -4991,8 +4989,8 @@ int kvm_io_bus_read(struct kvm_vcpu *vcpu, enum kvm_bus bus_idx, gpa_t addr,
>   	bus = srcu_dereference(vcpu->kvm->buses[bus_idx], &vcpu->kvm->srcu);
>   	if (!bus)
>   		return -ENOMEM;
> -	r = __kvm_io_bus_read(vcpu, bus, &range, val);
> -	return r < 0 ? r : 0;
> +
> +	return min(__kvm_io_bus_read(vcpu, bus, &range, val), 0);
>   }
>   
>   /* Caller must hold slots_lock. */
>