On 11/14/2018 07:17 PM, Chris Venteicher wrote: > 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: >>> [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. Oh, my bad. I just skimmed through referenced v3 and did not read it carefully. If I did I would learn that this feature you're adding is not just like any other feature. Therefore code movement and unification makes actually sense. So after all this is the right way to go. Sorry for the noise. All in all, the patches look okayish. But we will have to see v2 of them, I'm afraid. > > 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. This can be easily done in git now (assuming you're on the branch with these patches on): libvirt.git $ git rebase --exec "make -j10 all syntax-check check" master Michal -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
