[libvirt] [PATCH v2 05/33] qemu: Refactor virQEMUCapsInitHostCPUModel

[Jiri Denemark]

On Tue, Feb 21, 2017 at 09:24:13 -0500, John Ferlan wrote:
> 
> 
> On 02/15/2017 11:44 AM, Jiri Denemark wrote:
> > Signed-off-by: Jiri Denemark <jdenemar at redhat.com>
> > ---
> > 
> > Notes:
> >     Version 2:
> >     - no change
> > 
> >  src/qemu/qemu_capabilities.c | 109 ++++++++++++++++++++++---------------------
> >  1 file changed, 55 insertions(+), 54 deletions(-)
> > 
> 
> This is "visually" more than a refactor since you've specified an
> initialization of cpu->fallback... That initialization gets essentially
> the same value of 0 (ALLOW == 0) as would any VIR_ALLOC field, so it's
> not a problem per se.

It's just to make the value more visible. We use VIR_CPU_FALLBACK_ALLOW
by default and change it to VIR_CPU_FALLBACK_FORBID when we get the CPU
model from QEMU (rather than by querying the host CPU ourselves).

> Still makes me wonder if there should have been an "UNDEFINED"
> category...

No. Originally there was no fallback attribute and the functionality was
equivalent to fallback="allow". That is the attribute was added just to
be able to turn fallback off.

> My only other comment here is whether there is a concern that your error
> path doesn't clear the qemuCaps->hostCPUModel.  It wasn't clear to me
> whether this path can be called after libvirtd restart and if failure
> would mean anything or not (perhaps the one reason I could think of
> setting NULL previously).
> 
> ACK in principal - might be nice to mention why clearing hostCPUModel on
> failure isn't required anymore.

It didn't make sense even in the original code (if it did, setting it to
NULL would be a clear memory leak). The initial value of
qemuCaps->hostCPUModel is NULL and the code doesn't touch it until we
know everything is OK.

Jirka