[libvirt] [PATCH v2] fix error when parsing ppc64 models on x86 host
Stefan Berger
stefanb at linux.vnet.ibm.com
Fri Dec 9 17:19:51 UTC 2011
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.
More information about the libvir-list
mailing list