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

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.


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(-)

-- 
2.17.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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.

> 
> 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.


> 
> 
> 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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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:
>>>   [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.

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

On Sun, Nov 11, 2018 at 13:59:08 -0600, 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
> 
> 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.

I know this was suggested by Colin, but it actually makes review harder,
because there's no code which would make use of the new functions and
thus there seems to be no reason for refactoring. Not to mention that
seeing real usage of a new function influences its design.

Although it surely makes sense do have the refactoring patches in the
beginning of the series rather than mixing them with the rest of the new
code.

I applied both this series and your former v4 series, rebased the v4 one
on top of this RFC and now I'm reviewing the result. So please, keep
sending just one series with all the patches.

During the review, I'm not going to repeat anything already mentioned by
Michal, unless I want to say something more about it.

Jirka

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list