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

On 09/03/2013 02:28 AM, Li Zhang wrote: > From: Li Zhang <zhlcindy(a)linux.vnet.ibm.com&gt; > > 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&gt; > --- > 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).

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

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