On Thu, Feb 13, 2014 at 10:42:01AM +0000, Wangyufei (James) wrote: > > > > -----Original Message----- > > From: Martin Kletzander [mailto:mkletzan@xxxxxxxxxx] > > Sent: Thursday, February 13, 2014 6:08 PM > > To: Wangyufei (James) > > Cc: libvir-list@xxxxxxxxxx; Wangrui (K); Zhaoyanbin (A) > > Subject: Re: [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@xxxxxxxxxx> > > > 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. I think I'd like to see a test case which demonstrates the flawed behaviour and so proves the fix works. We already have a file tests/cputest.c in which to put such test cases. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list