[libvirt] [PATCH 1/2] FreeBSD: add nodeinfo support.

[Peter Krempa]

On 12/16/12 15:47, Roman Bogorodskiy wrote:
> Uses sysctl(3) interface to obtain CPU and memory information.
> ---
>   src/nodeinfo.c | 75 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 74 insertions(+), 1 deletion(-)
>
> diff --git a/src/nodeinfo.c b/src/nodeinfo.c
> index 096000b..f6fdf90 100644
> --- a/src/nodeinfo.c
> +++ b/src/nodeinfo.c
> @@ -38,6 +38,11 @@
>   # include <numa.h>
>   #endif
>
> +#ifdef __FreeBSD__
> +# include <sys/types.h>
> +# include <sys/sysctl.h>
> +#endif
> +
>   #include "c-ctype.h"
>   #include "memory.h"
>   #include "nodeinfo.h"
> @@ -53,6 +58,24 @@
>
>   #define VIR_FROM_THIS VIR_FROM_NONE
>
> +#ifdef __FreeBSD__
> +static int freebsdNodeGetCPUCount(void);

We usually don't forward-declare functions if they are defined right after.

> +
> +static int
> +freebsdNodeGetCPUCount(void)
> +{
> +    int ncpu_mib[2] = { CTL_HW, HW_NCPU };
> +    unsigned long ncpu;
> +    size_t ncpu_len = sizeof(ncpu);
> +
> +    if (sysctl(ncpu_mib, 2, &ncpu, &ncpu_len, NULL, 0) == -1) {
> +        return -1;
> +    }
> +
> +    return ncpu;
> +}
> +#endif
> +
>   #ifdef __linux__
>   # define CPUINFO_PATH "/proc/cpuinfo"
>   # define SYSFS_SYSTEM_PATH "/sys/devices/system"
> @@ -871,6 +894,46 @@ cleanup:
>       VIR_FORCE_FCLOSE(cpuinfo);
>       return ret;
>       }
> +#elif defined(__FreeBSD__)
> +    {
> +    nodeinfo->nodes = 1;
> +    nodeinfo->sockets = 1;
> +    nodeinfo->cores = 1;
> +    nodeinfo->threads = 1;
> +
> +    nodeinfo->cpus = freebsdNodeGetCPUCount();

Documentation to the nodeinfo structure says that if the correct 
topology cannot be enumerated, the actual count of processors should be 
returned in the "cores" field instead of cpus. On linux this kind of 
"lie" is used when the host is a strange NUMA architecture, where we 
can't successfully determine the actual count of NUMA nodes and stuff.

> +
> +    if (nodeinfo->cpus == -1) {
> +        virReportError(VIR_ERR_NO_SUPPORT, "%s",
> +                     _("cannot obtain cpu count"));

Hmm, it would be better (as I see this error reporting code twice in 
this patch) for the freebsdNodeGetCPUCount to report the error. Also I 
suppose that the sysctl call sets the "errno" variable where it would be 
better to use the virReportSystemError function that takes errno as a 
param and prints the system error. Also I think that the 
VIR_ERR_NO_SUPPORT isn't really appropriate for this kind of errors as 
this error code is mainly used when the remote daemon doesn't support 
the method that was called.

> +    }
> +
> +    unsigned long cpu_freq;
> +    size_t cpu_freq_len = sizeof(cpu_freq);
> +
> +    if (sysctlbyname("dev.cpu.0.freq", &cpu_freq, &cpu_freq_len, NULL, 0) < 0) {
> +        virReportError(VIR_ERR_NO_SUPPORT, "%s",
> +                    _("cannot obtain cpu freq"));

This also seems as a candidate for virReportSystemError()

> +        return -1;
> +    }
> +
> +    nodeinfo->mhz = cpu_freq;
> +
> +    /* get memory information */
> +    int mib[2] = { CTL_HW, HW_PHYSMEM };
> +    unsigned long physmem;
> +    size_t len = sizeof(physmem);
> +
> +    if (sysctl(mib, 2, &physmem, &len, NULL, 0) == -1) {
> +        virReportError(VIR_ERR_NO_SUPPORT, "%s",
> +                    _("cannot obtain memory count"));

And here too. Also "memory count" is awkward. Please use "memory size"

> +        return -1;
> +    }
> +
> +    nodeinfo->memory = (unsigned long)(physmem / 1024);
> +
> +    return 0;
> +    }
>   #else
>       /* XXX Solaris will need an impl later if they port QEMU driver */
>       virReportError(VIR_ERR_NO_SUPPORT, "%s",
> @@ -978,7 +1041,7 @@ int nodeGetMemoryStats(virConnectPtr conn ATTRIBUTE_UNUSED,
>   int
>   nodeGetCPUCount(void)
>   {
> -#ifdef __linux__
> +#if defined(__linux__)
>       /* To support older kernels that lack cpu/present, such as 2.6.18
>        * in RHEL5, we fall back to count cpu/cpuNN entries; this assumes
>        * that such kernels also lack hotplug, and therefore cpu/cpuNN
> @@ -1008,6 +1071,16 @@ nodeGetCPUCount(void)
>
>       VIR_FREE(cpupath);
>       return i;
> +#elif defined(__FreeBSD__)
> +    int cpu_count = -1;
> +
> +    cpu_count = freebsdNodeGetCPUCount();
> +    if (cpu_count == -1) {
> +        virReportError(VIR_ERR_NO_SUPPORT, "%s",
> +                       _("cannot obtain cpu count"));

Here's the duplicate code as noted above.

> +    }
> +
> +    return cpu_count;
>   #else
>       virReportError(VIR_ERR_NO_SUPPORT, "%s",
>                      _("host cpu counting not implemented on this platform"));
>

I unfortunately don't have FreeBSD at hand to actually test the system 
calls so I didn't check if this works :(

Peter