[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v5 20/36] qemu_process: Enter QMP command mode when starting QEMU Process



Quoting Jiri Denemark (2019-01-03 08:54:21)
> On Sun, Dec 02, 2018 at 23:10:14 -0600, Chris Venteicher wrote:
> > qemuProcessQmpStart starts a QEMU process and monitor connection that
> > can be used by multiple functions possibly for multiple QMP commands.
> > 
> > The QMP exchange to exit capabilities negotiation mode and enter command mode
> > can only be performed once after the monitor connection is established.
> > 
> > Move responsibility for entering QMP command mode into the qemuProcessQmp
> > code so multiple functions can issue QMP commands in arbitrary orders.
> > 
> > This also simplifies the functions using the connection provided by
> > qemuProcessQmpStart to issue QMP commands.
> > 
> > Test code now needs to call qemuMonitorSetCapabilities to send the
> > message to switch to command mode because the test code does not use the
> > qemuProcessQmp command that internally calls qemuMonitorSetCapabilities.
> > 
> > Signed-off-by: Chris Venteicher <cventeic redhat com>
> > ---
> >  src/qemu/qemu_capabilities.c | 12 ------------
> >  src/qemu/qemu_process.c      |  8 ++++++++
> >  tests/qemucapabilitiestest.c |  7 +++++++
> >  3 files changed, 15 insertions(+), 12 deletions(-)
> > 
> > diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> > index ce60648897..038e3ecf7a 100644
> > --- a/src/qemu/qemu_capabilities.c
> > +++ b/src/qemu/qemu_capabilities.c
> > @@ -4035,12 +4035,6 @@ virQEMUCapsInitQMPMonitor(virQEMUCapsPtr qemuCaps,
> >  
> >      /* @mon is supposed to be locked by callee */
> >  
> > -    if (qemuMonitorSetCapabilities(mon) < 0) {
> > -        VIR_DEBUG("Failed to set monitor capabilities %s",
> > -                  virGetLastErrorMessage());
> > -        goto cleanup;
> > -    }
> > -
> >      if (qemuMonitorGetVersion(mon,
> >                                &major, &minor, &micro,
> >                                &package) < 0) {
> > @@ -4213,12 +4207,6 @@ virQEMUCapsInitQMPMonitorTCG(virQEMUCapsPtr qemuCaps ATTRIBUTE_UNUSED,
> >  {
> >      int ret = -1;
> >  
> > -    if (qemuMonitorSetCapabilities(mon) < 0) {
> > -        VIR_DEBUG("Failed to set monitor capabilities %s",
> > -                  virGetLastErrorMessage());
> > -        goto cleanup;
> > -    }
> > -
> >      if (virQEMUCapsProbeQMPCPUDefinitions(qemuCaps, mon, true) < 0)
> >          goto cleanup;
> >  
> > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> > index 24945b1d17..80b938cc0b 100644
> > --- a/src/qemu/qemu_process.c
> > +++ b/src/qemu/qemu_process.c
> > @@ -8332,6 +8332,14 @@ qemuProcessQmpConnectMonitor(qemuProcessQmpPtr proc)
> >  
> >      virObjectLock(proc->mon);
> >  
> > +    /* Exit capabilities negotiation mode and enter QEMU command mode
> > +     * by issuing qmp_capabilities command to QEMU */
> > +    if (qemuMonitorSetCapabilities(proc->mon) < 0) {
> > +        VIR_DEBUG("Failed to set monitor capabilities %s",
> > +                  virGetLastErrorMessage());
> > +        goto cleanup;
> > +    }
> > +
> 
> It's quite unlikely, but we may eventually issue more commands to
> initialize the monitor, which would require us to update the code in
> qemucapabilitiestest.c again. Moreover, the test is supposed to test the
> QEMU capabilities probing code rather than individual monitor commands.
> Thus it should avoid direct qemuMonitor* calls.
> 
> For this reason, I think it would be better to separate this code into a
> new function called qemuProcessQMPInitMonitor. And you can remove the
> comment or transform it into a description of the new function. The new
> function will be called from here and from qemucapabilitiestest.c. Thus
> it cannot be static and its prototype should be placed in
> src/qemu/qemu_processpriv.h since the function will be externally called
> only from the test suite.
> 
> As a bonus, the code will be even more consistent with the VM startup
> code, where qemuProcessInitMonitor is doing similar thing.
> 
> Jirka

In the hypervisor baseline code (following patches) a series of different
QMP transactions are completed using the same QEMU process instance.

There are a series of QMP baseline messages followed by a QMP cpu
expansion message over the same connection to the same QEMU process.

Since qemuMonitorSetCapabilities must be called immediately and can only
be called once to put the connection monitor in the right state to
handle QMP messages we don't want to do qemuMonitorSetCapabilities in
individual functions to execute QMP baseline, expansion, etc. because
these functions can't know if qemuMonitorSetCapabilities has already
been called and it can't be called twice.

This seems to push in a direction where qemuMonitorSetCapabilities would
always be required right after qemuProcessQMPStart since the process is
not usable without and you can't allow qemuMonitorSetCapabilities to be
called more than once in downstream execution paths in different
decoupled functions downstream from qemuProcessQMPStart.

Since it seems mandatory that qemuMonitorSetCapabilities be called once
and only once after any call to qemuProcessQMPStart it seemed to make
sense to pull qemuMonitorSetCapabilities into qemuProcessStart rather
than always require the same series of two coupled function calls
anytime a qemu process (for QMP) is started.

That the idea behind why qemuMonitorSetCapabilities was pulled into
qemuProcessQMPStart but can go with whatever you think makes more sense.


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]