[libvirt] [PATCH 35/41] cpu: Rework cpuUpdate

[Jiri Denemark]

On Tue, Aug 30, 2016 at 17:53:56 -0400, John Ferlan wrote:
> 
> 
> On 08/12/2016 09:33 AM, Jiri Denemark wrote:
> > The reworked API is now called virCPUUpdate and it should change the
> > provided CPU definition into a one which can be consumed by QEMU command
> 
> s/by/by the/

Fixed.

...
> >  /**
> > - * cpuUpdate:
> > + * virCPUUpdate:
> 
> In a way I'd expect a "virCPUUpdate" API to go in src/util/...
> somewhere. Not a problem to keep it here, but I guess when I see vir
> prefixed functions I think src/util/...

Not really, I think the vir prefix is just used when someone refactors
an old code. AFAIK the goal (not enforced in any way) was to eventually
use the prefix for all functions. Anyway, see virQEMUCaps* is good
example of such functions/types that are not in src/util.

...
> > +virCPUUpdate(virArch arch,
> > +             virCPUDefPtr guest,
> > +             const virCPUDef *host)
> 
> Still not 100% clear whether if eventually (patch 40) it's possible to
> enter here with 'host == NULL'... Keeping in mind from patch 26 that
> virQEMUCapsInitHostCPUModel could have qemuCaps->cpuModel = NULL, thus
> the virQEMUCapsGetHostModel could then return a NULL from
> qemuProcessUpdateGuestCPU which calls this function. However, that
> possibility is gated by whether virQEMUCapsIsCPUModeSupported succeeds
> or not. Since you've added NULLSTR and host ? checks, I'm partially
> assuming it's possible to get here with host == NULL. The intro comments
> here don't help me determine that though.

Yes, it is possible to enter virCPUUpdate with host == NULL.
virQEMUCapsIsCPUModeSupported checks hostCPUModel only for
VIR_CPU_MODE_HOST_MODEL, but virCPUUpdate is called for all CPU
definitions. It may seem we do not call virCPUUpdate if hostCPUModel is
NULL since the code is still not in its final shape after this patch
(until patch 40 is applied).

> I'm lost in the terminology between old/new that's described in patch
> 40. If your belief is things are going to be OK, then I'm fine with
> that, but I figured I'd at least point out what I saw and what got
> confusing eventually. Hopefully it's understandable.
> 
> >  {
> >      struct cpuArchDriver *driver;
> >  
> > -    VIR_DEBUG("guest=%p, host=%p", guest, host);
> > +    VIR_DEBUG("arch=%s, guest=%p mode=%s model=%s, host=%p model=%s",
> > +              virArchToString(arch), guest, virCPUModeTypeToString(guest->mode),
> > +              NULLSTR(guest->model), host, NULLSTR(host ? host->model : NULL));
> 
> The Coverity build throws up here... According to cpu.h, arg 3 is
> NONNULL; however, as I pointed out above that could change.
> 
> So at the very least cpu.h needs to change to remove the NONNULL(3).

Yes, fixed.

...
> > +    if (!driver->update) {
> >          virReportError(VIR_ERR_NO_SUPPORT,
> > -                       _("cannot update guest CPU data for %s architecture"),
> > +                       _("cannot update guest CPU for %s architecture"),
> >                         virArchToString(host->arch));'
> 
> Should this just be 'arch' ?  if not, if host == NULL, then this will core.

Oops, yes, I forgot to change this one when adding an explicit arch
parameter.

> 
> >          return -1;
> >      }
> >  
> > -    return driver->update(guest, host);
> > +    if (driver->update(guest, host) < 0)
> 
> Seems as though all implemented ->update functions will return an error
> if 'host' is NULL, so that's good I guess. Whether it's expected is
> something you can answer!

It depends on the guest CPU definition, sometimes host == NULL is OK,
sometimes it is not. And the update functions detect whether they need
host CPU model for updating the CPU and report an error.

> > diff --git a/src/cpu/cpu.h b/src/cpu/cpu.h
> > index 422818e..f4bb51d 100644
> > --- a/src/cpu/cpu.h
> > +++ b/src/cpu/cpu.h
> > @@ -87,8 +87,8 @@ typedef virCPUDefPtr
> >                       unsigned int flags);
> >  
> >  typedef int
> > -(*cpuArchUpdate)    (virCPUDefPtr guest,
> > -                     const virCPUDef *host);
> > +(*virCPUArchUpdate)(virCPUDefPtr guest,
> > +                    const virCPUDef *host);
> 
> Why the name change? I see upcoming patches also use the new
> nomenclature, although I'll assume not all driver functions will change.
> Is that the goal for the future?

I think I mentioned it somewhere in the cover letter or maybe I just
wanted to do that, but the goal is to completely rework all APIs
provided by the cpu driver. Some of them will just change their names to
virCPU*, some of them will be completely removed, and several new APIs
will be added. So the new name also serves as a way to distinguish
reworked/new APIs from those that still needs to be rewritten or
removed.

> > @@ -182,9 +182,10 @@ cpuBaseline (virCPUDefPtr *cpus,
> >      ATTRIBUTE_NONNULL(1);
> >  
> >  int
> > -cpuUpdate   (virCPUDefPtr guest,
> > +virCPUUpdate(virArch arch,
> > +             virCPUDefPtr guest,
> >               const virCPUDef *host)
> > -    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
> > +    ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3);
> 
> Remove the ATTRIBUTE_NONNULL(3) as noted above or even go back to the

Done.

Jirka