[Qemu-devel] [PATCH] util: Fix MIN_NON_ZERO

[Qemu-devel] [PATCH] util: Fix MIN_NON_ZERO

[Fam Zheng]

MIN_NON_ZERO(0, 1) is evaluated to 0. Rewrite the macro to fix it.

Reported-by: Miroslav Rezanina <mrezanin@redhat.com>
Signed-off-by: Fam Zheng <famz@redhat.com>
---
 include/qemu/osdep.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index e63da28..e4c6ae6 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -151,7 +151,8 @@ extern int daemon(int, int);
 /* Minimum function that returns zero only iff both values are zero.
  * Intended for use with unsigned values only. */
 #ifndef MIN_NON_ZERO
-#define MIN_NON_ZERO(a, b) (((a) != 0 && (a) < (b)) ? (a) : (b))
+#define MIN_NON_ZERO(a, b) ((a) == 0 ? (b) : \
+                                ((b) == 0 ? (a) : (MIN(a, b))))
 #endif
 
 /* Round number down to multiple */
-- 
2.7.4

Re: [Qemu-devel] [PATCH] util: Fix MIN_NON_ZERO

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1519 bytes --]

On 07/12/2016 12:48 AM, Fam Zheng wrote:
> MIN_NON_ZERO(0, 1) is evaluated to 0. Rewrite the macro to fix it.

Huh?

Old expansion, in various stages:

(((0) != 0 && (0) < (1)) ? (0) : (1))
((0 && 1) ? 0 : 1)
(0 ? 0 : 1)
1

Maybe you meant MIN_NON_ZERO(1, 0), which evaluates to:

(((1) != 0 && (1) < (0)) ? (1) : (0))
((1 && 0) ? 1 : 0)
(0 ? 1 : 0)
0

in which case, you are correct that there is a bug.

> 
> Reported-by: Miroslav Rezanina <mrezanin@redhat.com>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  include/qemu/osdep.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index e63da28..e4c6ae6 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -151,7 +151,8 @@ extern int daemon(int, int);
>  /* Minimum function that returns zero only iff both values are zero.
>   * Intended for use with unsigned values only. */
>  #ifndef MIN_NON_ZERO
> -#define MIN_NON_ZERO(a, b) (((a) != 0 && (a) < (b)) ? (a) : (b))
> +#define MIN_NON_ZERO(a, b) ((a) == 0 ? (b) : \
> +                                ((b) == 0 ? (a) : (MIN(a, b))))

Another way might be:

(!(a) == !(b) ? MIN(a, b) : (a) + (b))

but you have to put more mental thought into that.  I'm just fine with
your version.

With the commit message fixed,
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH] util: Fix MIN_NON_ZERO

[Paolo Bonzini]

On 12/07/2016 17:54, Eric Blake wrote:
> On 07/12/2016 12:48 AM, Fam Zheng wrote:
>> MIN_NON_ZERO(0, 1) is evaluated to 0. Rewrite the macro to fix it.
> 
> Huh?
> 
> Old expansion, in various stages:
> 
> (((0) != 0 && (0) < (1)) ? (0) : (1))
> ((0 && 1) ? 0 : 1)
> (0 ? 0 : 1)
> 1
> 
> Maybe you meant MIN_NON_ZERO(1, 0), which evaluates to:
> 
> (((1) != 0 && (1) < (0)) ? (1) : (0))
> ((1 && 0) ? 1 : 0)
> (0 ? 1 : 0)
> 0
> 
> in which case, you are correct that there is a bug.

Commit message fixed, patch queued.

Paolo

>>
>> Reported-by: Miroslav Rezanina <mrezanin@redhat.com>
>> Signed-off-by: Fam Zheng <famz@redhat.com>
>> ---
>>  include/qemu/osdep.h | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
>> index e63da28..e4c6ae6 100644
>> --- a/include/qemu/osdep.h
>> +++ b/include/qemu/osdep.h
>> @@ -151,7 +151,8 @@ extern int daemon(int, int);
>>  /* Minimum function that returns zero only iff both values are zero.
>>   * Intended for use with unsigned values only. */
>>  #ifndef MIN_NON_ZERO
>> -#define MIN_NON_ZERO(a, b) (((a) != 0 && (a) < (b)) ? (a) : (b))
>> +#define MIN_NON_ZERO(a, b) ((a) == 0 ? (b) : \
>> +                                ((b) == 0 ? (a) : (MIN(a, b))))
> 
> Another way might be:
> 
> (!(a) == !(b) ? MIN(a, b) : (a) + (b))
> 
> but you have to put more mental thought into that.  I'm just fine with
> your version.
> 
> With the commit message fixed,
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 

Re: [Qemu-devel] [PATCH] util: Fix MIN_NON_ZERO

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1088 bytes --]

On Tue, Jul 12, 2016 at 02:48:33PM +0800, Fam Zheng wrote:
> MIN_NON_ZERO(0, 1) is evaluated to 0. Rewrite the macro to fix it.

This is incorrect.  The actual results are:

MIN_NON_ZERO(0, 1) = 1
MIN_NON_ZERO(1, 0) = 0

The second case is the buggy one.

> 
> Reported-by: Miroslav Rezanina <mrezanin@redhat.com>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  include/qemu/osdep.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index e63da28..e4c6ae6 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -151,7 +151,8 @@ extern int daemon(int, int);
>  /* Minimum function that returns zero only iff both values are zero.
>   * Intended for use with unsigned values only. */
>  #ifndef MIN_NON_ZERO
> -#define MIN_NON_ZERO(a, b) (((a) != 0 && (a) < (b)) ? (a) : (b))
> +#define MIN_NON_ZERO(a, b) ((a) == 0 ? (b) : \
> +                                ((b) == 0 ? (a) : (MIN(a, b))))
>  #endif
>  
>  /* Round number down to multiple */
> -- 
> 2.7.4
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH] util: Fix MIN_NON_ZERO

[Peter Lieven]

Am 12.07.2016 um 18:24 schrieb Paolo Bonzini:
>
> On 12/07/2016 17:54, Eric Blake wrote:
>> On 07/12/2016 12:48 AM, Fam Zheng wrote:
>>> MIN_NON_ZERO(0, 1) is evaluated to 0. Rewrite the macro to fix it.
>> Huh?
>>
>> Old expansion, in various stages:
>>
>> (((0) != 0 && (0) < (1)) ? (0) : (1))
>> ((0 && 1) ? 0 : 1)
>> (0 ? 0 : 1)
>> 1
>>
>> Maybe you meant MIN_NON_ZERO(1, 0), which evaluates to:
>>
>> (((1) != 0 && (1) < (0)) ? (1) : (0))
>> ((1 && 0) ? 1 : 0)
>> (0 ? 1 : 0)
>> 0
>>
>> in which case, you are correct that there is a bug.
> Commit message fixed, patch queued.
>
> Paolo

Shouldn't we Cc qemu-stable?

Peter