[libvirt] [PATCH 26/41] qemu: Store host-model CPU in qemu capabilities

[Jiri Denemark]

On Tue, Aug 30, 2016 at 10:27:44 -0400, John Ferlan wrote:
...
> > diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> > index 8f55fcc..97dc877 100644
> > --- a/src/qemu/qemu_capabilities.c
> > +++ b/src/qemu/qemu_capabilities.c
> > @@ -374,6 +374,7 @@ struct _virQEMUCaps {
> >      virArch arch;
> >  
> >      virDomainCapsCPUModelsPtr cpuDefinitions;
> > +    virCPUDefPtr cpuModel;
> 
> hostCpuModel ?
> 
> IOW: Some way to make it more clear to a casual reader and perhaps
> easier to find via cscope
> 
> Also I note that this isn't being written out to the cache (e.g. no
> change to virQEMUCapsFormatCache), so probably need to "note" that some
> how. Furthermore, perhaps this should be moved after the gic stuff and
> the note made that anything "after" this point isn't written out, rather
> it's refetched during Load

Makes sense, all of it. Fixed.

...
> > +void
> > +virQEMUCapsInitHostCPUModel(virQEMUCapsPtr qemuCaps,
> > +                            virCapsHostPtr host)
> > +{
> > +    virCPUDefPtr cpu = NULL;
> > +
> > +    if (!virQEMUCapsGuestIsNative(host->arch, qemuCaps->arch))
> > +        goto error;
> > +
> > +    if (host->cpu && host->cpu->model) {
> > +        if (VIR_ALLOC(cpu) < 0)
> > +            goto error;
> > +
> > +        cpu->sockets = cpu->cores = cpu->threads = 0;
> > +        cpu->type = VIR_CPU_TYPE_GUEST;
> > +        cpu->mode = VIR_CPU_MODE_CUSTOM;
> > +        cpu->match = VIR_CPU_MATCH_EXACT;
> > +
> > +        if (virCPUDefCopyModel(cpu, host->cpu, true) < 0)
> > +            goto error;
> > +    }
> > +
> > +    qemuCaps->cpuModel = cpu;
> 
> This is leaked - eg. no virCPUDefFree in virQEMUCapsDispose

Oops. Fixed.

> Since this is a void function it is possible to have a failure (above)
> thus having qemuCaps->cpuModel be NULL... Store that knowledge away as
> it becomes important later in patch 40 as processing will call
> virQEMUCapsGetHostModel which returns this field.

Not only that. The host->cpu->model is not guaranteed to be set (e.g.,
it's not set on AArch64) and thus qemuCaps->hostCPUModel can be NULL
even if there's no failure.

> 
> > +    return;
> > +
> > + error:
> > +    virCPUDefFree(cpu);
> > +    qemuCaps->cpuModel = NULL;
> 
> Should we virResetLastError() since we don't care about the failure in
> the two callers?

I guess so. Fixed.

Jirka