[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