[libvirt] [PATCH] BSD: implement nodeGetMemoryStats

[John Ferlan]

On 01/02/2014 10:35 AM, Roman Bogorodskiy wrote:
> Add a BSD implementation of nodeGetMemoryStats based
> on sysctl(3).
> ---
>  src/nodeinfo.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 73 insertions(+)
> 

Code seems reasonable to me - I don't have the capability to test under
__APPLE__, but figured I could at least give it a once over.


I have a couple of suggestions below, but ACK in general though

John

> diff --git a/src/nodeinfo.c b/src/nodeinfo.c
> index 1838547..ecacfa4 100644
> --- a/src/nodeinfo.c
> +++ b/src/nodeinfo.c
> @@ -55,6 +55,8 @@
>  #define VIR_FROM_THIS VIR_FROM_NONE
>  
>  #if defined(__FreeBSD__) || defined(__APPLE__)
> +# define BSD_MEMORY_STATS_ALL 4
> +
>  static int
>  appleFreebsdNodeGetCPUCount(void)
>  {
> @@ -96,6 +98,75 @@ appleFreebsdNodeGetMemorySize(unsigned long *memory)
>  
>      return 0;
>  }
> +
> +static int
> +appleFreebsdNodeGetMemoryStats(virNodeMemoryStatsPtr params,
> +                               int *nparams)
> +{
> +    size_t i, j = 0;
> +    unsigned long pagesize = getpagesize() >> 10;
> +    long bufpages;
> +    size_t bufpages_size = sizeof(bufpages);
> +    struct field_sysctl_map {
> +        const char *field;
> +        const char *sysctl_name;
> +    } sysctl_map[] = {
> +        {VIR_NODE_MEMORY_STATS_TOTAL, "vm.stats.vm.v_page_count"},
> +        {VIR_NODE_MEMORY_STATS_FREE, "vm.stats.vm.v_free_count"},
> +        {VIR_NODE_MEMORY_STATS_CACHED, "vm.stats.vm.v_cache_count"},
> +        {NULL, NULL}
> +    };
> +
> +    if ((*nparams) == 0) {
> +        *nparams = BSD_MEMORY_STATS_ALL;
> +        return 0;
> +    }
> +
> +    if ((*nparams) != BSD_MEMORY_STATS_ALL) {
> +        virReportInvalidArg(nparams,
> +                            _("nparams in %s must be %d"),
> +                            __FUNCTION__, BSD_MEMORY_STATS_ALL);
> +        return -1;
> +    }
> +
> +    for (i = 0; sysctl_map[i].field != NULL; i++) {
> +        u_int value;
> +        size_t value_size = sizeof(value);
> +        virNodeMemoryStatsPtr param;
> +
> +        if (sysctlbyname(sysctl_map[i].sysctl_name, &value,
> +                         &value_size, NULL, 0) < 0) {
> +            virReportSystemError(errno, "%s", _("sysctl failed"));

s/"%s", _("sysctl failed")/_("sysctl failed for '%s'"),
sysctl_map[i].sysctl_name/

That way it'd be easier to determine which failed...

> +            return -1;
> +        }
> +
> +        param = &params[j++];
> +        if (virStrcpyStatic(param->field, sysctl_map[i].field) == NULL) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("Field '%s' too long for destination"),
> +                           sysctl_map[i].field);
> +            return -1;
> +        }
> +        param->value = (unsigned long long)value * pagesize;
> +    }
> +
> +    {
> +        virNodeMemoryStatsPtr param = &params[j];

Not that it's necessary now, but perhaps a j++ here just in case another
parameter was ever added and someone neglected this one...

> +
> +        if (sysctlbyname("vfs.bufspace", &bufpages, &bufpages_size, NULL, 0) < 0) {

Probably should add an error message here

> +            return -1;
> +        }
> +        if (virStrcpyStatic(param->field, VIR_NODE_MEMORY_STATS_BUFFERS) == NULL) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("Field '%s' too long for destination"),
> +                           VIR_NODE_MEMORY_STATS_BUFFERS);
> +            return -1;
> +        }
> +        param->value = (unsigned long long)bufpages >> 10;
> +    }
> +
> +    return 0;
> +}
>  #endif

Add a "/* defined(__FreeBSD__) || defined(__APPLE__) */" to the #endif -
just as a way to be clear.

>  
>  #ifdef __linux__
> @@ -1041,6 +1112,8 @@ int nodeGetMemoryStats(int cellNum ATTRIBUTE_UNUSED,
>  
>          return ret;
>      }
> +#elif defined(__FreeBSD__) || defined(__APPLE__)
> +    return appleFreebsdNodeGetMemoryStats(params, nparams);
>  #else
>      virReportError(VIR_ERR_NO_SUPPORT, "%s",
>                     _("node memory stats not implemented on this platform"));
>