[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v2] Add flag to BaselineCPU API to return detailed CPU features



Sigh.  Rebase, I've heard of that, maybe I should remember to do it :-(

The failures are related to the rebasing, I'm working on that and
I'll incorporate your other fixes (with a couple of comments in line).

Only open issue is the flag name, I have no problems with
_EXPAND_FEATURES, unless there's an objection I'll do that.

On Tue, Jul 30, 2013 at 03:59:38PM -0600, Eric Blake wrote:
...
> 
> > +++ b/src/cpu/cpu.h
> > @@ -49,7 +49,8 @@ typedef int
> >                       const union cpuData *data,
> >                       const char **models,
> >                       unsigned int nmodels,
> > -                     const char *preferred);
> > +                     const char *preferred,
> > +                     unsigned int flags);
> >  
> >  typedef int
> >  (*cpuArchEncode)    (const virCPUDefPtr cpu,
> > @@ -76,7 +77,8 @@ typedef virCPUDefPtr
> >  (*cpuArchBaseline)  (virCPUDefPtr *cpus,
> >                       unsigned int ncpus,
> >                       const char **models,
> > -                     unsigned int nmodels);
> > +                     unsigned int nmodels,
> > +                     unsigned int /* flags */);
> 
> No need to comment out the parameter name (multiple instances */.

This was to avoid compile failures with unused parameters but
this becomes moot when I put in the code to explicitly check
that flag.

...

> > @@ -1383,6 +1416,9 @@ x86Decode(virCPUDefPtr cpu,
> >          goto out;
> >      }
> >  
> > +    if (flags & VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURE)
> > +        if (x86AddFeatures(cpuModel, map) < 0)
> > +            goto out;
> 
> You could write this with fewer nested if:
> 
> if ((flags & VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURE) &&
>     x86AddFeatures(cpuModel, map) < 0)
>     goto out;

I actually prefer the nested ifs over the logical expression (to
me that's more obvious) but I don't mind changing if that's your
preferred coding style.

...
> > @@ -6049,7 +6057,8 @@ cmdCPUBaseline(vshControl *ctl, const vshCmd *cmd)
> >          list[i] = vshStrdup(ctl, (const char *)xmlBufferContent(xml_buf));
> >      }
> >  
> > -    result = virConnectBaselineCPU(ctl->conn, list, count, 0);
> > +    result = virConnectBaselineCPU(ctl->conn, list, count, flags);
> > +vshPrint(ctl, "result - %p\n", result);
> 
> Leftover debugging?

Yep, shoot me, I should know better.  Sorry about that.

-- 
Don Dugger
"Censeo Toto nos in Kansa esse decisse." - D. Gale
n0ano n0ano com
Ph: 303/443-3786


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]