<div dir="ltr"><div class="gmail_extra">On Fri, Sep 6, 2013 at 6:32 AM, John Ferlan <span dir="ltr"><<a href="mailto:jferlan@redhat.com" target="_blank">jferlan@redhat.com</a>></span> wrote:<br><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">On 09/03/2013 02:28 AM, Li Zhang wrote:<br>
> From: Li Zhang <<a href="mailto:zhlcindy@linux.vnet.ibm.com">zhlcindy@linux.vnet.ibm.com</a>><br>
><br>
> On Power platform, Power7+ can support Power7 guest.<br>
> It needs to define XML configuration to specify guest's CPU model.<br>
><br>
> For exmaple:<br>
> <cpu match='exact'><br>
> <model>POWER7_v2.1</model><br>
> <vendor>IBM</vendor><br>
> </cpu><br>
><br>
> Signed-off-by: Li Zhang <<a href="mailto:zhlcindy@linux.vnet.ibm.com">zhlcindy@linux.vnet.ibm.com</a>><br>
> ---<br>
> src/cpu/cpu_powerpc.c | 178 ++++++++++++++++++++++++++++++++++++++++++++++++--<br>
> 1 file changed, 174 insertions(+), 4 deletions(-)<br>
><br>
<br>
</div>...<snip>...<br>
<br>
There were Coverity RESOURCE_LEAK's detected in this code - so while<br>
looking at those I looked at the rest of this function and had a few<br>
comments...<br>
<div class="im"><br>
> +<br>
> +static virCPUCompareResult<br>
> +ppcCompute(virCPUDefPtr host,<br>
> + const virCPUDefPtr cpu,<br>
> + virCPUDataPtr *guestData,<br>
> + char **message)<br>
> +<br>
> +{<br>
> + struct ppc_map *map = NULL;<br>
> + struct ppc_model *host_model = NULL;<br>
> + struct ppc_model *guest_model = NULL;<br>
> +<br>
> + int ret = 0;<br>
<br>
</div>NOTE: To be consistent should this be 'VIR_CPU_COMPARE_INCOMPATIBLE'<br>
(e.g. 0)? or better yet 'VIR_CPU_COMPARE_ERROR' (-1).<br></blockquote><div><br></div><div style>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.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im"><br>
> + virArch arch;<br>
> + size_t i;<br>
> +<br>
> + if (cpu->arch != VIR_ARCH_NONE) {<br>
> + bool found = false;<br>
> +<br>
> + for (i = 0; i < ARRAY_CARDINALITY(archs); i++) {<br>
> + if (archs[i] == cpu->arch) {<br>
> + found = true;<br>
> + break;<br>
> + }<br>
> + }<br>
> +<br>
> + if (!found) {<br>
> + VIR_DEBUG("CPU arch %s does not match host arch",<br>
> + virArchToString(cpu->arch));<br>
> + if (message &&<br>
> + virAsprintf(message,<br>
> + _("CPU arch %s does not match host arch"),<br>
> + virArchToString(cpu->arch)) < 0)<br>
> + goto error;<br>
> + return VIR_CPU_COMPARE_INCOMPATIBLE;<br>
<br>
</div>Why on a "message" do you go to error which changes the return value to<br>
ERROR, while if message isn't defined you return INCOMPATIBLE? Seems<br>
you'd want to set "ret" to INCOMPATIBLE, then goto out (or cleanup).<br>
Does the caller differentiate ERROR and INCOMPATIBLE? Does it do<br>
something different if it passed a message, but you got different return<br>
values?<br></blockquote><div><br></div><div style>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. </div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im"><br>
<br>
> + }<br>
> + arch = cpu->arch;<br>
> + } else {<br>
> + arch = host->arch;<br>
> + }<br>
> +<br>
> + if (cpu->vendor &&<br>
> + (!host->vendor || STRNEQ(cpu->vendor, host->vendor))) {<br>
> + VIR_DEBUG("host CPU vendor does not match required CPU vendor %s",<br>
> + cpu->vendor);<br>
> + if (message &&<br>
> + virAsprintf(message,<br>
> + _("host CPU vendor does not match required "<br>
> + "CPU vendor %s"),<br>
> + cpu->vendor) < 0)<br>
> + goto error;<br>
> + return VIR_CPU_COMPARE_INCOMPATIBLE;<br>
<br>
</div>Same comment as above<br></blockquote><div><br></div><div style>Same for me as well. Comes from x86 again.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im"><br>
> + }<br>
> +<br>
> + if (!(map = ppcLoadMap()) ||<br>
> + !(host_model = ppcModelFromCPU(host, map)) ||<br>
> + !(guest_model = ppcModelFromCPU(cpu, map)))<br>
> + goto error;<br>
<br>
</div>If you initialize ret to COMPARE_ERROR, then it's a goto out (or<br>
cleanup)... Also the only way these are ever used is if (guestData !=<br>
NULL), thus why not put them inside the below if condition?<br>
<div class="im"><br>
> +<br>
> + if (guestData != NULL) {<br>
> + if (cpu->type == VIR_CPU_TYPE_GUEST &&<br>
> + cpu->match == VIR_CPU_MATCH_STRICT &&<br>
> + STRNEQ(guest_model->name, host_model->name)) {<br>
> + VIR_DEBUG("host CPU model does not match required CPU model %s",<br>
> + guest_model->name);<br>
> + if (message &&<br>
> + virAsprintf(message,<br>
> + _("host CPU model does not match required "<br>
> + "CPU model %s"),<br>
> + guest_model->name) < 0)<br>
> + goto error;<br>
> + return VIR_CPU_COMPARE_INCOMPATIBLE;<br>
<br>
</div>Coverity found a RESOURCE_LEAK here on 'host_model' and 'guest_model'.<br>
Coverity missed that 'map' is also leaked...<br>
<br>
And this would have the same comments as above regarding the goto/return<br>
<div class="im"><br>
> + }<br>
> +<br>
> + if (!(*guestData = ppcMakeCPUData(arch, &guest_model->data)))<br>
> + goto error;<br>
<br>
</div>Initializing ret to ERROR would mean this would be a goto out (or cleanup).<br>
<div class="im"><br>
> + }<br>
> +<br>
> + ret = VIR_CPU_COMPARE_IDENTICAL;<br>
> +<br>
> +out:<br>
<br>
</div>I think by convention this is usually "cleanup:"<br>
<span class="HOEnZb"><font color="#888888"><br>
<br>
John<br>
</font></span><div class="im HOEnZb"><br>
> + ppcMapFree(map);<br>
> + ppcModelFree(host_model);<br>
> + ppcModelFree(guest_model);<br>
> + return ret;<br>
> +<br>
> +error:<br>
> + ret = VIR_CPU_COMPARE_ERROR;<br>
> + goto out;<br>
> +}<br>
> +<br>
<br></div></blockquote><div><br></div><div style>I agree with all of John's reviews here as well.</div></div><div><br></div>-- <br>Doug Goldstein
</div></div>