[libvirt] [PATCHv2 09/11] qemu_capabilities: Persist QEMU instance over multiple QMP Cmds

[Chris Venteicher]

Quoting Collin Walling (2018-07-11 17:48:54)
> I understand your approach, but I do not 100% agree with exposing the QEMU 
> command stuff outside of qemu_capabilities.
> 
> In patch 10/11, you have a function defined in qemu_caps for baselining. I'd
> spawn / kill your QEMU process within there. For when the features flag is set 
> (requiring you to call expansion), I'd have a new function in qemu_caps that 
> follows a similar process for cpu expansion.
> 
> This would lead to libvirt spawning / killing at most 2 separate instances of 
> QEMU (if --features is present) during hypervisor-cpu-baseline, which I do 
> not believe is all that harmful.
> 

Seems like a close call here.

If folks are ok with spinning up QEMU process twice for a command like 
hypervisor baseline then...

I think this likely becomes a question of where are things headed in terms of 
relying on QMP commands (or multiple QMP commands) to do specific things in 
libvirt beyond the scope of querying and caching capabilities.

Seems like were moving in the direction of relying on QEMU to track (and 
calculate based on) the dynamic state of S390 configuration so seemed likely 
there would be more reliance on QMP commands (individual or multiple) to get 
things done in libvirt in the future.

Those were the reasons that tipped me to pull out the ability to start and 
maintain QEMU process across multiple QMP commands.

Acknowledge I don't have the history here though so welcome discussion and glad 
to do whatever you think is best.


