| Message ID | 1468306113-847-1-git-send-email-famz@redhat.com |
|---|---|
| State | New |
| Headers | show |
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>
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> >
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 >
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
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 */
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(-)