[libvirt] [PATCH] Fix: helper function virCompareLimitUlong should return -1 when the latter parameter is 0

[Eric Blake]

On 10/11/2013 12:50 AM, mars at linux.vnet.ibm.com wrote:
> From: Bing Bu Cao <mars at linux.vnet.ibm.com>

Long subject line.  Look at 'git shortlog -30' to get a feel for how to
write a concise summary.  I retitled it:

util: fix two virCompareLimitUlong bugs

> 
> The helper function virCompareLimitUlong compare limit values,

s/compare/compares/

> where value of 0 is equal to unlimited, if the latter parameter is 0,
> it should return -1 instead of 1, hence the user can only set hard_limit when
> swap_hard_limit currently is unlimited.
> 
> Signed-off-by: Bing Bu Cao <mars at linux.vnet.ibm.com>
> ---
>  src/util/virutil.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/src/util/virutil.c b/src/util/virutil.c
> index d9e0bc4..3dcf1fe 100644
> --- a/src/util/virutil.c
> +++ b/src/util/virutil.c
> @@ -2067,6 +2067,9 @@ virCompareLimitUlong(unsigned long long a, unsigned long b)

Oh my.  This function is BROKEN on 32-bit platforms!  Comparing a 64-bit
and 32-bit number, but where all callers pass 2 64-bit numbers, is prone
to miscalculation if 'b' was a multiple of 4G.

>      if (a == b)
>          return 0;
>  
> +    if (0 == b)

Yoda-style conditionals this is.  Although some projects swear by the
style of constants on the left of comparisons, we tend to avoid
it in libvirt.  I think 'if (!b)' is more concise anyways.

> +        return -1;
> +
>      if (a == 0 || a > b)
>          return 1;
>  
> 

Congratulations on your first libvirt patch.  ACK and pushed as follows:

From 5bf6c3f20c99d2f6e93f6826a1ba158ded63bded Mon Sep 17 00:00:00 2001
From: Bing Bu Cao <mars at linux.vnet.ibm.com>
Date: Fri, 11 Oct 2013 14:50:33 +0800
Subject: [PATCH] util: fix two virCompareLimitUlong bugs

The helper function virCompareLimitUlong compares limit values,
where value of 0 is equal to unlimited, if the latter parameter is 0,
it should return -1 instead of 1, hence the user can only set hard_limit
when
swap_hard_limit currently is unlimited.

Worse, all callers pass 2 64-bit values, but on 32-bit platforms,
the second argument was silently truncated to 32 bits, which
could lead to incorrect computations.

Signed-off-by: Bing Bu Cao <mars at linux.vnet.ibm.com>
Signed-off-by: Eric Blake <eblake at redhat.com>
---
 src/util/virutil.c | 5 ++++-
 src/util/virutil.h | 2 +-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/src/util/virutil.c b/src/util/virutil.c
index d9e0bc4..854c0ad 100644
--- a/src/util/virutil.c
+++ b/src/util/virutil.c
@@ -2062,11 +2062,14 @@ virFindFCHostCapableVport(const char
*sysfs_prefix ATTRIBUTE_UNUSED)
  * Returns 0 if the numbers are equal, -1 if b is greater, 1 if a is
greater.
  */
 int
-virCompareLimitUlong(unsigned long long a, unsigned long b)
+virCompareLimitUlong(unsigned long long a, unsigned long long b)
 {
     if (a == b)
         return 0;

+    if (!b)
+        return -1;
+
     if (a == 0 || a > b)
         return 1;

diff --git a/src/util/virutil.h b/src/util/virutil.h
index 4b06992..9f6fb20 100644
--- a/src/util/virutil.h
+++ b/src/util/virutil.h
@@ -168,7 +168,7 @@ char *virGetFCHostNameByWWN(const char *sysfs_prefix,

 char *virFindFCHostCapableVport(const char *sysfs_prefix);

-int virCompareLimitUlong(unsigned long long a, unsigned long b);
+int virCompareLimitUlong(unsigned long long a, unsigned long long b);

 int virParseOwnershipIds(const char *label, uid_t *uidPtr, gid_t *gidPtr);

-- 
1.8.3.1


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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 621 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20131011/0221241f/attachment-0001.sig>