> -----Original Message----- > From: Eric Blake [mailto:eblake@xxxxxxxxxx] > Sent: Wednesday, October 09, 2013 11:49 AM > To: Chen Hanxiao > Cc: libvir-list@xxxxxxxxxx > Subject: Re: [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@xxxxxxxxxxxxxx> > > > > 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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list