Quoting Michal Privoznik (2018-11-14 09:45:06)
On 11/11/2018 08:59 PM, Chris Venteicher wrote:
Make process code usable outside qemu_capabilities by moving code from qemu_capabilities to qemu_process and exposing public functions.
The process code is used to activate a non domain QEMU process for QMP message exchanges.
This patch set modifies capabilities to use the new public functions.
--
The process code is being decoupled from qemu_capabilities now to support hypervisor baseline and comparison using QMP commands.
This patch set was originally submitted as part of the baseline patch set: [libvirt] [PATCH v4 00/37] BaselineHypervisorCPU using QEMU QMP exchanges https://www.redhat.com/archives/libvir-list/2018-November/msg00091.html
Okay, so you want to implement cpu-baseline for s390. But that doesn't really explain the code movement. Also, somehow the code movement makes the code bigger? I guess what I am saying is that I don't see much justification for these patches.
Here is the feedback from an earlier hypervisor baseline review that resulted in this patch set. https://www.redhat.com/archives/libvir-list/2018-July/msg00881.html I think Jiri correctly identified capabilities, and now baseline and comparison, are unrelated services that all independently need to start a non-domain QEMU process for QMP messaging. I am not sure, but it seems likely there could be other (S390...) commands in the future that use QMP messages outside of a domain context to get info or do work at the QEMU level. All the baseline code I had in qemu_capabilities didn't make sense there anymore once I moved the process code from qemu_capabilities to qemu_process. Here is the latest baseline patch set: https://www.redhat.com/archives/libvir-list/2018-November/msg00091.html In the latest baseline patch set, all the baseline code is in qemu_driver and uses the process functions exposed now from qemu_process. So as best I can tell there main choice is... 1) Leave process code in qemu_capabilities and make the 4 core process functions (new, start, stop, free) and data strut public so they can also be used by baseline and comparison from qemu_driver. 2) Move the process code from qemu_capabilities to qemu_process. (this patch set) and expose the functions / data struct from qemu_process. In case 1 functions have the virQemuCaps prefix. In case 2 functions have the qemuProcess prefix. In either approach there are some changes needed to the process code to decouple it from the capabilities code to support both capabilities and baseline. I did spend a few patches in this patch set breaking out the init, process launch and monitor connection code into different static functions in the style used elsewhere in qemu_process. That could be reversed if it doesn't add enough value if the decision is to move the process code to qemu_process.
The baseline and comparison requirements are described here: https://bugzilla.redhat.com/show_bug.cgi?id=1511999 https://bugzilla.redhat.com/show_bug.cgi?id=1511996
I am extracting and resubmitting just the process changes as a stand alone series to try to make review easier.
The patch set shows capabilities using the public functions. To see baseline using the public functions... Look at the "qemu_driver:" patches at the end of https://www.redhat.com/archives/libvir-list/2018-November/msg00091.html
Also, The "qemu_driver: Support feature expansion via QEMU when baselining cpu" patch might be of particular interest because the same QEMU process is used for both baseline and expansion using QMP commands.
--
Many patches were used to isolate code moves and name changes from other actual implementation changes.
The patches reuse the pattern of public qemuProcess{Start,Stop} functions and internal static qemuProcess{Init,Launch,ConnectMonitor} functions but adds a "Qmp" suffix to make them unique.
A number of patches are about re-partitioning the code into static functions for initialization, process launch and connection monitor stuff. This matches the established pattern in qemu_process and seemed to make sense to do.
For concurrency... A thread safe library function creates a unique directory under libDir for each QEMU process (for QMP messaging) to decouple processes in terms of sockets and file system footprint.
Every patch should compile independently if applied in sequence.
Oh, but it doesn't. I'm running 'make -j10 all syntax-check check' and I am hitting compilation/syntax error occasionally.
Yep. My bad. I thought I was careful about making and checking every patch... but stuff got through. At least one of the errors looks like a slip when I did a merge as part of a rebase where I changed the patch order to make it easier to review. It's clear now I need to manualy or by script 'make -j10 all syntax-check check' on each patch before I submit.
Chris Venteicher (22): qemu_process: Move process code from qemu_capabilities to qemu_process qemu_process: Use qemuProcess prefix qemu_process: Limit qemuProcessNew to const input strings qemu_process: Refer to proc not cmd in process code qemu_process: Use consistent name for stop process function qemu_capabilities: Stop QEMU process before freeing qemu_process: Use qemuProcess struct for a single process qemu_process: Persist stderr in qemuProcess struct qemu_capabilities: Detect caps probe failure by checking monitor ptr qemu_process: Introduce qemuProcessStartQmp qemu_process: Collect monitor code in single function qemu_process: Store libDir in qemuProcess struct qemu_process: Setup paths within qemuProcessInitQmp qemu_process: Stop retaining Qemu Monitor config in qemuProcess qemu_process: Don't open monitor if process failed qemu_process: Cleanup qemuProcess alloc function qemu_process: Cleanup qemuProcessStopQmp function qemu_process: Catch process free before process stop qemu_monitor: Make monitor callbacks optional qemu_process: Enter QMP command mode when starting QEMU Process qemu_process: Use unique directories for QMP processes qemu_process: Stop locking QMP process monitor immediately
src/qemu/qemu_capabilities.c | 300 +++++------------------------ src/qemu/qemu_monitor.c | 4 +- src/qemu/qemu_process.c | 356 +++++++++++++++++++++++++++++++++++ src/qemu/qemu_process.h | 37 ++++ tests/qemucapabilitiestest.c | 7 + 5 files changed, 444 insertions(+), 260 deletions(-)
Michal