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@xxxxxxxxxx>
>>> ---
>>> 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
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list