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

Wed Nov 14 18:17:56 UTC 2018

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