Re: [libvirt] [PATCH v2 05/33] qemu: Refactor virQEMUCapsInitHostCPUModel
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(a)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