Re: [PATCHv2 00/11] BaselineHypervisorCPU using QEMU QMP exchanges

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux