[libvirt] [PATCH v2 2/3] cpu: Implement guestData and update for PPC

[Li Zhang]

On 2013年09月06日 19:32, John Ferlan wrote:
> On 09/03/2013 02:28 AM, Li Zhang wrote:
>> From: Li Zhang <zhlcindy at linux.vnet.ibm.com>
>>
>> On Power platform, Power7+ can support Power7 guest.
>> It needs to define XML configuration to specify guest's CPU model.
>>
>> For exmaple:
>>    <cpu match='exact'>
>>      <model>POWER7_v2.1</model>
>>      <vendor>IBM</vendor>
>>    </cpu>
>>
>> Signed-off-by: Li Zhang <zhlcindy at linux.vnet.ibm.com>
>> ---
>>   src/cpu/cpu_powerpc.c | 178 ++++++++++++++++++++++++++++++++++++++++++++++++--
>>   1 file changed, 174 insertions(+), 4 deletions(-)
>>
> ...<snip>...
>
> There were Coverity RESOURCE_LEAK's detected in this code - so while
> looking at those I looked at the rest of this function and had a few
> comments...
>
>> +
>> +static virCPUCompareResult
>> +ppcCompute(virCPUDefPtr host,
>> +             const virCPUDefPtr cpu,
>> +             virCPUDataPtr *guestData,
>> +             char **message)
>> +
>> +{
>> +    struct ppc_map *map = NULL;
>> +    struct ppc_model *host_model = NULL;
>> +    struct ppc_model *guest_model = NULL;
>> +
>> +    int ret = 0;
> NOTE: To be consistent should this be 'VIR_CPU_COMPARE_INCOMPATIBLE'
> (e.g. 0)?   or better yet 'VIR_CPU_COMPARE_ERROR' (-1).
OK, I will change it.


>
>> +    virArch arch;
>> +    size_t i;
>> +
>> +    if (cpu->arch != VIR_ARCH_NONE) {
>> +        bool found = false;
>> +
>> +        for (i = 0; i < ARRAY_CARDINALITY(archs); i++) {
>> +            if (archs[i] == cpu->arch) {
>> +                found = true;
>> +                break;
>> +            }
>> +        }
>> +
>> +        if (!found) {
>> +            VIR_DEBUG("CPU arch %s does not match host arch",
>> +                      virArchToString(cpu->arch));
>> +            if (message &&
>> +                virAsprintf(message,
>> +                            _("CPU arch %s does not match host arch"),
>> +                            virArchToString(cpu->arch)) < 0)
>> +                goto error;
>> +            return VIR_CPU_COMPARE_INCOMPATIBLE;
> Why on a "message" do you go to error which changes the return value to
> ERROR, while if message isn't defined you return INCOMPATIBLE?  Seems
> you'd want to set "ret" to INCOMPATIBLE, then goto out (or cleanup).
> Does the caller differentiate ERROR and INCOMPATIBLE?  Does it do
> something different if it passed a message, but you got different return
> values?
>

On a message, if virAsprintf return value < 0, it will goto error which 
means some error happens.

This place return INCOMPATIBLE only if (!found) not if (!message).
It doesn't belong to the block of 'if(message & ...)'.

This means that we can't find out any archs we support matched.
So we need to report INCOMPATIBLE.

We don't alloc any memory here, map, host_model and guest_model are all 
NULL.

>> +        }
>> +        arch = cpu->arch;
>> +    } else {
>> +        arch = host->arch;
>> +    }
>> +
>> +    if (cpu->vendor &&
>> +        (!host->vendor || STRNEQ(cpu->vendor, host->vendor))) {
>> +        VIR_DEBUG("host CPU vendor does not match required CPU vendor %s",
>> +                  cpu->vendor);
>> +        if (message &&
>> +            virAsprintf(message,
>> +                        _("host CPU vendor does not match required "
>> +                        "CPU vendor %s"),
>> +                        cpu->vendor) < 0)
>> +            goto error;
>> +        return VIR_CPU_COMPARE_INCOMPATIBLE;
> Same comment as above
>
>> +    }
>> +
>> +    if (!(map = ppcLoadMap()) ||
>> +        !(host_model = ppcModelFromCPU(host, map)) ||
>> +        !(guest_model = ppcModelFromCPU(cpu, map)))
>> +        goto error;
> If you initialize ret to COMPARE_ERROR, then it's a goto out (or
> cleanup)...  Also the only way these are ever used is if (guestData !=
> NULL), thus why not put them inside the below if condition?

OK, let me clean these goto clause.

>> +
>> +    if (guestData != NULL) {
>> +        if (cpu->type == VIR_CPU_TYPE_GUEST &&
>> +            cpu->match == VIR_CPU_MATCH_STRICT &&
>> +            STRNEQ(guest_model->name, host_model->name)) {
>> +            VIR_DEBUG("host CPU model does not match required CPU model %s",
>> +                      guest_model->name);
>> +            if (message &&
>> +                virAsprintf(message,
>> +                            _("host CPU model does not match required "
>> +                            "CPU model %s"),
>> +                            guest_model->name) < 0)
>> +                goto error;
>> +            return VIR_CPU_COMPARE_INCOMPATIBLE;
> Coverity found a RESOURCE_LEAK here on 'host_model' and 'guest_model'.
> Coverity missed that 'map' is also leaked...

Ah, right. There is memory leak here. Will clean this.

>
> And this would have the same comments as above regarding the goto/return
>
>> +        }
>> +
>> +        if (!(*guestData = ppcMakeCPUData(arch, &guest_model->data)))
>> +            goto error;
> Initializing ret to ERROR would mean this would be a goto out (or cleanup).
>
>> +    }
>> +
>> +    ret = VIR_CPU_COMPARE_IDENTICAL;
>> +
>> +out:
> I think by convention this is usually "cleanup:"
>
>
> John
>
>> +    ppcMapFree(map);
>> +    ppcModelFree(host_model);
>> +    ppcModelFree(guest_model);
>> +    return ret;
>> +
>> +error:
>> +    ret = VIR_CPU_COMPARE_ERROR;
>> +    goto out;
>> +}
>> +
>