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@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