5 p.m.
On 06/14/2011 03:40 AM, Daniel P. Berrange wrote:
On Tue, Jun 14, 2011 at 10:15:36AM +0100, Daniel P. Berrange wrote:
> On Tue, Jun 07, 2011 at 10:02:55AM +0900, Minoru Usui wrote:
>> virNodeGetCPUStats: Implement linux support
>>
>> Signed-off-by: Minoru Usui <usui(a)mxm.nes.nec.co.jp>
>> +#define TICK_TO_NSEC (1000ull * 1000ull * 1000ull / sysconf(_SC_CLK_TCK))
>> +#define CPU_HEADER_LEN 8
>> +
>> +int linuxNodeGetCPUStats(FILE *procstat,
>> + int cpuNum,
>> + virCPUStatsPtr params,
>> + int *nparams)
>> +{
>> + int ret = -1;
>> + 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];
>> + } else {
>> + sprintf(cpu_header, "cpu%d", cpuNum);
>> + }
>
> 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....
Jim Meyering reminded me that it is INT_BUFSIZE_BOUND. So eg you
want todo
char cpu_header[3 + INT_BUFSIZE_BOUND(cpuNum) + 1];
to allow enough space for 'cpu' + any cpuNum value formatted as
a string + the trailing NULL.
Actually, INT_BUFSIZE_BOUND already includes space for the trailing NUL,
so the +1 is not needed.
ACK if the buffer overflow problem is solved
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