Re: [libvirt] [PATCH 30/41] qemu: Introduce virQEMUCapsIsCPUModeSupported
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(a)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