[libvirt] [PATCH 30/41] qemu: Introduce virQEMUCapsIsCPUModeSupported

[John Ferlan]

On 09/15/2016 07:30 AM, Jiri Denemark wrote:
> On Tue, Aug 30, 2016 at 11:08:44 -0400, John Ferlan wrote:
>>
>>
>> On 08/12/2016 09:33 AM, Jiri Denemark wrote:
>>> Signed-off-by: Jiri Denemark <jdenemar at redhat.com>
>>> ---
>>>  src/qemu/qemu_capabilities.c | 55 ++++++++++++++++++++++++++++++++++----------
>>>  src/qemu/qemu_capabilities.h |  4 ++++
>>>  2 files changed, 47 insertions(+), 12 deletions(-)
>>>
>>
>> So another 10 down almost 75% done!  Let's consider ACK's again...
>>
>> Still not sure about patch 20 w/r/t to the need for "unknown" printing,
>> the sorting by yes, no, and unknown, and whether LoadCache should "read"
>> what's been printed and validate against the current (if that matters).
>>
>> As for 21-30, if there's no reply then an ACK can be implied.  A couple
>> of replies (23, 24) make suggestions for function name changes - your
>> choice on those. Two (25, 27) I've just left some thoughts - they don't
>> really require changes.
>>
>> Patch 22 will need an adjustment for an ACK, but the build would fail
>> anyway, so it's obvious.
>>
>> Patch 26 needs an adjust to fix a leak for an ACK, your choice on
>> renaming cpuModel, and I think the virResetLastError should be called.
>> I've seen "future" patches where returning NULL may come into play...
>>
>> Patch 28 - your call on the domain page reference
>>
>> That just leaves this one with one innocuous adjustment shown below. Not
>> required for an ACK
> 
> Heh, this summary doesn't really clarify the situation and it's not
> entirely obvious which patches are ACKed which patches are ACKed once I
> make the suggested changes, and which patches need additional review of
> the changes. We'll see if it changes after I process all review
> comments.
> 
> ...

I was clear to me even reading it days later.

>From 21-30, if I didn't comment, then ACK.

If I did comment or suggest, then your choice on making the changes, but
still it's an implied ACK with the caveat of 22 which had a build
failure and 26 which had a leak and I would assume you would fix

I think you've addressed those so perhaps to be more explicit

ACK 21-30 (and I already ACK'd 20 separately)

John

I trust that you'd make the changes and I didn't want to see PATCH v2
00/41  (or some *larger* number!)

>>> +    if (virQEMUCapsIsCPUModeSupported(qemuCaps, caps, domCaps->virttype,
>>> +                                      VIR_CPU_MODE_CUSTOM)) {
>>> +        virDomainCapsCPUModelsPtr filtered = NULL;
>>> +        char **models = NULL;
>>> +
>>> +        if (qemuCaps->cpuDefinitions &&
>>
>> Redundant check for MODE_CUSTOM when ModeSupported is true
> 
> Right. Fixed.
> 
> Jirka
>