10:19 p.m.
On 2013年09月06日 19:32, John Ferlan wrote:
On 09/03/2013 02:28 AM, Li Zhang wrote:
> From: Li Zhang <zhlcindy(a)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(a)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;
> +}
> +