> On 07/09/2018 11:56 PM, Chris Venteicher wrote:
> > Commit makes starting a single persistent QEMU instance possible for use
> > over multiple independent QMP commands without starting and stopping
> > QEMU for each QMP command or command type.
> > 
> > Commit allows functions outside qemu_capabilities
> > (ex.  qemuConnectBaselineHypervisorCPU in qemu_driver)
> > requiring multiple different QMP messages to be sent to QEMU to issue
> > those requests over a single QMP Command Channel without starting and
> > stopping QEMU for each independent QMP Command usage.
> > 
> > Commit moves following to global scope so parent function can
> > maintain QEMU instance over multiple QMP commands / command types:
> > virQEMUCapsInitQMPCommand
> > virQEMUCapsInitQMPCommandFree
> > 
> > Commit Introduces virQEMUCapsNewQMPCommandConnection to Start and
> > connect to QEMU so QMP commands can be performed.
> > 
> > The new reusable function isolates code for starting QEMU and
> > establishing Monitor connections from code for obtaining capabilities so
> > that arbitrary QMP commands can be exchanged with QEMU.
> > ---
> >  src/qemu/qemu_capabilities.c | 61 +++++++++++++++++++++++-------------
> >  src/qemu/qemu_capabilities.h | 26 +++++++++++++++
> >  2 files changed, 66 insertions(+), 21 deletions(-)
> > 
> > diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> > index f33152ec40..6f8983384a 100644
> > --- a/src/qemu/qemu_capabilities.c
> > +++ b/src/qemu/qemu_capabilities.c
> > @@ -4265,25 +4265,6 @@ virQEMUCapsInitQMPMonitorTCG(virQEMUCapsPtr qemuCaps ATTRIBUTE_UNUSED,
> >  }
> >  
> >  
> > -typedef struct _virQEMUCapsInitQMPCommand virQEMUCapsInitQMPCommand;
> > -typedef virQEMUCapsInitQMPCommand *virQEMUCapsInitQMPCommandPtr;
> > -struct _virQEMUCapsInitQMPCommand {
> > -    char *binary;
> > -    uid_t runUid;
> > -    gid_t runGid;
> > -    char **qmperr;
> > -    char *qmperr_internal;
> > -    char *monarg;
> > -    char *monpath;
> > -    char *pidfile;
> > -    virCommandPtr cmd;
> > -    qemuMonitorPtr mon;
> > -    virDomainChrSourceDef config;
> > -    pid_t pid;
> > -    virDomainObjPtr vm;
> > -};
> > -
> > -
> >  static void
> >  virQEMUCapsInitQMPCommandAbort(virQEMUCapsInitQMPCommandPtr cmd)
> >  {
> > @@ -4318,7 +4299,7 @@ virQEMUCapsInitQMPCommandAbort(virQEMUCapsInitQMPCommandPtr cmd)
> >  }
> >  
> >  
> > -static void
> > +void
> >  virQEMUCapsInitQMPCommandFree(virQEMUCapsInitQMPCommandPtr cmd)
> >  {
> >      if (!cmd)
> > @@ -4334,7 +4315,7 @@ virQEMUCapsInitQMPCommandFree(virQEMUCapsInitQMPCommandPtr cmd)
> >  
> >  
> >  static virQEMUCapsInitQMPCommandPtr
> > -virQEMUCapsInitQMPCommandNew(char *binary,
> > +virQEMUCapsInitQMPCommandNew(const char *binary,
> >                               const char *libDir,
> >                               uid_t runUid,
> >                               gid_t runGid,
> > @@ -4475,6 +4456,44 @@ virQEMUCapsInitQMPCommandRun(virQEMUCapsInitQMPCommandPtr cmd,
> >  }
> >  
> >  
> > +/* Start and connect to QEMU so QMP commands can be performed.
> > + */
> > +virQEMUCapsInitQMPCommandPtr
> > +virQEMUCapsNewQMPCommandConnection(const char *exec,
> > +                      const char *libDir, uid_t runUid, gid_t runGid,
> > +                      bool forceTCG)
> 
> Nit: line up the 2nd and 3rd line of params with the 1st
> 
> > +{
> > +    virQEMUCapsInitQMPCommandPtr cmd = NULL;
> > +    virQEMUCapsInitQMPCommandPtr rtn_cmd = NULL;
> > +
> > +    VIR_DEBUG("exec =%s", NULLSTR(exec));
> 
> Nit: remove space between exec and =
> 
> > +
> > +    /* Allocate and initialize QMPCommand structure */
> > +    if (!(cmd = virQEMUCapsInitQMPCommandNew(exec, libDir,
> > +                                             runUid, runGid, NULL)))
> > +        goto cleanup;
> > +
> > +    /* Spawn QEMU and establish connection for QMP commands */
> > +    if (virQEMUCapsInitQMPCommandRun(cmd, forceTCG) != 0)
> > +        goto cleanup;
> > +
> > +    /* Exit capabilities negotiation mode and enter QEMU command mode
> > +     * by issuing qmp_capabilities command to QEMU */
> > +    if (qemuMonitorSetCapabilities(cmd->mon) < 0) {
> > +        VIR_DEBUG("Failed to set monitor capabilities %s",
> > +                  virGetLastErrorMessage());
> > +        goto cleanup;
> > +    }
> > +
> > +    VIR_STEAL_PTR(rtn_cmd, cmd);
> > +
> > + cleanup:
> > +    virQEMUCapsInitQMPCommandFree(cmd);
> > +
> > +    return rtn_cmd;
> > +}
> 
> Other than the above nits, the function looks good to me.
> 
> > +
> > +
> >  static int
> >  virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps,
> >                     const char *libDir,
> > diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
> > index d5cd486295..7be636d739 100644
> > --- a/src/qemu/qemu_capabilities.h
> > +++ b/src/qemu/qemu_capabilities.h
> > @@ -490,6 +490,32 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */
> >      QEMU_CAPS_LAST /* this must always be the last item */
> >  } virQEMUCapsFlags;
> >  
> > +typedef struct _virQEMUCapsInitQMPCommand virQEMUCapsInitQMPCommand;
> > +typedef virQEMUCapsInitQMPCommand *virQEMUCapsInitQMPCommandPtr;
> > +
> > +struct _virQEMUCapsInitQMPCommand {
> > +    char *binary;
> > +    uid_t runUid;
> > +    gid_t runGid;
> > +    char **qmperr;
> > +    char *qmperr_internal;
> > +    char *monarg;
> > +    char *monpath;
> > +    char *pidfile;
> > +    virCommandPtr cmd;
> > +    qemuMonitorPtr mon;
> > +    virDomainChrSourceDef config;
> > +    pid_t pid;
> > +    virDomainObjPtr vm;
> > +};
> > +
> > +virQEMUCapsInitQMPCommandPtr
> > +virQEMUCapsNewQMPCommandConnection(const char *exec,
> > +                                   const char *libDir, uid_t runUid, gid_t runGid,
> > +                                   bool forceTCG);
> > +
> > +void virQEMUCapsInitQMPCommandFree(virQEMUCapsInitQMPCommandPtr cmd);
> > +
> >  typedef struct _virQEMUCaps virQEMUCaps;
> >  typedef virQEMUCaps *virQEMUCapsPtr;
> >  
> > 
> 
> 
> -- 
> Respectfully,
> - Collin Walling
>