10:45 a.m.
When parsing ppc64 models on an x86 host an out-of-memory error message
is displayed due
to it checking for retcpus being NULL. Fix this by removing the check
whether retcpus is NULL
since we will realloc into this variable.
Also in the X86 model parser display the OOM error at the location where
it happens.
---
v2: Fixing leak of 'cpus'
---
src/qemu/qemu_capabilities.c | 37 ++++++++++++++++++++-----------------
1 file changed, 20 insertions(+), 17 deletions(-)
Index: libvirt-iterator/src/qemu/qemu_capabilities.c
===================================================================
--- libvirt-iterator.orig/src/qemu/qemu_capabilities.c
+++ libvirt-iterator/src/qemu/qemu_capabilities.c
@@ -443,8 +443,10 @@ qemuCapsParseX86Models(const char *outpu
if (retcpus) {
unsigned int len;
- if (VIR_REALLOC_N(cpus, count + 1) < 0)
+ if (VIR_REALLOC_N(cpus, count + 1) < 0) {
+ virReportOOMError();
goto error;
+ }
if (next)
len = next - p - 1;
@@ -456,8 +458,10 @@ qemuCapsParseX86Models(const char *outpu
len -= 2;
}
- if (!(cpus[count] = strndup(p, len)))
+ if (!(cpus[count] = strndup(p, len))) {
+ virReportOOMError();
goto error;
+ }
}
count++;
} while ((p = next));
@@ -491,12 +495,7 @@ qemuCapsParsePPCModels(const char *outpu
const char *next;
unsigned int count = 0;
const char **cpus = NULL;
- int i;
-
- if (!retcpus) {
- VIR_DEBUG("No retcpus specified");
- return -1;
- }
+ int i, ret;
do {
const char *t;
@@ -523,32 +522,38 @@ qemuCapsParsePPCModels(const char *outpu
if (retcpus) {
unsigned int len;
- if (VIR_REALLOC_N(cpus, count + 1) < 0)
+ if (VIR_REALLOC_N(cpus, count + 1) < 0) {
virReportOOMError();
+ ret = -1;
goto error;
+ }
len = t - p - 1;
- if (!(cpus[count] = strndup(p, len)))
+ if (!(cpus[count] = strndup(p, len))) {
virReportOOMError();
+ ret = -1;
goto error;
+ }
}
count++;
} while ((p = next));
if (retcount)
*retcount = count;
- if (retcpus)
+ if (retcpus) {
*retcpus = cpus;
- return 0;
+ cpus = NULL;
+ }
+ ret = 0;
error:
if (cpus) {
for (i = 0; i < count; i++)
VIR_FREE(cpus[i]);
+ VIR_FREE(cpus);
}
- VIR_FREE(cpus);
- return -1;
+ return ret;
}
int
@@ -587,10 +592,8 @@ qemuCapsProbeCPUModels(const char *qemu,
if (virCommandRun(cmd, NULL) < 0)
goto cleanup;
- if (parse(output, count, cpus) < 0) {
- virReportOOMError();
+ if (parse(output, count, cpus) < 0)
goto cleanup;
- }
ret = 0;
11:01 a.m.
On 12/09/2011 09:45 AM, Stefan Berger wrote:
When parsing ppc64 models on an x86 host an out-of-memory error
message
is displayed due
to it checking for retcpus being NULL. Fix this by removing the check
whether retcpus is NULL
since we will realloc into this variable.
Also in the X86 model parser display the OOM error at the location where
it happens.
---
v2: Fixing leak of 'cpus'
+ int i, ret;
Uninitialized,
do {
const char *t;
if ((next = strchr(p, '\n')))
next++;
if (!STRPREFIX(p, "PowerPC "))
continue;
and you can set p = NULL and get to the continue on the first iteration,
@@ -523,32 +522,38 @@ qemuCapsParsePPCModels(const char *outpu
if (retcpus) {
unsigned int len;
- if (VIR_REALLOC_N(cpus, count + 1) < 0)
+ if (VIR_REALLOC_N(cpus, count + 1) < 0) {
virReportOOMError();
+ ret = -1;
goto error;
+ }
len = t - p - 1;
- if (!(cpus[count] = strndup(p, len)))
+ if (!(cpus[count] = strndup(p, len))) {
virReportOOMError();
+ ret = -1;
goto error;
+ }
}
count++;
} while ((p = next));
and then break out of the loop, with ret still unassigned,
if (retcount)
*retcount = count;
- if (retcpus)
+ if (retcpus) {
*retcpus = cpus;
- return 0;
+ cpus = NULL;
+ }
+ ret = 0;
but at least you then assign it here, so it is never used unassigned.
It took me a while to follow this logic, but it is correct.
On the other hand, if you initialize ret to -1 where it is declared,
then you don't have to assign it at both virReportOOMError() points, and
you don't have to trace through the entire flow to ensure that you never
reach error: with it unassigned.
error:
Also, it might be worth tweaking this to cleanup: instead of error:, now
that we also execute it on success paths.
if (cpus) {
for (i = 0; i < count; i++)
VIR_FREE(cpus[i]);
+ VIR_FREE(cpus);
}
- VIR_FREE(cpus);
- return -1;
+ return ret;
}
int
ACK - what you have is correct, although you might want to squash in my
nits before pushing. No v3 necessary.
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
11:19 a.m.
On 12/09/2011 12:01 PM, Eric Blake wrote:
On 12/09/2011 09:45 AM, Stefan Berger wrote:
> When parsing ppc64 models on an x86 host an out-of-memory error message
> is displayed due
> to it checking for retcpus being NULL. Fix this by removing the check
> whether retcpus is NULL
> since we will realloc into this variable.
> Also in the X86 model parser display the OOM error at the location where
> it happens.
>
> ---
>
> v2: Fixing leak of 'cpus'
> + int i, ret;
Uninitialized,
> do {
> const char *t;
if ((next = strchr(p, '\n')))
next++;
if (!STRPREFIX(p, "PowerPC "))
continue;
and you can set p = NULL and get to the continue on the first iteration,
> @@ -523,32 +522,38 @@ qemuCapsParsePPCModels(const char *outpu
> if (retcpus) {
> unsigned int len;
>
> - if (VIR_REALLOC_N(cpus, count + 1)< 0)
> + if (VIR_REALLOC_N(cpus, count + 1)< 0) {
> virReportOOMError();
> + ret = -1;
> goto error;
> + }
>
> len = t - p - 1;
>
> - if (!(cpus[count] = strndup(p, len)))
> + if (!(cpus[count] = strndup(p, len))) {
> virReportOOMError();
> + ret = -1;
> goto error;
> + }
> }
> count++;
> } while ((p = next));
and then break out of the loop, with ret still unassigned,
> if (retcount)
> *retcount = count;
> - if (retcpus)
> + if (retcpus) {
> *retcpus = cpus;
> - return 0;
> + cpus = NULL;
> + }
> + ret = 0;
but at least you then assign it here, so it is never used unassigned.
It took me a while to follow this logic, but it is correct.
On the other hand, if you initialize ret to -1 where it is declared,
then you don't have to assign it at both virReportOOMError() points, and
you don't have to trace through the entire flow to ensure that you never
reach error: with it unassigned.
> error:
Also, it might be worth tweaking this to cleanup: instead of error:, now
that we also execute it on success paths.
> if (cpus) {
> for (i = 0; i< count; i++)
> VIR_FREE(cpus[i]);
> + VIR_FREE(cpus);
> }
> - VIR_FREE(cpus);
> - return -1;
> + return ret;
> }
>
> int
ACK - what you have is correct, although you might want to squash in my
nits before pushing. No v3 necessary.
Applied the nits. Pushed. Thanks.
4732
days inactive
4732
days old
2 comments
2 participants
participants (2)
-
Eric Blake -
Stefan Berger