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