Re: [libvirt] [PATCH RFC 00/22] Move process code to qemu_process

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.