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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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?

> +
> +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

Attachment: signature.asc
Description: OpenPGP digital signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]