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

On 10/11/2013 12:50 AM, mars(a)linux.vnet.ibm.com wrote: 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 s/compare/compares/ 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. 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. 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(a)linux.vnet.ibm.com&gt; 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(a)linux.vnet.ibm.com&gt; Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- 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