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

[Collin Walling]

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.

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