Re: [libvirt] [PATCH 1/2]cgroup: introduce kernel version check function for cgroup

-----Original Message----- From: Eric Blake [mailto:eblake@redhat.com] Sent: Wednesday, October 09, 2013 11:49 AM To: Chen Hanxiao Cc: libvir-list(a)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(a)cn.fujitsu.com&gt; > > 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?

> + > +static bool virCgroupVersionCheck(const char * dstVersion) Style: no space after '*' in pointer type declarations. > +{ > + unsigned long currentVersion; > + unsigned long version; > + > + if (virCgroupGetKernelVersion(&currentVersion) < 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;

if we still determine we need this function. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org