-----Original Message----- From: Eric Blake [mailto:eblake@redhat.com] Sent: Wednesday, October 09, 2013 11:49 AM To: Chen Hanxiao Cc: libvir-list@redhat.com Subject: Re: [libvirt] [PATCH 1/2]cgroup: introduce kernel version check function for cgroup
On 10/08/2013 02:38 AM, Chen Hanxiao wrote:
From: Chen Hanxiao <chenhanxiao@cn.fujitsu.com>
Some cgroup value range may change
s/range/ranges/
in the further kernel.
s/the further kernel/future kernels/
Introduce kernel version check function for cgroup. This will be helpful to determine the proper values.
I'm half-inclined to NACK. Version checks are lousy, as features may be backported to a distro-style kernel that reports an older version number. Is there any way you can probe for an actual feature, rather than relying on brittle version number checks?
Maybe we could leave this job to kernel.
+ +static bool virCgroupVersionCheck(const char * dstVersion)
Style: no space after '*' in pointer type declarations.
+{ + unsigned long currentVersion; + unsigned long version; + + if (virCgroupGetKernelVersion(¤tVersion) < 0) { + return -1; + } + + if (virParseVersionString(dstVersion, &version, true) < 0) { + return -1; + } + + return (((long long)currentVersion - (long long)version) >= 0) ? true : false;
The casts are pointless. Either we're on a 64-bit platform where it adds no precision, or we're on a 32-bit platform where the extra precision slows us down; but either way the version number already fits within a long.
Style: 'bool_expr ? true: false' is wasteful; just write 'bool_expr'.
Thus, this would be:
return currentVersion >= version;
Thanks for your explanation.
if we still determine we need this function.
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org