On Tue, Jun 07, 2011 at 09:58:47AM +0900, Minoru Usui wrote: ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

virNodeGetCPUStats: Define internal driver API Signed-off-by: Minoru Usui <usui(a)mxm.nes.nec.co.jp&gt; --- src/driver.h | 9 +++++++++ 1 files changed, 9 insertions(+), 0 deletions(-) diff --git a/src/driver.h b/src/driver.h index 5df798a..a39411b 100644 --- a/src/driver.h +++ b/src/driver.h @@ -371,6 +371,14 @@ typedef struct _virDriver virDriver; typedef virDriver *virDriverPtr; typedef int + (*virDrvNodeGetCPUStats) + (virConnectPtr conn, + int cpuNum, + virCPUStatsPtr params, + int *nparams, + unsigned int flags); + +typedef int (*virDrvNodeGetCellsFreeMemory) (virConnectPtr conn, unsigned long long *freeMems, @@ -705,6 +713,7 @@ struct _virDriver { virDrvDomainBlockPeek domainBlockPeek; virDrvDomainMemoryPeek domainMemoryPeek; virDrvDomainGetBlockInfo domainGetBlockInfo; + virDrvNodeGetCPUStats nodeGetCPUStats; virDrvNodeGetCellsFreeMemory nodeGetCellsFreeMemory; virDrvNodeGetFreeMemory nodeGetFreeMemory; virDrvDomainEventRegister domainEventRegister; -- 1.7.1

virNodeGetCPUTime: Implement public API Signed-off-by: Minoru Usui <usui(a)mxm.nes.nec.co.jp&gt; --- src/libvirt.c | 91 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 91 insertions(+), 0 deletions(-) diff --git a/src/libvirt.c b/src/libvirt.c index eaae0ec..45d9be7 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -5338,6 +5338,97 @@ error: } /** + * virNodeGetCPUStats: + * @conn: pointer to the hypervisor connection. + * @cpuNum: number of node cpu. (VIR_CPU_STATS_ALL_CPUS means total cpu + * statistics) + * @params: pointer to node cpu time parameter objects + * @nparams: number of node cpu time parameter (this value should be same or + * less than the number of parameters supported) + * @flags: currently unused, for future extension. always pass 0. + * + * This function provides individual cpu statistics of the node. + * If you want to get total cpu statistics of the node, you must specify + * VIR_CPU_STATS_ALL_CPUS to @cpuNum. + * The @params array will be filled with the values equal to the number of + * parameters suggested by @nparams + * + * As the value of @nparams is dynamic, call the API setting @nparams to 0 and + * @params as NULL, the API returns the number of parameters supported by the + * HV by updating @nparams on SUCCESS. The caller should then allocate @params + * array, i.e. (sizeof(@virCPUStats) * @nparams) bytes and call + * the API again. + * + * Here is the sample code snippet: + * + * if ((virNodeGetCPUStats(conn, cpuNum, NULL, &nparams, 0) == 0) && + * (nparams != 0)) { + * params = vshMalloc(ctl, sizeof(virCPUStats) * nparams); + * memset(params, 0, sizeof(virCPUStats) * nparams); + * if (virNodeGetCPUStats(conn, cpuNum, params, &nparams, 0)) { + * vshError(ctl, "%s", _("Unable to get node cpu stats")); + * goto error; + * } + * } + * + * This function doesn't requires privileged access to the hypervisor. + * This function expects the caller to allocate the @params. + * + * CPU time Statistics: + * + * VIR_NODE_CPU_STATS_KERNEL: + * The cumulative CPU time which spends by kernel, + * when the node booting up.(nanoseconds) + * VIR_NODE_CPU_STATS_USER: + * The cumulative CPU time which spends by user processes, + * when the node booting up.(nanoseconds) + * VIR_NODE_CPU_STATS_IDLE: + * The cumulative idle CPU time, when the node booting up.(nanoseconds) + * VIR_NODE_CPU_STATS_IOWAIT: + * The cumulative I/O wait CPU time, when the node booting up.(nanoseconds) + * VIR_NODE_CPU_STATS_UTILIZATION: + * The CPU utilization. The usage value is in percent and 100% + * represents all CPUs on the server. + * + * Returns -1 in case of error, 0 in case of success. + */ +int virNodeGetCPUStats (virConnectPtr conn, + int cpuNum, + virCPUStatsPtr params, + int *nparams, unsigned int flags) +{ + VIR_DEBUG("conn=%p, cpuNum=%d, params=%p, nparams=%d, flags=%u", + conn, cpuNum, params, nparams ? *nparams : -1, flags); + + virResetLastError(); + + if (!VIR_IS_CONNECT(conn)) { + virLibConnError(VIR_ERR_INVALID_CONN, __FUNCTION__); + virDispatchError(NULL); + return -1; + } + + if ((nparams == NULL) || (*nparams < 0) || + ((cpuNum < 0) && (cpuNum != VIR_CPU_STATS_ALL_CPUS))) { + virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); + goto error; + } + + if (conn->driver->nodeGetCPUStats) { + int ret; + ret = conn->driver->nodeGetCPUStats (conn, cpuNum, params, nparams, flags); + if (ret < 0) + goto error; + return ret; + } + virLibConnError(VIR_ERR_NO_SUPPORT, __FUNCTION__); + +error: + virDispatchError(conn); + return -1; +} + +/** * virNodeGetFreeMemory: * @conn: pointer to the hypervisor connection * -- 1.7.1

On 06/14/2011 03:04 AM, Daniel P. Berrange wrote: Trailing whitespace; tsk-tsk. 'make syntax-check' caught it. Copy and paste from a previous example, so I've fixed that, too. s/requires/require/ Pushed with this squashed in (along with 2/12 unchanged): diff --git i/src/libvirt.c w/src/libvirt.c index ccb00cd..ace0120 100644 --- i/src/libvirt.c +++ w/src/libvirt.c @@ -3008,16 +3008,15 @@ error: * array, i.e. (sizeof(@virTypedParameter) * @nparams) bytes and call the API * again. * - * Here is the sample code snippet: + * Here is a sample code snippet: * * if ((virDomainGetMemoryParameters(dom, NULL, &nparams, 0) == 0) && * (nparams != 0)) { - * params = vshMalloc(ctl, sizeof(*params) * nparams); + * if ((params = malloc(sizeof(*params) * nparams)) == NULL) + * goto error; * memset(params, 0, sizeof(*params) * nparams); - * if (virDomainGetMemoryParameters(dom, params, &nparams, 0)) { - * vshError(ctl, "%s", _("Unable to get memory parameters")); + * if (virDomainGetMemoryParameters(dom, params, &nparams, 0)) * goto error; - * } * } * * This function requires privileged access to the hypervisor. This function @@ -5288,7 +5287,7 @@ error: /** * virNodeGetCPUStats: * @conn: pointer to the hypervisor connection. - * @cpuNum: number of node cpu. (VIR_CPU_STATS_ALL_CPUS means total cpu + * @cpuNum: number of node cpu. (VIR_CPU_STATS_ALL_CPUS means total cpu * statistics) * @params: pointer to node cpu time parameter objects * @nparams: number of node cpu time parameter (this value should be same or @@ -5307,19 +5306,18 @@ error: * array, i.e. (sizeof(@virCPUStats) * @nparams) bytes and call * the API again. * - * Here is the sample code snippet: + * Here is a sample code snippet: * * if ((virNodeGetCPUStats(conn, cpuNum, NULL, &nparams, 0) == 0) && * (nparams != 0)) { - * params = vshMalloc(ctl, sizeof(virCPUStats) * nparams); + * if ((params = malloc(sizeof(virCPUStats) * nparams)) == NULL) + * goto error; * memset(params, 0, sizeof(virCPUStats) * nparams); - * if (virNodeGetCPUStats(conn, cpuNum, params, &nparams, 0)) { - * vshError(ctl, "%s", _("Unable to get node cpu stats")); + * if (virNodeGetCPUStats(conn, cpuNum, params, &nparams, 0)) * goto error; - * } * } * - * This function doesn't requires privileged access to the hypervisor. + * This function doesn't require privileged access to the hypervisor. * This function expects the caller to allocate the @params. * * CPU time Statistics: -- Eric Blake eblake(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

On Tue, Jun 07, 2011 at 10:01:12AM +0900, Minoru Usui wrote: ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

virNodeGetCPUStats: Implement virsh support Signed-off-by: Minoru Usui <usui(a)mxm.nes.nec.co.jp&gt; --- tools/virsh.c | 134 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ tools/virsh.pod | 7 +++ 2 files changed, 141 insertions(+), 0 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 5679a2d..2fbc91a 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -3682,6 +3682,139 @@ cmdNodeinfo(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) } /* + * "nodecpustats" command + */ +static const vshCmdInfo info_nodecpustats[] = { + {"help", N_("Prints cpu stats of the node.")}, + {"desc", N_("Returns cpu stats of the node.(nsec)")}, + {NULL, NULL} +}; + +static const vshCmdOptDef opts_node_cpustats[] = { + {"cpu", VSH_OT_INT, 0, N_("prints specified cpu statistics only.")}, + {"percent", VSH_OT_BOOL, 0, N_("prints by percentage during 1 second.")}, + {NULL, 0, 0, NULL} +}; + +static bool +cmdNodeCPUStats(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) +{ + int i, j; + bool flag_utilization = false; + bool flag_percent = vshCommandOptBool(cmd, "percent"); + int cpuNum = VIR_CPU_STATS_ALL_CPUS; + virCPUStatsPtr params; + int nparams = 0; + bool ret = false; + struct cpu_stats { + unsigned long long user; + unsigned long long sys; + unsigned long long idle; + unsigned long long iowait; + unsigned long long util; + } cpu_stats[2]; + double user_time, sys_time, idle_time, iowait_time, total_time; + double usage; + + if (!vshConnectionUsability(ctl, ctl->conn)) + return false; + + if (vshCommandOptInt(cmd, "cpu", &cpuNum) < 0) { + vshError(ctl, "%s", _("Invalid value of cpuNum")); + return false; + } + + if (virNodeGetCPUStats(ctl->conn, cpuNum, NULL, &nparams, 0) != 0) { + vshError(ctl, "%s", + _("Unable to get number of cpu stats")); + return false; + } + if (nparams == 0) { + /* nothing to output */ + return true; + } + + memset(cpu_stats, 0, sizeof(struct cpu_stats) * 2); + params = vshCalloc(ctl, nparams, sizeof(*params)); + + i = 0; + do { + if (virNodeGetCPUStats(ctl->conn, cpuNum, params, &nparams, 0) != 0) { + vshError(ctl, "%s", _("Unable to get node cpu stats")); + goto cleanup; + } + + for (j = 0; j < nparams; j++) { + unsigned long long value = params[j].value; + + if (strcmp(params[j].field, VIR_CPU_STATS_KERNEL) == 0) + cpu_stats[i].sys = value; + + if (strcmp(params[j].field, VIR_CPU_STATS_USER) == 0) + cpu_stats[i].user = value; + + if (strcmp(params[j].field, VIR_CPU_STATS_IDLE) == 0) + cpu_stats[i].idle = value; + + if (strcmp(params[j].field, VIR_CPU_STATS_IOWAIT) == 0) + cpu_stats[i].iowait = value; + + if (strcmp(params[j].field, VIR_CPU_STATS_UTILIZATION) == 0) { + cpu_stats[i].util = value; + flag_utilization = true; + } + } + + if (flag_utilization || (flag_percent == false)) + break; + + i++; + sleep(1); + } while(i < 2); + + if (flag_percent == false) { + if (flag_utilization == false) { + vshPrint(ctl, "%-15s %20llu\n", _("user :"), cpu_stats[0].user); + vshPrint(ctl, "%-15s %20llu\n", _("system:"), cpu_stats[0].sys); + vshPrint(ctl, "%-15s %20llu\n", _("idle :"), cpu_stats[0].idle); + vshPrint(ctl, "%-15s %20llu\n", _("iowait:"), cpu_stats[0].iowait); + } + } else { + if (flag_utilization) { + usage = cpu_stats[0].util; + + vshPrint(ctl, "%-15s %5.1lf%%\n", _("usage:"), usage); + vshPrint(ctl, "%-15s %5.1lf%%\n", _("idle :"), 100 - usage); + } else { + user_time = cpu_stats[1].user - cpu_stats[0].user; + sys_time = cpu_stats[1].sys - cpu_stats[0].sys; + idle_time = cpu_stats[1].idle - cpu_stats[0].idle; + iowait_time = cpu_stats[1].iowait - cpu_stats[0].iowait; + total_time = user_time + sys_time + idle_time + iowait_time; + + usage = (user_time + sys_time) / total_time * 100; + + vshPrint(ctl, "%-15s %5.1lf%%\n", + _("usage:"), usage); + vshPrint(ctl, "%-15s %5.1lf%%\n", + _(" user :"), user_time / total_time * 100); + vshPrint(ctl, "%-15s %5.1lf%%\n", + _(" system:"), sys_time / total_time * 100); + vshPrint(ctl, "%-15s %5.1lf%%\n", + _("idle :"), idle_time / total_time * 100); + vshPrint(ctl, "%-15s %5.1lf%%\n", + _("iowait:"), iowait_time / total_time * 100); + } + } + + ret = true; + + cleanup: + VIR_FREE(params); + return ret; +} + +/* * "capabilities" command */ static const vshCmdInfo info_capabilities[] = { @@ -11293,6 +11426,7 @@ static const vshCmdDef hostAndHypervisorCmds[] = { {"freecell", cmdFreecell, opts_freecell, info_freecell, 0}, {"hostname", cmdHostname, NULL, info_hostname, 0}, {"nodeinfo", cmdNodeinfo, NULL, info_nodeinfo, 0}, + {"nodecpustats", cmdNodeCPUStats, opts_node_cpustats, info_nodecpustats}, {"qemu-monitor-command", cmdQemuMonitorCommand, opts_qemu_monitor_command, info_qemu_monitor_command, 0}, {"sysinfo", cmdSysinfo, NULL, info_sysinfo, 0}, diff --git a/tools/virsh.pod b/tools/virsh.pod index 6ca3002..ade4eab 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -237,6 +237,13 @@ Print the XML representation of the hypervisor sysinfo, if available. Returns basic information about the node, like number and type of CPU, and size of the physical memory. +=item B<nodecpustats> optional I<--cpu> I<--percent> + +Returns cpu stats of the node. +If I<--cpu> is specified, this will prints specified cpu statistics only. +If I<--percent> is specified, this will prints percentage of each kind of cpu +statistics during 1 second. + =item B<capabilities> Print an XML document describing the capabilities of the hypervisor -- 1.7.1

On 06/14/2011 03:06 AM, Daniel P. Berrange wrote: Not sure that the (nsec) string aids understanding here, so I nuked it. 2 is a magic number here, easily avoided. 'make syntax-check' called you on this; use STREQ instead. For a (minor) efficiency boost, we can use 'else if' instead of 'if'. Comparison against bool is discouraged in HACKING. Sort these lines, and for consistency, keep the trailing 0. The usage is [--cpu] <cpu>; I've tweaked this slightly. Here's what I squashed in before pushing: diff --git i/tools/virsh.c w/tools/virsh.c index 6071aaa..1e2385c 100644 --- i/tools/virsh.c +++ w/tools/virsh.c @@ -3718,7 +3718,7 @@ cmdNodeinfo(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) */ static const vshCmdInfo info_nodecpustats[] = { {"help", N_("Prints cpu stats of the node.")}, - {"desc", N_("Returns cpu stats of the node.(nsec)")}, + {"desc", N_("Returns cpu stats of the node.")}, {NULL, NULL} }; @@ -3766,7 +3766,7 @@ cmdNodeCPUStats(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) return true; } - memset(cpu_stats, 0, sizeof(struct cpu_stats) * 2); + memset(cpu_stats, 0, sizeof(cpu_stats)); params = vshCalloc(ctl, nparams, sizeof(*params)); i = 0; @@ -3779,33 +3779,29 @@ cmdNodeCPUStats(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) for (j = 0; j < nparams; j++) { unsigned long long value = params[j].value; - if (strcmp(params[j].field, VIR_CPU_STATS_KERNEL) == 0) + if (STREQ(params[j].field, VIR_CPU_STATS_KERNEL)) { cpu_stats[i].sys = value; - - if (strcmp(params[j].field, VIR_CPU_STATS_USER) == 0) + } else if (STREQ(params[j].field, VIR_CPU_STATS_USER)) { cpu_stats[i].user = value; - - if (strcmp(params[j].field, VIR_CPU_STATS_IDLE) == 0) + } else if (STREQ(params[j].field, VIR_CPU_STATS_IDLE)) { cpu_stats[i].idle = value; - - if (strcmp(params[j].field, VIR_CPU_STATS_IOWAIT) == 0) + } else if (STREQ(params[j].field, VIR_CPU_STATS_IOWAIT)) { cpu_stats[i].iowait = value; - - if (strcmp(params[j].field, VIR_CPU_STATS_UTILIZATION) == 0) { + } else if (STREQ(params[j].field, VIR_CPU_STATS_UTILIZATION)) { cpu_stats[i].util = value; flag_utilization = true; } } - if (flag_utilization || (flag_percent == false)) + if (flag_utilization || !flag_percent) break; i++; sleep(1); } while(i < 2); - if (flag_percent == false) { - if (flag_utilization == false) { + if (!flag_percent) { + if (!flag_utilization) { vshPrint(ctl, "%-15s %20llu\n", _("user :"), cpu_stats[0].user); vshPrint(ctl, "%-15s %20llu\n", _("system:"), cpu_stats[0].sys); vshPrint(ctl, "%-15s %20llu\n", _("idle :"), cpu_stats[0].idle); @@ -11526,8 +11522,8 @@ static const vshCmdDef hostAndHypervisorCmds[] = { VSH_CMD_FLAG_NOCONNECT}, {"freecell", cmdFreecell, opts_freecell, info_freecell, 0}, {"hostname", cmdHostname, NULL, info_hostname, 0}, + {"nodecpustats", cmdNodeCPUStats, opts_node_cpustats, info_nodecpustats, 0}, {"nodeinfo", cmdNodeinfo, NULL, info_nodeinfo, 0}, - {"nodecpustats", cmdNodeCPUStats, opts_node_cpustats, info_nodecpustats}, {"qemu-monitor-command", cmdQemuMonitorCommand, opts_qemu_monitor_command, info_qemu_monitor_command, 0}, {"sysinfo", cmdSysinfo, NULL, info_sysinfo, 0}, diff --git i/tools/virsh.pod w/tools/virsh.pod index b2dbbb9..b4c580a 100644 --- i/tools/virsh.pod +++ w/tools/virsh.pod @@ -239,10 +239,10 @@ and size of the physical memory. The output corresponds to virNodeInfo structure. Specifically, the "CPU socket(s)" field means number of CPU sockets per NUMA cell. -=item B<nodecpustats> optional I<--cpu> I<--percent> +=item B<nodecpustats> optional I<cpu> I<--percent> Returns cpu stats of the node. -If I<--cpu> is specified, this will prints specified cpu statistics only. +If I<cpu> is specified, this will prints specified cpu statistics only. If I<--percent> is specified, this will prints percentage of each kind of cpu statistics during 1 second. -- Eric Blake eblake(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

On Tue, Jun 07, 2011 at 10:02:55AM +0900, Minoru Usui wrote: cpu_header is declared to be 8 bytes in size, which only allows for integers upto 4 digits long, before we get a buffer overflow here. gnulib has some macro which can be used to declare a string buffer large enough to hold an integer, but I can't remember what the macro is called right now. Hopefully Eric will remind us shortly.... ACK if the buffer overflow problem is solved Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On 06/14/2011 03:40 AM, Daniel P. Berrange wrote: Actually, INT_BUFSIZE_BOUND already includes space for the trailing NUL, so the +1 is not needed. Also, 'make syntax-check' complained about cppi indentation, and the fact that we've blacklisted sprintf (use snprintf instead). Plus you had some weird indentation. Here's what I squashed before pushing: diff --git i/src/nodeinfo.c w/src/nodeinfo.c index 235a68a..bdf8f8a 100644 --- i/src/nodeinfo.c +++ w/src/nodeinfo.c @@ -66,10 +66,10 @@ int linuxNodeInfoCPUPopulate(FILE *cpuinfo, virNodeInfoPtr nodeinfo, bool need_hyperthreads); -int linuxNodeGetCPUStats(FILE *procstat, - int cpuNum, - virCPUStatsPtr params, - int *nparams); +static int linuxNodeGetCPUStats(FILE *procstat, + int cpuNum, + virCPUStatsPtr params, + int *nparams); /* Return the positive decimal contents of the given * CPU_SYS_PATH/cpu%u/FILE, or -1 on error. If MISSING_OK and the @@ -384,8 +384,7 @@ int linuxNodeInfoCPUPopulate(FILE *cpuinfo, return 0; } -#define TICK_TO_NSEC (1000ull * 1000ull * 1000ull / sysconf(_SC_CLK_TCK)) -#define CPU_HEADER_LEN 8 +# define TICK_TO_NSEC (1000ull * 1000ull * 1000ull / sysconf(_SC_CLK_TCK)) int linuxNodeGetCPUStats(FILE *procstat, int cpuNum, @@ -396,7 +395,7 @@ int linuxNodeGetCPUStats(FILE *procstat, char line[1024]; unsigned long long usr, ni, sys, idle, iowait; unsigned long long irq, softirq, steal, guest, guest_nice; - char cpu_header[CPU_HEADER_LEN]; + char cpu_header[3 + INT_BUFSIZE_BOUND(cpuNum)]; if ((*nparams) == 0) { /* Current number of cpu stats supported by linux */ @@ -414,7 +413,7 @@ int linuxNodeGetCPUStats(FILE *procstat, if (cpuNum == VIR_CPU_STATS_ALL_CPUS) { strcpy(cpu_header, "cpu"); } else { - sprintf(cpu_header, "cpu%d", cpuNum); + snprintf(cpu_header, sizeof(cpu_header), "cpu%d", cpuNum); } while (fgets(line, sizeof(line), procstat) != NULL) { @@ -436,7 +435,7 @@ int linuxNodeGetCPUStats(FILE *procstat, switch (i) { case 0: /* fill kernel cpu time here */ - if (virStrcpyStatic(param->field, VIR_CPU_STATS_KERNEL)== NULL) { + if (virStrcpyStatic(param->field, VIR_CPU_STATS_KERNEL) == NULL) { nodeReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Field kernel cpu time too long for destination")); goto cleanup; @@ -528,22 +527,23 @@ int nodeGetCPUStats(virConnectPtr conn ATTRIBUTE_UNUSED, int cpuNum, virCPUStatsPtr params, int *nparams, - unsigned int flags ATTRIBUTE_UNUSED) + unsigned int flags) { + virCheckFlags(0, -1); #ifdef __linux__ { - int ret; - FILE *procstat = fopen(PROCSTAT_PATH, "r"); - if (!procstat) { - virReportSystemError(errno, - _("cannot open %s"), PROCSTAT_PATH); - return -1; - } - ret = linuxNodeGetCPUStats(procstat, cpuNum, params, nparams); - VIR_FORCE_FCLOSE(procstat); + int ret; + FILE *procstat = fopen(PROCSTAT_PATH, "r"); + if (!procstat) { + virReportSystemError(errno, + _("cannot open %s"), PROCSTAT_PATH); + return -1; + } + ret = linuxNodeGetCPUStats(procstat, cpuNum, params, nparams); + VIR_FORCE_FCLOSE(procstat); - return ret; + return ret; } #else nodeReportError(VIR_ERR_NO_SUPPORT, "%s", -- Eric Blake eblake(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

On Tue, Jun 07, 2011 at 10:03:36AM +0900, Minoru Usui wrote: Indentation went wrong here, but ACK if that is fixed Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

virNodeGetMemoryStats: Define internal driver API Signed-off-by: Minoru Usui <usui(a)mxm.nes.nec.co.jp&gt; --- src/driver.h | 9 +++++++++ 1 files changed, 9 insertions(+), 0 deletions(-) diff --git a/src/driver.h b/src/driver.h index a39411b..6239123 100644 --- a/src/driver.h +++ b/src/driver.h @@ -379,6 +379,14 @@ typedef int unsigned int flags); typedef int + (*virDrvNodeGetMemoryStats) + (virConnectPtr conn, + int cellNum, + virMemoryStatsPtr params, + int *nparams, + unsigned int flags); + +typedef int (*virDrvNodeGetCellsFreeMemory) (virConnectPtr conn, unsigned long long *freeMems, @@ -714,6 +722,7 @@ struct _virDriver { virDrvDomainMemoryPeek domainMemoryPeek; virDrvDomainGetBlockInfo domainGetBlockInfo; virDrvNodeGetCPUStats nodeGetCPUStats; + virDrvNodeGetMemoryStats nodeGetMemoryStats; virDrvNodeGetCellsFreeMemory nodeGetCellsFreeMemory; virDrvNodeGetFreeMemory nodeGetFreeMemory; virDrvDomainEventRegister domainEventRegister; -- 1.7.1

On Tue, Jun 07, 2011 at 10:04:54AM +0900, Minoru Usui wrote: Opps, I think this chunk should be in your first patch. Same comment as the earlier patch - use malloc() and fprintf(stderr) here. ACK, if those 2 points above are addressed Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

virNodeGetMemoryStats: Implement remote protocol Signed-off-by: Minoru Usui <usui(a)mxm.nes.nec.co.jp&gt; --- daemon/remote.c | 77 ++++++++++++++++++++++++++++++++++++++++++ src/remote/remote_driver.c | 65 +++++++++++++++++++++++++++++++++++ src/remote/remote_protocol.x | 22 +++++++++++- 3 files changed, 163 insertions(+), 1 deletions(-) diff --git a/daemon/remote.c b/daemon/remote.c index 5b947af..1525db3 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -1550,6 +1550,83 @@ no_memory: goto cleanup; } +static int +remoteDispatchNodeGetMemoryStats (struct qemud_server *server ATTRIBUTE_UNUSED, + struct qemud_client *client ATTRIBUTE_UNUSED, + virConnectPtr conn, + remote_message_header *hdr ATTRIBUTE_UNUSED, + remote_error *rerr, + remote_node_get_memory_stats_args *args, + remote_node_get_memory_stats_ret *ret) +{ + virMemoryStatsPtr params = NULL; + int i; + int cellNum = args->cellNum; + int nparams = args->nparams; + unsigned int flags; + int rv = -1; + + if (!conn) { + virNetError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open")); + goto cleanup; + } + + flags = args->flags; + + if (nparams > REMOTE_NODE_MEMORY_STATS_MAX) { + virNetError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams too large")); + goto cleanup; + } + if (VIR_ALLOC_N(params, nparams) < 0) { + virReportOOMError(); + goto cleanup; + } + + if (virNodeGetMemoryStats(conn, cellNum, params, &nparams, flags) < 0) + goto cleanup; + + /* In this case, we need to send back the number of parameters + * supported + */ + if (args->nparams == 0) { + ret->nparams = nparams; + goto success; + } + + /* Serialise the memory parameters. */ + ret->params.params_len = nparams; + if (VIR_ALLOC_N(ret->params.params_val, nparams) < 0) + goto no_memory; + + for (i = 0; i < nparams; ++i) { + /* remoteDispatchClientRequest will free this: */ + ret->params.params_val[i].field = strdup(params[i].field); + if (ret->params.params_val[i].field == NULL) + goto no_memory; + + ret->params.params_val[i].value = params[i].value; + } + +success: + rv = 0; + +cleanup: + if (rv < 0) { + remoteDispatchError(rerr); + if (ret->params.params_val) { + for (i = 0; i < nparams; i++) + VIR_FREE(ret->params.params_val[i].field); + VIR_FREE(ret->params.params_val); + } + } + VIR_FREE(params); + return rv; + +no_memory: + virReportOOMError(); + goto cleanup; +} + /*-------------------------------------------------------------*/ static int diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index c57c407..ef3f67d 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -1763,6 +1763,70 @@ done: } static int +remoteNodeGetMemoryStats (virConnectPtr conn, + int cellNum, + virMemoryStatsPtr params, int *nparams, + unsigned int flags) +{ + int rv = -1; + remote_node_get_memory_stats_args args; + remote_node_get_memory_stats_ret ret; + int i = -1; + struct private_data *priv = conn->privateData; + + remoteDriverLock(priv); + + args.nparams = *nparams; + args.cellNum = cellNum; + args.flags = flags; + + memset (&ret, 0, sizeof ret); + if (call (conn, priv, 0, REMOTE_PROC_NODE_GET_MEMORY_STATS, + (xdrproc_t) xdr_remote_node_get_memory_stats_args, (char *) &args, + (xdrproc_t) xdr_remote_node_get_memory_stats_ret, (char *) &ret) == -1) + goto done; + + /* Check the length of the returned list carefully. */ + if (ret.params.params_len > REMOTE_NODE_MEMORY_STATS_MAX || + ret.params.params_len > *nparams) { + remoteError(VIR_ERR_RPC, "%s", + _("remoteNodeGetMemoryStats: " + "returned number of stats exceeds limit")); + goto cleanup; + } + /* Handle the case when the caller does not know the number of stats + * and is asking for the number of stats supported + */ + if (*nparams == 0) { + *nparams = ret.nparams; + rv = 0; + goto cleanup; + } + + *nparams = ret.params.params_len; + + /* Deserialise the result. */ + for (i = 0; i < *nparams; ++i) { + if (virStrcpyStatic(params[i].field, ret.params.params_val[i].field) == NULL) { + remoteError(VIR_ERR_INTERNAL_ERROR, + _("Stats %s too big for destination"), + ret.params.params_val[i].field); + goto cleanup; + } + params[i].value = ret.params.params_val[i].value; + } + + rv = 0; + +cleanup: + xdr_free ((xdrproc_t) xdr_remote_node_get_memory_stats_ret, + (char *) &ret); +done: + remoteDriverUnlock(priv); + return rv; +} + +static int remoteNodeGetCellsFreeMemory(virConnectPtr conn, unsigned long long *freeMems, int startCell, @@ -6362,6 +6426,7 @@ static virDriver remote_driver = { .domainMemoryPeek = remoteDomainMemoryPeek, /* 0.4.2 */ .domainGetBlockInfo = remoteDomainGetBlockInfo, /* 0.8.1 */ .nodeGetCPUStats = remoteNodeGetCPUStats, /* 0.9.3 */ + .nodeGetMemoryStats = remoteNodeGetMemoryStats, /* 0.9.3 */ .nodeGetCellsFreeMemory = remoteNodeGetCellsFreeMemory, /* 0.3.3 */ .nodeGetFreeMemory = remoteNodeGetFreeMemory, /* 0.3.3 */ .domainEventRegister = remoteDomainEventRegister, /* 0.5.0 */ diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index 59a9d62..78b8c5f 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -137,6 +137,9 @@ const REMOTE_DOMAIN_MEMORY_PARAMETERS_MAX = 16; /* Upper limit on list of node cpu stats. */ const REMOTE_NODE_CPU_STATS_MAX = 16; +/* Upper limit on list of node memory stats. */ +const REMOTE_NODE_MEMORY_STATS_MAX = 16; + /* Upper limit on number of NUMA cells */ const REMOTE_NODE_MAX_CELLS = 1024; @@ -327,6 +330,11 @@ struct remote_node_get_cpu_stats { unsigned hyper value; }; +struct remote_node_get_memory_stats { + remote_nonnull_string field; + unsigned hyper value; +}; + /*----- Calls. -----*/ /* For each call we may have a 'remote_CALL_args' and 'remote_CALL_ret' @@ -422,6 +430,17 @@ struct remote_node_get_cpu_stats_ret { int nparams; }; +struct remote_node_get_memory_stats_args { + int nparams; + int cellNum; + unsigned int flags; +}; + +struct remote_node_get_memory_stats_ret { + remote_node_get_memory_stats params<REMOTE_NODE_MEMORY_STATS_MAX>; + int nparams; +}; + struct remote_node_get_cells_free_memory_args { int startCell; int maxcells; @@ -2317,7 +2336,8 @@ enum remote_procedure { REMOTE_PROC_INTERFACE_CHANGE_ROLLBACK = 222, /* autogen autogen */ REMOTE_PROC_DOMAIN_GET_SCHEDULER_PARAMETERS_FLAGS = 223, /* skipgen autogen */ REMOTE_PROC_DOMAIN_EVENT_CONTROL_ERROR = 224, /* skipgen skipgen */ - REMOTE_PROC_NODE_GET_CPU_STATS = 225 /* skipgen skipgen */ + REMOTE_PROC_NODE_GET_CPU_STATS = 225, /* skipgen skipgen */ + REMOTE_PROC_NODE_GET_MEMORY_STATS = 226 /* skipgen skipgen */ /* * Notice how the entries are grouped in sets of 10 ? -- 1.7.1

On 06/14/2011 03:18 AM, Daniel P. Berrange wrote: Another merge conflict, and again missing remote_protocol-structs changes. Pushed with this squashed in. diff --git i/src/remote_protocol-structs w/src/remote_protocol-structs index ea7bdd2..ef5f266 100644 --- i/src/remote_protocol-structs +++ w/src/remote_protocol-structs @@ -75,6 +75,10 @@ struct remote_node_get_cpu_stats { remote_nonnull_string field; uint64_t value; }; +struct remote_node_get_memory_stats { + remote_nonnull_string field; + uint64_t value; +}; struct remote_open_args { remote_string name; int flags; @@ -137,6 +141,18 @@ struct remote_node_get_cpu_stats_ret { } params; int nparams; }; +struct remote_node_get_memory_stats_args { + int nparams; + int cellNum; + u_int flags; +}; +struct remote_node_get_memory_stats_ret { + struct { + u_int params_len; + remote_node_get_memory_stats * params_val; + } params; + int nparams; +}; struct remote_node_get_cells_free_memory_args { int startCell; int maxcells; -- Eric Blake eblake(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

On Tue, Jun 07, 2011 at 10:09:15AM +0900, Minoru Usui wrote: ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

virNodeGetMemoryStats: Implement linux support Signed-off-by: Minoru Usui <usui(a)mxm.nes.nec.co.jp&gt; --- src/libvirt_private.syms | 1 + src/lxc/lxc_driver.c | 1 + src/nodeinfo.c | 161 ++++++++++++++++++++++++++++++++++++++++++++++ src/nodeinfo.h | 5 ++ src/qemu/qemu_driver.c | 1 + src/uml/uml_driver.c | 1 + 6 files changed, 170 insertions(+), 0 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a8c77f2..4685e81 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -732,6 +732,7 @@ virNodeDeviceObjUnlock; # nodeinfo.h nodeCapsInitNUMA; nodeGetCPUStats; +nodeGetMemoryStats; nodeGetCellsFreeMemory; nodeGetFreeMemory; nodeGetInfo; diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 3286154..daf93f2 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -2780,6 +2780,7 @@ static virDriver lxcDriver = { .domainSetSchedulerParametersFlags = lxcSetSchedulerParametersFlags, /* 0.9.2 */ .domainInterfaceStats = lxcDomainInterfaceStats, /* 0.7.3 */ .nodeGetCPUStats = nodeGetCPUStats, /* 0.9.3 */ + .nodeGetMemoryStats = nodeGetMemoryStats, /* 0.9.3 */ .nodeGetCellsFreeMemory = nodeGetCellsFreeMemory, /* 0.6.5 */ .nodeGetFreeMemory = nodeGetFreeMemory, /* 0.6.5 */ .domainEventRegister = lxcDomainEventRegister, /* 0.7.0 */ diff --git a/src/nodeinfo.c b/src/nodeinfo.c index 39afd0e..29afd1f 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -58,8 +58,12 @@ # define CPUINFO_PATH "/proc/cpuinfo" # define CPU_SYS_PATH "/sys/devices/system/cpu" # define PROCSTAT_PATH "/proc/stat" +# define MEMINFO_PATH "/proc/meminfo" +# define NODE_SYS_PATH "/sys/devices/system/node" # define LINUX_NB_CPU_STATS 4 +# define LINUX_NB_MEMORY_STATS_ALL 4 +# define LINUX_NB_MEMORY_STATS_CELL 2 /* NB, this is not static as we need to call it from the testsuite */ int linuxNodeInfoCPUPopulate(FILE *cpuinfo, @@ -70,6 +74,10 @@ int linuxNodeGetCPUStats(FILE *procstat, int cpuNum, virCPUStatsPtr params, int *nparams); +int linuxNodeGetMemoryStats(FILE *meminfo, + int cellNum, + virMemoryStatsPtr params, + int *nparams); /* Return the positive decimal contents of the given * CPU_SYS_PATH/cpu%u/FILE, or -1 on error. If MISSING_OK and the @@ -488,6 +496,112 @@ int linuxNodeGetCPUStats(FILE *procstat, cleanup: return ret; } + +int linuxNodeGetMemoryStats(FILE *meminfo, + int cellNum, + virMemoryStatsPtr params, + int *nparams) +{ + int ret = -1; + int i = 0, j = 0, k = 0; + int found = 0; + int nr_param; + char line[1024]; + char meminfo_hdr[VIR_MEMORY_STATS_FIELD_LENGTH]; + unsigned long val; + struct field_conv { + const char *meminfo_hdr; // meminfo header + const char *field; // MemoryStats field name + } field_conv[] = { + {"MemTotal:", VIR_MEMORY_STATS_TOTAL}, + {"MemFree:", VIR_MEMORY_STATS_FREE}, + {"Buffers:", VIR_MEMORY_STATS_BUFFERS}, + {"Cached:", VIR_MEMORY_STATS_CACHED}, + {NULL, NULL} + }; + + if (cellNum == VIR_MEMORY_STATS_ALL_CELLS) { + nr_param = LINUX_NB_MEMORY_STATS_ALL; + } else { + nr_param = LINUX_NB_MEMORY_STATS_CELL; + } + + if ((*nparams) == 0) { + /* Current number of memory stats supported by linux */ + *nparams = nr_param; + ret = 0; + goto cleanup; + } + + if ((*nparams) != nr_param) { + nodeReportError(VIR_ERR_INVALID_ARG, + "%s", _("Invalid stats count")); + goto cleanup; + } + + while (fgets(line, sizeof(line), meminfo) != NULL) { + char *buf = line; + + if (STRPREFIX(buf, "Node ")) { + /* + * /sys/devices/system/node/nodeX/meminfo format is below. + * So, skip prefix "Node XX ". + * + * Node 0 MemTotal: 8386980 kB + * Node 0 MemFree: 5300920 kB + * : + */ + char *p; + + p = buf; + for (i = 0; i < 2; i++) { + p = strchr(p, ' '); + if (p == NULL) { + nodeReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("no prefix found")); + goto cleanup; + } + p++; + } + buf = p; + } + + if (sscanf(buf, "%s %lu kB", meminfo_hdr, &val) < 2) { + continue; + } + + for (j = 0; field_conv[j].meminfo_hdr != NULL; j++) { + struct field_conv *convp = &field_conv[j]; + + if (STREQ(meminfo_hdr, convp->meminfo_hdr)) { + virMemoryStatsPtr param = &params[k++]; + + if (virStrcpyStatic(param->field, convp->field) == NULL) { + nodeReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("Field kernel memory too long for destination")); + goto cleanup; + } + param->value = val; + found++; + break; + } + } + if (found >= nr_param) { + break; + } + } + + if (found == 0) { + nodeReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("no available memory line found")); + goto cleanup; + } + + ret = 0; + +cleanup: + return ret; +} #endif int nodeGetInfo(virConnectPtr conn ATTRIBUTE_UNUSED, virNodeInfoPtr nodeinfo) { @@ -554,6 +668,53 @@ int nodeGetCPUStats(virConnectPtr conn ATTRIBUTE_UNUSED, #endif } +int nodeGetMemoryStats(virConnectPtr conn ATTRIBUTE_UNUSED, + int cellNum, + virMemoryStatsPtr params, + int *nparams, + unsigned int flags ATTRIBUTE_UNUSED) +{ +#ifdef __linux__ + { + int ret; + char meminfo_path[1024]; + FILE *meminfo; + + if (cellNum == VIR_MEMORY_STATS_ALL_CELLS) { + sprintf(meminfo_path, MEMINFO_PATH); + } else { + if (numa_available() < 0) { + nodeReportError(VIR_ERR_NO_SUPPORT, + "%s", _("NUMA not supported on this host")); + return -1; + } + + if (cellNum > numa_max_node()) { + nodeReportError(VIR_ERR_INVALID_ARG, "%s", _("Invalid cell number")); + return -1; + } + + sprintf(meminfo_path, "%s/node%d/meminfo", NODE_SYS_PATH, cellNum); + } + meminfo = fopen(meminfo_path, "r"); + + if (!meminfo) { + virReportSystemError(errno, + _("cannot open %s"), MEMINFO_PATH); + return -1; + } + ret = linuxNodeGetMemoryStats(meminfo, cellNum, params, nparams); + VIR_FORCE_FCLOSE(meminfo); + + return ret; + } +#else + nodeReportError(VIR_ERR_NO_SUPPORT, "%s", + _("node memory stats not implemented on this platform")); + return -1; +#endif +} + #if HAVE_NUMACTL # if LIBNUMA_API_VERSION <= 1 # define NUMA_MAX_N_CPUS 4096 diff --git a/src/nodeinfo.h b/src/nodeinfo.h index 361e3e5..1ebcdb1 100644 --- a/src/nodeinfo.h +++ b/src/nodeinfo.h @@ -35,6 +35,11 @@ int nodeGetCPUStats(virConnectPtr conn ATTRIBUTE_UNUSED, virCPUStatsPtr params, int *nparams, unsigned int flags ATTRIBUTE_UNUSED); +int nodeGetMemoryStats(virConnectPtr conn ATTRIBUTE_UNUSED, + int cellNum, + virMemoryStatsPtr params, + int *nparams, + unsigned int flags ATTRIBUTE_UNUSED); int nodeGetCellsFreeMemory(virConnectPtr conn, unsigned long long *freeMems, int startCell, diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 108a37f..ce93bf1 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8045,6 +8045,7 @@ static virDriver qemuDriver = { .domainMemoryPeek = qemudDomainMemoryPeek, /* 0.4.4 */ .domainGetBlockInfo = qemuDomainGetBlockInfo, /* 0.8.1 */ .nodeGetCPUStats = nodeGetCPUStats, /* 0.9.3 */ + .nodeGetMemoryStats = nodeGetMemoryStats, /* 0.9.3 */ .nodeGetCellsFreeMemory = nodeGetCellsFreeMemory, /* 0.4.4 */ .nodeGetFreeMemory = nodeGetFreeMemory, /* 0.4.4 */ .domainEventRegister = qemuDomainEventRegister, /* 0.5.0 */ diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index 471277d..80b51dd 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -2219,6 +2219,7 @@ static virDriver umlDriver = { .domainSetAutostart = umlDomainSetAutostart, /* 0.5.0 */ .domainBlockPeek = umlDomainBlockPeek, /* 0.5.0 */ .nodeGetCPUStats = nodeGetCPUStats, /* 0.9.3 */ + .nodeGetMemoryStats = nodeGetMemoryStats, /* 0.9.3 */ .nodeGetCellsFreeMemory = nodeGetCellsFreeMemory, /* 0.5.0 */ .nodeGetFreeMemory = nodeGetFreeMemory, /* 0.5.0 */ .isEncrypted = umlIsEncrypted, /* 0.7.3 */ -- 1.7.1

On 06/14/2011 03:21 AM, Daniel P. Berrange wrote: ACK with this squashed in (whitespace ignored, the real diff was a bit bigger but had more indentation changed), so I pushed: diff --git i/src/libvirt_private.syms w/src/libvirt_private.syms index 57243f1..7a05539 100644 --- i/src/libvirt_private.syms +++ w/src/libvirt_private.syms @@ -734,10 +734,10 @@ virNodeDeviceObjUnlock; # nodeinfo.h nodeCapsInitNUMA; nodeGetCPUStats; -nodeGetMemoryStats; nodeGetCellsFreeMemory; nodeGetFreeMemory; nodeGetInfo; +nodeGetMemoryStats; # nwfilter_conf.h diff --git i/src/nodeinfo.c w/src/nodeinfo.c index 809fc44..cb2f805 100644 --- i/src/nodeinfo.c +++ w/src/nodeinfo.c @@ -559,9 +563,8 @@ int linuxNodeGetMemoryStats(FILE *meminfo, buf = p; } - if (sscanf(buf, "%s %lu kB", meminfo_hdr, &val) < 2) { + if (sscanf(buf, "%s %lu kB", meminfo_hdr, &val) < 2) continue; - } for (j = 0; field_conv[j].meminfo_hdr != NULL; j++) { struct field_conv *convp = &field_conv[j]; @@ -579,10 +582,9 @@ int linuxNodeGetMemoryStats(FILE *meminfo, break; } } - if (found >= nr_param) { + if (found >= nr_param) break; } - } if (found == 0) { nodeReportError(VIR_ERR_INTERNAL_ERROR, @@ -666,16 +668,22 @@ int nodeGetMemoryStats(virConnectPtr conn ATTRIBUTE_UNUSED, int cellNum, virMemoryStatsPtr params, int *nparams, - unsigned int flags ATTRIBUTE_UNUSED) + unsigned int flags) { + virCheckFlags(0, -1); + #ifdef __linux__ { int ret; - char meminfo_path[1024]; + char *meminfo_path = NULL; FILE *meminfo; if (cellNum == VIR_MEMORY_STATS_ALL_CELLS) { - sprintf(meminfo_path, MEMINFO_PATH); + meminfo_path = strdup(MEMINFO_PATH); + if (!meminfo_path) { + virReportOOMError(); + return -1; + } } else { if (numa_available() < 0) { nodeReportError(VIR_ERR_NO_SUPPORT, @@ -684,21 +692,28 @@ int nodeGetMemoryStats(virConnectPtr conn ATTRIBUTE_UNUSED, } if (cellNum > numa_max_node()) { - nodeReportError(VIR_ERR_INVALID_ARG, "%s", _("Invalid cell number")); + nodeReportError(VIR_ERR_INVALID_ARG, "%s", + _("Invalid cell number")); return -1; } - sprintf(meminfo_path, "%s/node%d/meminfo", NODE_SYS_PATH, cellNum); + if (virAsprintf(&meminfo_path, "%s/node%d/meminfo", + NODE_SYS_PATH, cellNum) < 0) { + virReportOOMError(); + return -1; + } } meminfo = fopen(meminfo_path, "r"); if (!meminfo) { virReportSystemError(errno, - _("cannot open %s"), MEMINFO_PATH); + _("cannot open %s"), meminfo_path); + VIR_FREE(meminfo_path); return -1; } ret = linuxNodeGetMemoryStats(meminfo, cellNum, params, nparams); VIR_FORCE_FCLOSE(meminfo); + VIR_FREE(meminfo_path); return ret; } -- Eric Blake eblake(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org