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

[Doug Goldstein <cardoe@xxxxxxxxxx>]

On Fri, Sep 6, 2013 at 6:32 AM, John Ferlan <jferlan@xxxxxxxxxx> wrote:

On 09/03/2013 02:28 AM, Li Zhang wrote:
> From: Li Zhang <zhlcindy@xxxxxxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxxxxx>
> ---
>  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).

I agree here. It should likely be VIR_CPU_COMPARE_ERROR as a good initializer. On top of that using "int" is wrong, the correct type is virCPUCompareResult.

 

> +    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?

This is something copied and pasted from x86Compute() from the x86 side. Only x86 and PPC now define this function and both use the same code. If its always going to be the same behavior I suggest we life it up into cpu.c in cpuGuestData() and improve the semantics as you (John) noted. 

> +        }
> +        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

Same for me as well. Comes from x86 again.

 

> +    }
> +
> +    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?

> +
> +    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...

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;
> +}
> +

I agree with all of John's reviews here as well.

--
Doug Goldstein

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list