Hey Chris,
Your patches are looking better with each iteration. Awesome progress thus far!
I'd to make one suggestion for you that will keep your patches a bit cleaner
and also makes the review process smoother: introduce code in the same patches
that it gets used.
Take for example:
6/11 qemu_monitor: Introduce qemuMonitorCPUModelInfoRemovePropByBoolValue
The code introduced by this patch is not used until:
11/11 qemu_driver: BaselineHypervisorCPU supports S390 using QEMU/QMP
Which makes it tricky when looking at patch 11, then trying to remember which
patch introduced the new function. Having everything in one patch allows a
reviewer to see all of your new code and when it gets used in once glance
(or in one email / git show).
Last thing: make sure to commit with --signoff so your name is present at the
bottom of each commit :)
On 07/09/2018 11:56 PM, Chris Venteicher wrote:
> Some architectures (S390) depend on QEMU to compute baseline CPU model and
> expand a models feature set.
>
> Interacting with QEMU requires starting the QEMU process and completing one or
> more query-cpu-model-baseline QMP exchanges with QEMU in addition to a
> query-cpu-model-expansion QMP exchange to expand all features in the model.
>
> See "s390x CPU models: exposing features" patch set on Qemu-devel for discussion
> of QEMU aspects.
>
> This is part of resolution of: https://bugzilla.redhat.com/show_bug.cgi?id=1511999
>
> Version 2 address all issues raised by Collin as well as all other issues
> identified with additional testing.
>
>
> Chris Venteicher (11):
> qemu_monitor_json: Introduce qemuMonitorCPUModelInfo / JSON conversion
> qemu_monitor: Introduce qemuMonitorGetCPUModelBaseline
> (query-cpu-model-baseline)
> qemu_monitor: Indicate when CPUModelInfo props report migratablity
> qemu_monitor: Introduce qemuMonitorCPUModelInfoInit and
> qemuMonitorCPUModelInfoFreeContents
> qemu_monitor: CPUModelExpansion on both model name and properties
> qemu_monitor: Introduce qemuMonitorCPUModelInfoRemovePropByBoolValue
> qemu_capabilities: Introduce virCPUDef / qemuMonitorCPUModelInfo
> conversions
> qemu_capabilities: QMPCommandPtr without maintaining shadow qmperr
> qemu_capabilities: Persist QEMU instance over multiple QMP Cmds
> qemu_capabilities: Introduce virQEMUCapsQMPBaselineCPUModel (baseline
> using QEMU)
> qemu_driver: BaselineHypervisorCPU supports S390 using QEMU/QMP
>
> src/qemu/qemu_capabilities.c | 349 +++++++++++++++++++++++++++++------
> src/qemu/qemu_capabilities.h | 33 ++++
> src/qemu/qemu_driver.c | 74 +++++++-
> src/qemu/qemu_monitor.c | 117 +++++++++++-
> src/qemu/qemu_monitor.h | 21 ++-
> src/qemu/qemu_monitor_json.c | 232 +++++++++++++++++++----
> src/qemu/qemu_monitor_json.h | 14 +-
> tests/cputest.c | 7 +-
> 8 files changed, 724 insertions(+), 123 deletions(-)
>
--
Respectfully,
- Collin Walling
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list