Re: [libvirt] [PATCH] cpu: break out when a right cpuCandidate found
-----Original Message-----
From: Martin Kletzander [mailto:mkletzan@redhat.com]
Sent: Thursday, February 13, 2014 6:08 PM
To: Wangyufei (James)
Cc: libvir-list(a)redhat.com; Wangrui (K); Zhaoyanbin (A)
Subject: Re: [libvirt] [PATCH] cpu: break out when a right cpuCandidate found
On Thu, Feb 13, 2014 at 07:44:20AM +0000, Wangyufei (James) wrote:
> >From 8123c5d64f940fa0fb0de32fc5e68035980b6b01 Mon Sep 17
00:00:00 2001
> From: WangYufei <james.wangyufei(a)huawei.com>
> Date: Thu, 13 Feb 2014 07:17:11 +0000
> Subject: [PATCH] cpu: break out when a right cpuCandidate found
>
> In function x86Decode there's a code segment in while cycle like this:
> if (cpuModel == NULL
> || cpuModel->nfeatures > cpuCandidate->nfeatures) {
> virCPUDefFree(cpuModel);
> cpuModel = cpuCandidate;
> cpuData = candidate->data;
> } else {
> virCPUDefFree(cpuCandidate);
> }
> when it finds the right cpuCandidate, it doesn't break out the cycle, but
continues
> run in it, and cpuModel will never get a new value, it's meaningless. It
should
> break out when a right cpuCndidate found.
>
Inside this condition, the code doesn't always choose the perfect
candidate. You don't consider a situation when the cycle continues
and the next candidate model is the preferred one, thus satisfies
previous condition, which looks like this:
if (preferred && STREQ(cpuCandidate->model, preferred)) {
virCPUDefFree(cpuModel);
cpuModel = cpuCandidate;
cpuData = candidate->data;
break;
}
Where the "perfect" cpuModel is found, used and the condition breaks
(appropriately this time). But I could also misunderstood the code.
Well, thank you for your reply. I have seen preferred, but there's no where to
modify the value of preferred in the cycle.
So the preferred is a fixed value. In this case, I can make it better to mdify the patch
like this:
--- a/src/cpu/cpu_x86.c
+++ b/src/cpu/cpu_x86.c
@@ -1558,6 +1558,7 @@ x86Decode(virCPUDefPtr cpu,
virCPUDefFree(cpuModel);
cpuModel = cpuCandidate;
cpuData = candidate->data;
+ if (!preferred)
+ break;
} else {
virCPUDefFree(cpuCandidate);
}
In my situation：
virQEMUCapsInitCPU ->cpuDecode ->x86Decode
if (!(data = cpuNodeData(arch))
|| cpuDecode(cpu, data, NULL, 0, NULL) < 0)
goto cleanup;
preferred is always NULL. So we can do it better.
Martin