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 = ¶ms[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 = ¶ms[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"));
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list