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