[libvirt] [PATCH] cpu: break out when a right cpuCandidate found

Daniel P. Berrange berrange at redhat.com
Thu Feb 13 10:57:10 UTC 2014


On Thu, Feb 13, 2014 at 10:42:01AM +0000, Wangyufei (James) wrote:
> 
> 
> > -----Original Message-----
> > From: Martin Kletzander [mailto:mkletzan at redhat.com]
> > Sent: Thursday, February 13, 2014 6:08 PM
> > To: Wangyufei (James)
> > Cc: libvir-list at 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 at 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.

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 :|




More information about the libvir-list mailing list