Qemu process code in qemu_capabilities.c is moved to qemu_process.c in order to make the code usable outside the original capabilities usecases. The moved code activates and manages Qemu processes without establishing a guest domain. ** This patch is a straight cut/paste move between files. ** (Exception: I did change a function prototype in qemu_process.h to be one parameter per line.) Then, subsequent patches modify the process code to make function prefixes and variable names match qemu_process, and make the code usable for more than the capabilities usecase. Signed-off-by: Chris Venteicher <cventeic@redhat.com> --- src/qemu/qemu_capabilities.c | 218 +---------------------------------- src/qemu/qemu_process.c | 199 ++++++++++++++++++++++++++++++++ src/qemu/qemu_process.h | 30 +++++ 3 files changed, 230 insertions(+), 217 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 20a1a0c201..14b1ab5bec 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -47,6 +47,7 @@ #define __QEMU_CAPSPRIV_H_ALLOW__ #include "qemu_capspriv.h" #include "qemu_qapi.h" +#include "qemu_process.h" #include <fcntl.h> #include <sys/stat.h> @@ -3939,18 +3940,6 @@ virQEMUCapsIsValid(void *data, } -static void virQEMUCapsMonitorNotify(qemuMonitorPtr mon ATTRIBUTE_UNUSED, - virDomainObjPtr vm ATTRIBUTE_UNUSED, - void *opaque ATTRIBUTE_UNUSED) -{ -} - -static qemuMonitorCallbacks callbacks = { - .eofNotify = virQEMUCapsMonitorNotify, - .errorNotify = virQEMUCapsMonitorNotify, -}; - - /** * virQEMUCapsInitQMPArch: * @qemuCaps: QEMU capabilities @@ -4245,211 +4234,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 *monarg; - char *monpath; - char *pidfile; - virCommandPtr cmd; - qemuMonitorPtr mon; - virDomainChrSourceDef config; - pid_t pid; - virDomainObjPtr vm; -}; - - -static void -virQEMUCapsInitQMPCommandAbort(virQEMUCapsInitQMPCommandPtr cmd) -{ - if (cmd->mon) - virObjectUnlock(cmd->mon); - qemuMonitorClose(cmd->mon); - cmd->mon = NULL; - - virCommandAbort(cmd->cmd); - virCommandFree(cmd->cmd); - cmd->cmd = NULL; - - if (cmd->monpath) - unlink(cmd->monpath); - - virDomainObjEndAPI(&cmd->vm); - - if (cmd->pid != 0) { - char ebuf[1024]; - - VIR_DEBUG("Killing QMP caps process %lld", (long long)cmd->pid); - if (virProcessKill(cmd->pid, SIGKILL) < 0 && errno != ESRCH) - VIR_ERROR(_("Failed to kill process %lld: %s"), - (long long)cmd->pid, - virStrerror(errno, ebuf, sizeof(ebuf))); - - VIR_FREE(*cmd->qmperr); - } - if (cmd->pidfile) - unlink(cmd->pidfile); - cmd->pid = 0; -} - - -static void -virQEMUCapsInitQMPCommandFree(virQEMUCapsInitQMPCommandPtr cmd) -{ - if (!cmd) - return; - - virQEMUCapsInitQMPCommandAbort(cmd); - VIR_FREE(cmd->binary); - VIR_FREE(cmd->monpath); - VIR_FREE(cmd->monarg); - VIR_FREE(cmd->pidfile); - VIR_FREE(cmd); -} - - -static virQEMUCapsInitQMPCommandPtr -virQEMUCapsInitQMPCommandNew(char *binary, - const char *libDir, - uid_t runUid, - gid_t runGid, - char **qmperr) -{ - virQEMUCapsInitQMPCommandPtr cmd = NULL; - - if (VIR_ALLOC(cmd) < 0) - goto error; - - if (VIR_STRDUP(cmd->binary, binary) < 0) - goto error; - - cmd->runUid = runUid; - cmd->runGid = runGid; - cmd->qmperr = qmperr; - - /* the ".sock" sufix is important to avoid a possible clash with a qemu - * domain called "capabilities" - */ - if (virAsprintf(&cmd->monpath, "%s/%s", libDir, - "capabilities.monitor.sock") < 0) - goto error; - if (virAsprintf(&cmd->monarg, "unix:%s,server,nowait", cmd->monpath) < 0) - goto error; - - /* ".pidfile" suffix is used rather than ".pid" to avoid a possible clash - * with a qemu domain called "capabilities" - * Normally we'd use runDir for pid files, but because we're using - * -daemonize we need QEMU to be allowed to create them, rather - * than libvirtd. So we're using libDir which QEMU can write to - */ - if (virAsprintf(&cmd->pidfile, "%s/%s", libDir, "capabilities.pidfile") < 0) - goto error; - - virPidFileForceCleanupPath(cmd->pidfile); - - cmd->config.type = VIR_DOMAIN_CHR_TYPE_UNIX; - cmd->config.data.nix.path = cmd->monpath; - cmd->config.data.nix.listen = false; - - return cmd; - - error: - virQEMUCapsInitQMPCommandFree(cmd); - return NULL; -} - - -/* Returns -1 on fatal error, - * 0 on success, - * 1 when probing QEMU failed - */ -static int -virQEMUCapsInitQMPCommandRun(virQEMUCapsInitQMPCommandPtr cmd, - bool forceTCG) -{ - virDomainXMLOptionPtr xmlopt = NULL; - const char *machine; - int status = 0; - int ret = -1; - - if (forceTCG) - machine = "none,accel=tcg"; - else - machine = "none,accel=kvm:tcg"; - - VIR_DEBUG("Try to probe capabilities of '%s' via QMP, machine %s", - cmd->binary, machine); - - /* - * We explicitly need to use -daemonize here, rather than - * virCommandDaemonize, because we need to synchronize - * with QEMU creating its monitor socket API. Using - * daemonize guarantees control won't return to libvirt - * until the socket is present. - */ - cmd->cmd = virCommandNewArgList(cmd->binary, - "-S", - "-no-user-config", - "-nodefaults", - "-nographic", - "-machine", machine, - "-qmp", cmd->monarg, - "-pidfile", cmd->pidfile, - "-daemonize", - NULL); - virCommandAddEnvPassCommon(cmd->cmd); - virCommandClearCaps(cmd->cmd); - virCommandSetGID(cmd->cmd, cmd->runGid); - virCommandSetUID(cmd->cmd, cmd->runUid); - - virCommandSetErrorBuffer(cmd->cmd, cmd->qmperr); - - /* Log, but otherwise ignore, non-zero status. */ - if (virCommandRun(cmd->cmd, &status) < 0) - goto cleanup; - - if (status != 0) { - VIR_DEBUG("QEMU %s exited with status %d: %s", - cmd->binary, status, *cmd->qmperr); - goto ignore; - } - - if (virPidFileReadPath(cmd->pidfile, &cmd->pid) < 0) { - VIR_DEBUG("Failed to read pidfile %s", cmd->pidfile); - goto ignore; - } - - if (!(xmlopt = virDomainXMLOptionNew(NULL, NULL, NULL, NULL, NULL)) || - !(cmd->vm = virDomainObjNew(xmlopt))) - goto cleanup; - - cmd->vm->pid = cmd->pid; - - if (!(cmd->mon = qemuMonitorOpen(cmd->vm, &cmd->config, true, true, - 0, &callbacks, NULL))) - goto ignore; - - virObjectLock(cmd->mon); - - ret = 0; - - cleanup: - if (!cmd->mon) - virQEMUCapsInitQMPCommandAbort(cmd); - virObjectUnref(xmlopt); - - return ret; - - ignore: - ret = 1; - goto cleanup; -} - - static int virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, const char *libDir, diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 12d1fca0d4..2cc52d3394 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8093,3 +8093,202 @@ qemuProcessReconnectAll(virQEMUDriverPtr driver) struct qemuProcessReconnectData data = {.driver = driver}; virDomainObjListForEach(driver->domains, qemuProcessReconnectHelper, &data); } + + +static void virQEMUCapsMonitorNotify(qemuMonitorPtr mon ATTRIBUTE_UNUSED, + virDomainObjPtr vm ATTRIBUTE_UNUSED, + void *opaque ATTRIBUTE_UNUSED) +{ +} + +static qemuMonitorCallbacks callbacks = { + .eofNotify = virQEMUCapsMonitorNotify, + .errorNotify = virQEMUCapsMonitorNotify, +}; + + +void +virQEMUCapsInitQMPCommandFree(virQEMUCapsInitQMPCommandPtr cmd) +{ + if (!cmd) + return; + + virQEMUCapsInitQMPCommandAbort(cmd); + VIR_FREE(cmd->binary); + VIR_FREE(cmd->monpath); + VIR_FREE(cmd->monarg); + VIR_FREE(cmd->pidfile); + VIR_FREE(cmd); +} + + +virQEMUCapsInitQMPCommandPtr +virQEMUCapsInitQMPCommandNew(char *binary, + const char *libDir, + uid_t runUid, + gid_t runGid, + char **qmperr) +{ + virQEMUCapsInitQMPCommandPtr cmd = NULL; + + if (VIR_ALLOC(cmd) < 0) + goto error; + + if (VIR_STRDUP(cmd->binary, binary) < 0) + goto error; + + cmd->runUid = runUid; + cmd->runGid = runGid; + cmd->qmperr = qmperr; + + /* the ".sock" sufix is important to avoid a possible clash with a qemu + * domain called "capabilities" + */ + if (virAsprintf(&cmd->monpath, "%s/%s", libDir, + "capabilities.monitor.sock") < 0) + goto error; + if (virAsprintf(&cmd->monarg, "unix:%s,server,nowait", cmd->monpath) < 0) + goto error; + + /* ".pidfile" suffix is used rather than ".pid" to avoid a possible clash + * with a qemu domain called "capabilities" + * Normally we'd use runDir for pid files, but because we're using + * -daemonize we need QEMU to be allowed to create them, rather + * than libvirtd. So we're using libDir which QEMU can write to + */ + if (virAsprintf(&cmd->pidfile, "%s/%s", libDir, "capabilities.pidfile") < 0) + goto error; + + virPidFileForceCleanupPath(cmd->pidfile); + + cmd->config.type = VIR_DOMAIN_CHR_TYPE_UNIX; + cmd->config.data.nix.path = cmd->monpath; + cmd->config.data.nix.listen = false; + + return cmd; + + error: + virQEMUCapsInitQMPCommandFree(cmd); + return NULL; +} + + +/* Returns -1 on fatal error, + * 0 on success, + * 1 when probing QEMU failed + */ +int +virQEMUCapsInitQMPCommandRun(virQEMUCapsInitQMPCommandPtr cmd, + bool forceTCG) +{ + virDomainXMLOptionPtr xmlopt = NULL; + const char *machine; + int status = 0; + int ret = -1; + + if (forceTCG) + machine = "none,accel=tcg"; + else + machine = "none,accel=kvm:tcg"; + + VIR_DEBUG("Try to probe capabilities of '%s' via QMP, machine %s", + cmd->binary, machine); + + /* + * We explicitly need to use -daemonize here, rather than + * virCommandDaemonize, because we need to synchronize + * with QEMU creating its monitor socket API. Using + * daemonize guarantees control won't return to libvirt + * until the socket is present. + */ + cmd->cmd = virCommandNewArgList(cmd->binary, + "-S", + "-no-user-config", + "-nodefaults", + "-nographic", + "-machine", machine, + "-qmp", cmd->monarg, + "-pidfile", cmd->pidfile, + "-daemonize", + NULL); + virCommandAddEnvPassCommon(cmd->cmd); + virCommandClearCaps(cmd->cmd); + virCommandSetGID(cmd->cmd, cmd->runGid); + virCommandSetUID(cmd->cmd, cmd->runUid); + + virCommandSetErrorBuffer(cmd->cmd, cmd->qmperr); + + /* Log, but otherwise ignore, non-zero status. */ + if (virCommandRun(cmd->cmd, &status) < 0) + goto cleanup; + + if (status != 0) { + VIR_DEBUG("QEMU %s exited with status %d: %s", + cmd->binary, status, *cmd->qmperr); + goto ignore; + } + + if (virPidFileReadPath(cmd->pidfile, &cmd->pid) < 0) { + VIR_DEBUG("Failed to read pidfile %s", cmd->pidfile); + goto ignore; + } + + if (!(xmlopt = virDomainXMLOptionNew(NULL, NULL, NULL, NULL, NULL)) || + !(cmd->vm = virDomainObjNew(xmlopt))) + goto cleanup; + + cmd->vm->pid = cmd->pid; + + if (!(cmd->mon = qemuMonitorOpen(cmd->vm, &cmd->config, true, true, + 0, &callbacks, NULL))) + goto ignore; + + virObjectLock(cmd->mon); + + ret = 0; + + cleanup: + if (!cmd->mon) + virQEMUCapsInitQMPCommandAbort(cmd); + virObjectUnref(xmlopt); + + return ret; + + ignore: + ret = 1; + goto cleanup; +} + + +void +virQEMUCapsInitQMPCommandAbort(virQEMUCapsInitQMPCommandPtr cmd) +{ + if (cmd->mon) + virObjectUnlock(cmd->mon); + qemuMonitorClose(cmd->mon); + cmd->mon = NULL; + + virCommandAbort(cmd->cmd); + virCommandFree(cmd->cmd); + cmd->cmd = NULL; + + if (cmd->monpath) + unlink(cmd->monpath); + + virDomainObjEndAPI(&cmd->vm); + + if (cmd->pid != 0) { + char ebuf[1024]; + + VIR_DEBUG("Killing QMP caps process %lld", (long long)cmd->pid); + if (virProcessKill(cmd->pid, SIGKILL) < 0 && errno != ESRCH) + VIR_ERROR(_("Failed to kill process %lld: %s"), + (long long)cmd->pid, + virStrerror(errno, ebuf, sizeof(ebuf))); + + VIR_FREE(*cmd->qmperr); + } + if (cmd->pidfile) + unlink(cmd->pidfile); + cmd->pid = 0; +} diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h index 2037467c94..5e6f7c76ac 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -214,4 +214,34 @@ int qemuProcessStartManagedPRDaemon(virDomainObjPtr vm); void qemuProcessKillManagedPRDaemon(virDomainObjPtr vm); +typedef struct _virQEMUCapsInitQMPCommand virQEMUCapsInitQMPCommand; +typedef virQEMUCapsInitQMPCommand *virQEMUCapsInitQMPCommandPtr; +struct _virQEMUCapsInitQMPCommand { + char *binary; + uid_t runUid; + gid_t runGid; + char **qmperr; + char *monarg; + char *monpath; + char *pidfile; + virCommandPtr cmd; + qemuMonitorPtr mon; + virDomainChrSourceDef config; + pid_t pid; + virDomainObjPtr vm; +}; + +virQEMUCapsInitQMPCommandPtr virQEMUCapsInitQMPCommandNew(char *binary, + const char *libDir, + uid_t runUid, + gid_t runGid, + char **qmperr); + +void virQEMUCapsInitQMPCommandFree(virQEMUCapsInitQMPCommandPtr cmd); + +int virQEMUCapsInitQMPCommandRun(virQEMUCapsInitQMPCommandPtr cmd, + bool forceTCG); + +void virQEMUCapsInitQMPCommandAbort(virQEMUCapsInitQMPCommandPtr cmd); + #endif /* __QEMU_PROCESS_H__ */ -- 2.17.1

s/virQEMUCapsInitQMPCommand/qemuProcessQmp/ No functionality change. Use file appropriate prefix in moved code. Signed-off-by: Chris Venteicher <cventeic@redhat.com> --- src/qemu/qemu_capabilities.c | 14 +++++++------- src/qemu/qemu_process.c | 28 ++++++++++++++-------------- src/qemu/qemu_process.h | 24 ++++++++++++------------ 3 files changed, 33 insertions(+), 33 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 14b1ab5bec..2f78fe27fe 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -4241,15 +4241,15 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, gid_t runGid, char **qmperr) { - virQEMUCapsInitQMPCommandPtr cmd = NULL; + qemuProcessQmpPtr cmd = NULL; int ret = -1; int rc; - if (!(cmd = virQEMUCapsInitQMPCommandNew(qemuCaps->binary, libDir, - runUid, runGid, qmperr))) + if (!(cmd = qemuProcessQmpNew(qemuCaps->binary, libDir, + runUid, runGid, qmperr))) goto cleanup; - if ((rc = virQEMUCapsInitQMPCommandRun(cmd, false)) != 0) { + if ((rc = qemuProcessQmpRun(cmd, false)) != 0) { if (rc == 1) ret = 0; goto cleanup; @@ -4259,8 +4259,8 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, goto cleanup; if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM)) { - virQEMUCapsInitQMPCommandAbort(cmd); - if ((rc = virQEMUCapsInitQMPCommandRun(cmd, true)) != 0) { + qemuProcessQmpAbort(cmd); + if ((rc = qemuProcessQmpRun(cmd, true)) != 0) { if (rc == 1) ret = 0; goto cleanup; @@ -4273,7 +4273,7 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, ret = 0; cleanup: - virQEMUCapsInitQMPCommandFree(cmd); + qemuProcessQmpFree(cmd); return ret; } diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 2cc52d3394..35e0c6172a 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8108,12 +8108,12 @@ static qemuMonitorCallbacks callbacks = { void -virQEMUCapsInitQMPCommandFree(virQEMUCapsInitQMPCommandPtr cmd) +qemuProcessQmpFree(qemuProcessQmpPtr cmd) { if (!cmd) return; - virQEMUCapsInitQMPCommandAbort(cmd); + qemuProcessQmpAbort(cmd); VIR_FREE(cmd->binary); VIR_FREE(cmd->monpath); VIR_FREE(cmd->monarg); @@ -8122,14 +8122,14 @@ virQEMUCapsInitQMPCommandFree(virQEMUCapsInitQMPCommandPtr cmd) } -virQEMUCapsInitQMPCommandPtr -virQEMUCapsInitQMPCommandNew(char *binary, - const char *libDir, - uid_t runUid, - gid_t runGid, - char **qmperr) +qemuProcessQmpPtr +qemuProcessQmpNew(char *binary, + const char *libDir, + uid_t runUid, + gid_t runGid, + char **qmperr) { - virQEMUCapsInitQMPCommandPtr cmd = NULL; + qemuProcessQmpPtr cmd = NULL; if (VIR_ALLOC(cmd) < 0) goto error; @@ -8168,7 +8168,7 @@ virQEMUCapsInitQMPCommandNew(char *binary, return cmd; error: - virQEMUCapsInitQMPCommandFree(cmd); + qemuProcessQmpFree(cmd); return NULL; } @@ -8178,8 +8178,8 @@ virQEMUCapsInitQMPCommandNew(char *binary, * 1 when probing QEMU failed */ int -virQEMUCapsInitQMPCommandRun(virQEMUCapsInitQMPCommandPtr cmd, - bool forceTCG) +qemuProcessQmpRun(qemuProcessQmpPtr cmd, + bool forceTCG) { virDomainXMLOptionPtr xmlopt = NULL; const char *machine; @@ -8249,7 +8249,7 @@ virQEMUCapsInitQMPCommandRun(virQEMUCapsInitQMPCommandPtr cmd, cleanup: if (!cmd->mon) - virQEMUCapsInitQMPCommandAbort(cmd); + qemuProcessQmpAbort(cmd); virObjectUnref(xmlopt); return ret; @@ -8261,7 +8261,7 @@ virQEMUCapsInitQMPCommandRun(virQEMUCapsInitQMPCommandPtr cmd, void -virQEMUCapsInitQMPCommandAbort(virQEMUCapsInitQMPCommandPtr cmd) +qemuProcessQmpAbort(qemuProcessQmpPtr cmd) { if (cmd->mon) virObjectUnlock(cmd->mon); diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h index 5e6f7c76ac..67ed90ad4d 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -214,9 +214,9 @@ int qemuProcessStartManagedPRDaemon(virDomainObjPtr vm); void qemuProcessKillManagedPRDaemon(virDomainObjPtr vm); -typedef struct _virQEMUCapsInitQMPCommand virQEMUCapsInitQMPCommand; -typedef virQEMUCapsInitQMPCommand *virQEMUCapsInitQMPCommandPtr; -struct _virQEMUCapsInitQMPCommand { +typedef struct _qemuProcessQmp qemuProcessQmp; +typedef qemuProcessQmp *qemuProcessQmpPtr; +struct _qemuProcessQmp { char *binary; uid_t runUid; gid_t runGid; @@ -231,17 +231,17 @@ struct _virQEMUCapsInitQMPCommand { virDomainObjPtr vm; }; -virQEMUCapsInitQMPCommandPtr virQEMUCapsInitQMPCommandNew(char *binary, - const char *libDir, - uid_t runUid, - gid_t runGid, - char **qmperr); +qemuProcessQmpPtr qemuProcessQmpNew(char *binary, + const char *libDir, + uid_t runUid, + gid_t runGid, + char **qmperr); -void virQEMUCapsInitQMPCommandFree(virQEMUCapsInitQMPCommandPtr cmd); +void qemuProcessQmpFree(qemuProcessQmpPtr cmd); -int virQEMUCapsInitQMPCommandRun(virQEMUCapsInitQMPCommandPtr cmd, - bool forceTCG); +int qemuProcessQmpRun(qemuProcessQmpPtr cmd, + bool forceTCG); -void virQEMUCapsInitQMPCommandAbort(virQEMUCapsInitQMPCommandPtr cmd); +void qemuProcessQmpAbort(qemuProcessQmpPtr cmd); #endif /* __QEMU_PROCESS_H__ */ -- 2.17.1

Add the const qualifier on non modified strings (string only copied inside qemuProcessQmpNew) so that const strings can be used directly in calls to qemuProcessQmpNew in future patches. Signed-off-by: Chris Venteicher <cventeic@redhat.com> --- src/qemu/qemu_process.c | 2 +- src/qemu/qemu_process.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 35e0c6172a..46aed4fc9c 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8123,7 +8123,7 @@ qemuProcessQmpFree(qemuProcessQmpPtr cmd) qemuProcessQmpPtr -qemuProcessQmpNew(char *binary, +qemuProcessQmpNew(const char *binary, const char *libDir, uid_t runUid, gid_t runGid, diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h index 67ed90ad4d..6d4fbda5fc 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -231,7 +231,7 @@ struct _qemuProcessQmp { virDomainObjPtr vm; }; -qemuProcessQmpPtr qemuProcessQmpNew(char *binary, +qemuProcessQmpPtr qemuProcessQmpNew(const char *binary, const char *libDir, uid_t runUid, gid_t runGid, -- 2.17.1

s/cmd/proc/ in process code imported from qemu_capabilities. No functionality is changed. Just variable renaming. Process code imported from qemu_capabilities was oriented around starting a process to issue a single QMP command. Future usecases (ex. baseline, compare) expect to use a single process to issue multiple different QMP commands. This patch changes the variable naming from cmd to proc to put focus on the process being maintained to issue commands. Signed-off-by: Chris Venteicher <cventeic@redhat.com> --- src/qemu/qemu_capabilities.c | 18 ++--- src/qemu/qemu_process.c | 138 +++++++++++++++++------------------ src/qemu/qemu_process.h | 4 +- 3 files changed, 80 insertions(+), 80 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 2f78fe27fe..41a0dfa844 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -4241,39 +4241,39 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, gid_t runGid, char **qmperr) { - qemuProcessQmpPtr cmd = NULL; + qemuProcessQmpPtr proc = NULL; int ret = -1; int rc; - if (!(cmd = qemuProcessQmpNew(qemuCaps->binary, libDir, - runUid, runGid, qmperr))) + if (!(proc = qemuProcessQmpNew(qemuCaps->binary, libDir, + runUid, runGid, qmperr))) goto cleanup; - if ((rc = qemuProcessQmpRun(cmd, false)) != 0) { + if ((rc = qemuProcessQmpRun(proc, false)) != 0) { if (rc == 1) ret = 0; goto cleanup; } - if (virQEMUCapsInitQMPMonitor(qemuCaps, cmd->mon) < 0) + if (virQEMUCapsInitQMPMonitor(qemuCaps, proc->mon) < 0) goto cleanup; if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM)) { - qemuProcessQmpAbort(cmd); - if ((rc = qemuProcessQmpRun(cmd, true)) != 0) { + qemuProcessQmpAbort(proc); + if ((rc = qemuProcessQmpRun(proc, true)) != 0) { if (rc == 1) ret = 0; goto cleanup; } - if (virQEMUCapsInitQMPMonitorTCG(qemuCaps, cmd->mon) < 0) + if (virQEMUCapsInitQMPMonitorTCG(qemuCaps, proc->mon) < 0) goto cleanup; } ret = 0; cleanup: - qemuProcessQmpFree(cmd); + qemuProcessQmpFree(proc); return ret; } diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 46aed4fc9c..5f4853e0c4 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8108,17 +8108,17 @@ static qemuMonitorCallbacks callbacks = { void -qemuProcessQmpFree(qemuProcessQmpPtr cmd) +qemuProcessQmpFree(qemuProcessQmpPtr proc) { - if (!cmd) + if (!proc) return; - qemuProcessQmpAbort(cmd); - VIR_FREE(cmd->binary); - VIR_FREE(cmd->monpath); - VIR_FREE(cmd->monarg); - VIR_FREE(cmd->pidfile); - VIR_FREE(cmd); + qemuProcessQmpAbort(proc); + VIR_FREE(proc->binary); + VIR_FREE(proc->monpath); + VIR_FREE(proc->monarg); + VIR_FREE(proc->pidfile); + VIR_FREE(proc); } @@ -8129,25 +8129,25 @@ qemuProcessQmpNew(const char *binary, gid_t runGid, char **qmperr) { - qemuProcessQmpPtr cmd = NULL; + qemuProcessQmpPtr proc = NULL; - if (VIR_ALLOC(cmd) < 0) + if (VIR_ALLOC(proc) < 0) goto error; - if (VIR_STRDUP(cmd->binary, binary) < 0) + if (VIR_STRDUP(proc->binary, binary) < 0) goto error; - cmd->runUid = runUid; - cmd->runGid = runGid; - cmd->qmperr = qmperr; + proc->runUid = runUid; + proc->runGid = runGid; + proc->qmperr = qmperr; /* the ".sock" sufix is important to avoid a possible clash with a qemu * domain called "capabilities" */ - if (virAsprintf(&cmd->monpath, "%s/%s", libDir, + if (virAsprintf(&proc->monpath, "%s/%s", libDir, "capabilities.monitor.sock") < 0) goto error; - if (virAsprintf(&cmd->monarg, "unix:%s,server,nowait", cmd->monpath) < 0) + if (virAsprintf(&proc->monarg, "unix:%s,server,nowait", proc->monpath) < 0) goto error; /* ".pidfile" suffix is used rather than ".pid" to avoid a possible clash @@ -8156,19 +8156,19 @@ qemuProcessQmpNew(const char *binary, * -daemonize we need QEMU to be allowed to create them, rather * than libvirtd. So we're using libDir which QEMU can write to */ - if (virAsprintf(&cmd->pidfile, "%s/%s", libDir, "capabilities.pidfile") < 0) + if (virAsprintf(&proc->pidfile, "%s/%s", libDir, "capabilities.pidfile") < 0) goto error; - virPidFileForceCleanupPath(cmd->pidfile); + virPidFileForceCleanupPath(proc->pidfile); - cmd->config.type = VIR_DOMAIN_CHR_TYPE_UNIX; - cmd->config.data.nix.path = cmd->monpath; - cmd->config.data.nix.listen = false; + proc->config.type = VIR_DOMAIN_CHR_TYPE_UNIX; + proc->config.data.nix.path = proc->monpath; + proc->config.data.nix.listen = false; - return cmd; + return proc; error: - qemuProcessQmpFree(cmd); + qemuProcessQmpFree(proc); return NULL; } @@ -8178,7 +8178,7 @@ qemuProcessQmpNew(const char *binary, * 1 when probing QEMU failed */ int -qemuProcessQmpRun(qemuProcessQmpPtr cmd, +qemuProcessQmpRun(qemuProcessQmpPtr proc, bool forceTCG) { virDomainXMLOptionPtr xmlopt = NULL; @@ -8192,7 +8192,7 @@ qemuProcessQmpRun(qemuProcessQmpPtr cmd, machine = "none,accel=kvm:tcg"; VIR_DEBUG("Try to probe capabilities of '%s' via QMP, machine %s", - cmd->binary, machine); + proc->binary, machine); /* * We explicitly need to use -daemonize here, rather than @@ -8201,55 +8201,55 @@ qemuProcessQmpRun(qemuProcessQmpPtr cmd, * daemonize guarantees control won't return to libvirt * until the socket is present. */ - cmd->cmd = virCommandNewArgList(cmd->binary, - "-S", - "-no-user-config", - "-nodefaults", - "-nographic", - "-machine", machine, - "-qmp", cmd->monarg, - "-pidfile", cmd->pidfile, - "-daemonize", - NULL); - virCommandAddEnvPassCommon(cmd->cmd); - virCommandClearCaps(cmd->cmd); - virCommandSetGID(cmd->cmd, cmd->runGid); - virCommandSetUID(cmd->cmd, cmd->runUid); + proc->cmd = virCommandNewArgList(proc->binary, + "-S", + "-no-user-config", + "-nodefaults", + "-nographic", + "-machine", machine, + "-qmp", proc->monarg, + "-pidfile", proc->pidfile, + "-daemonize", + NULL); + virCommandAddEnvPassCommon(proc->cmd); + virCommandClearCaps(proc->cmd); + virCommandSetGID(proc->cmd, proc->runGid); + virCommandSetUID(proc->cmd, proc->runUid); - virCommandSetErrorBuffer(cmd->cmd, cmd->qmperr); + virCommandSetErrorBuffer(proc->cmd, proc->qmperr); /* Log, but otherwise ignore, non-zero status. */ - if (virCommandRun(cmd->cmd, &status) < 0) + if (virCommandRun(proc->cmd, &status) < 0) goto cleanup; if (status != 0) { VIR_DEBUG("QEMU %s exited with status %d: %s", - cmd->binary, status, *cmd->qmperr); + proc->binary, status, *proc->qmperr); goto ignore; } - if (virPidFileReadPath(cmd->pidfile, &cmd->pid) < 0) { - VIR_DEBUG("Failed to read pidfile %s", cmd->pidfile); + if (virPidFileReadPath(proc->pidfile, &proc->pid) < 0) { + VIR_DEBUG("Failed to read pidfile %s", proc->pidfile); goto ignore; } if (!(xmlopt = virDomainXMLOptionNew(NULL, NULL, NULL, NULL, NULL)) || - !(cmd->vm = virDomainObjNew(xmlopt))) + !(proc->vm = virDomainObjNew(xmlopt))) goto cleanup; - cmd->vm->pid = cmd->pid; + proc->vm->pid = proc->pid; - if (!(cmd->mon = qemuMonitorOpen(cmd->vm, &cmd->config, true, true, + if (!(proc->mon = qemuMonitorOpen(proc->vm, &proc->config, true, true, 0, &callbacks, NULL))) goto ignore; - virObjectLock(cmd->mon); + virObjectLock(proc->mon); ret = 0; cleanup: - if (!cmd->mon) - qemuProcessQmpAbort(cmd); + if (!proc->mon) + qemuProcessQmpAbort(proc); virObjectUnref(xmlopt); return ret; @@ -8261,34 +8261,34 @@ qemuProcessQmpRun(qemuProcessQmpPtr cmd, void -qemuProcessQmpAbort(qemuProcessQmpPtr cmd) +qemuProcessQmpAbort(qemuProcessQmpPtr proc) { - if (cmd->mon) - virObjectUnlock(cmd->mon); - qemuMonitorClose(cmd->mon); - cmd->mon = NULL; + if (proc->mon) + virObjectUnlock(proc->mon); + qemuMonitorClose(proc->mon); + proc->mon = NULL; - virCommandAbort(cmd->cmd); - virCommandFree(cmd->cmd); - cmd->cmd = NULL; + virCommandAbort(proc->cmd); + virCommandFree(proc->cmd); + proc->cmd = NULL; - if (cmd->monpath) - unlink(cmd->monpath); + if (proc->monpath) + unlink(proc->monpath); - virDomainObjEndAPI(&cmd->vm); + virDomainObjEndAPI(&proc->vm); - if (cmd->pid != 0) { + if (proc->pid != 0) { char ebuf[1024]; - VIR_DEBUG("Killing QMP caps process %lld", (long long)cmd->pid); - if (virProcessKill(cmd->pid, SIGKILL) < 0 && errno != ESRCH) + VIR_DEBUG("Killing QMP caps process %lld", (long long)proc->pid); + if (virProcessKill(proc->pid, SIGKILL) < 0 && errno != ESRCH) VIR_ERROR(_("Failed to kill process %lld: %s"), - (long long)cmd->pid, + (long long)proc->pid, virStrerror(errno, ebuf, sizeof(ebuf))); - VIR_FREE(*cmd->qmperr); + VIR_FREE(*proc->qmperr); } - if (cmd->pidfile) - unlink(cmd->pidfile); - cmd->pid = 0; + if (proc->pidfile) + unlink(proc->pidfile); + proc->pid = 0; } diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h index 6d4fbda5fc..dea5a84e8c 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -237,11 +237,11 @@ qemuProcessQmpPtr qemuProcessQmpNew(const char *binary, gid_t runGid, char **qmperr); -void qemuProcessQmpFree(qemuProcessQmpPtr cmd); +void qemuProcessQmpFree(qemuProcessQmpPtr proc); int qemuProcessQmpRun(qemuProcessQmpPtr cmd, bool forceTCG); -void qemuProcessQmpAbort(qemuProcessQmpPtr cmd); +void qemuProcessQmpAbort(qemuProcessQmpPtr proc); #endif /* __QEMU_PROCESS_H__ */ -- 2.17.1

s/qemuProcessQmpAbort/qemuProcessQmpStop/ applied to change function name used to stop QEMU processes in process code moved from qemu_capabilities. No functionality change. The new name, qemuProcessQmpStop, is consistent with the existing function qemuProcessStop used to stop Domain processes. Signed-off-by: Chris Venteicher <cventeic@redhat.com> --- src/qemu/qemu_capabilities.c | 2 +- src/qemu/qemu_process.c | 6 +++--- src/qemu/qemu_process.h | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 41a0dfa844..d903fbddf8 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -4259,7 +4259,7 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, goto cleanup; if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM)) { - qemuProcessQmpAbort(proc); + qemuProcessQmpStop(proc); if ((rc = qemuProcessQmpRun(proc, true)) != 0) { if (rc == 1) ret = 0; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 5f4853e0c4..8465448a49 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8113,7 +8113,7 @@ qemuProcessQmpFree(qemuProcessQmpPtr proc) if (!proc) return; - qemuProcessQmpAbort(proc); + qemuProcessQmpStop(proc); VIR_FREE(proc->binary); VIR_FREE(proc->monpath); VIR_FREE(proc->monarg); @@ -8249,7 +8249,7 @@ qemuProcessQmpRun(qemuProcessQmpPtr proc, cleanup: if (!proc->mon) - qemuProcessQmpAbort(proc); + qemuProcessQmpStop(proc); virObjectUnref(xmlopt); return ret; @@ -8261,7 +8261,7 @@ qemuProcessQmpRun(qemuProcessQmpPtr proc, void -qemuProcessQmpAbort(qemuProcessQmpPtr proc) +qemuProcessQmpStop(qemuProcessQmpPtr proc) { if (proc->mon) virObjectUnlock(proc->mon); diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h index dea5a84e8c..9a72db9a08 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -242,6 +242,6 @@ void qemuProcessQmpFree(qemuProcessQmpPtr proc); int qemuProcessQmpRun(qemuProcessQmpPtr cmd, bool forceTCG); -void qemuProcessQmpAbort(qemuProcessQmpPtr proc); +void qemuProcessQmpStop(qemuProcessQmpPtr proc); #endif /* __QEMU_PROCESS_H__ */ -- 2.17.1

virQEMUCapsInitQMP now stops QEMU process in all execution paths, before freeing the process structure. The qemuProcessQmpStop function can be called multiple times without problems... Won't attempt to stop processes and free resources multiple times. Follow the convention established in qemu_process of 1) alloc process structure 2) start process 3) use process 4) stop process 5) free process data structure The process data structure persists after the process activation fails or the process dies or is killed so stderr strings can be retrieved until the process data structure is freed. Signed-off-by: Chris Venteicher <cventeic@redhat.com> --- src/qemu/qemu_capabilities.c | 1 + src/qemu/qemu_process.c | 19 ++++++++++++------- 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index d903fbddf8..a79329a134 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -4273,6 +4273,7 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, ret = 0; cleanup: + qemuProcessQmpStop(proc); qemuProcessQmpFree(proc); return ret; } diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 8465448a49..fa050a1a27 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8263,14 +8263,17 @@ qemuProcessQmpRun(qemuProcessQmpPtr proc, void qemuProcessQmpStop(qemuProcessQmpPtr proc) { - if (proc->mon) + if (proc->mon) { virObjectUnlock(proc->mon); - qemuMonitorClose(proc->mon); - proc->mon = NULL; + qemuMonitorClose(proc->mon); + proc->mon = NULL; + } - virCommandAbort(proc->cmd); - virCommandFree(proc->cmd); - proc->cmd = NULL; + if (proc->cmd) { + virCommandAbort(proc->cmd); + virCommandFree(proc->cmd); + proc->cmd = NULL; + } if (proc->monpath) unlink(proc->monpath); @@ -8287,8 +8290,10 @@ qemuProcessQmpStop(qemuProcessQmpPtr proc) virStrerror(errno, ebuf, sizeof(ebuf))); VIR_FREE(*proc->qmperr); + + proc->pid = 0; } + if (proc->pidfile) unlink(proc->pidfile); - proc->pid = 0; } -- 2.17.1

In new process code, move from model where qemuProcessQmp struct can be used to activate a series of Qemu processes to model where one qemuProcessQmp struct is used for one and only one Qemu process. By allowing only one process activation per qemuProcessQmp struct, the struct can safely store process outputs like status and stderr, without being overwritten, until qemuProcessQmpFree is called. By doing this, process outputs like status and stderr can remain stored in the qemuProcessQmp struct without being overwritten by subsequent process activations. The forceTCG parameter (use / don't use KVM) will be passed when the qemuProcessQmp struct is initialized since the qemuProcessQmp struct won't be reused. Signed-off-by: Chris Venteicher <cventeic@redhat.com> --- src/qemu/qemu_capabilities.c | 20 ++++++++++++++++---- src/qemu/qemu_process.c | 10 ++++++---- src/qemu/qemu_process.h | 7 ++++--- 3 files changed, 26 insertions(+), 11 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index a79329a134..bed9ca26a1 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -4242,14 +4242,15 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, char **qmperr) { qemuProcessQmpPtr proc = NULL; + qemuProcessQmpPtr procTCG = NULL; int ret = -1; int rc; if (!(proc = qemuProcessQmpNew(qemuCaps->binary, libDir, - runUid, runGid, qmperr))) + runUid, runGid, qmperr, false))) goto cleanup; - if ((rc = qemuProcessQmpRun(proc, false)) != 0) { + if ((rc = qemuProcessQmpRun(proc)) != 0) { if (rc == 1) ret = 0; goto cleanup; @@ -4259,14 +4260,23 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, goto cleanup; if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM)) { + + /* The second QEMU process probes for TCG capabilities + * in case the first process reported KVM as enabled + * (otherwise the first one already reported TCG capabilities). */ + qemuProcessQmpStop(proc); - if ((rc = qemuProcessQmpRun(proc, true)) != 0) { + + procTCG = qemuProcessQmpNew(qemuCaps->binary, libDir, + runUid, runGid, NULL, true); + + if ((rc = qemuProcessQmpRun(procTCG)) != 0) { if (rc == 1) ret = 0; goto cleanup; } - if (virQEMUCapsInitQMPMonitorTCG(qemuCaps, proc->mon) < 0) + if (virQEMUCapsInitQMPMonitorTCG(qemuCaps, procTCG->mon) < 0) goto cleanup; } @@ -4274,7 +4284,9 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, cleanup: qemuProcessQmpStop(proc); + qemuProcessQmpStop(procTCG); qemuProcessQmpFree(proc); + qemuProcessQmpFree(procTCG); return ret; } diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index fa050a1a27..324151a7c3 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8127,7 +8127,8 @@ qemuProcessQmpNew(const char *binary, const char *libDir, uid_t runUid, gid_t runGid, - char **qmperr) + char **qmperr, + bool forceTCG) { qemuProcessQmpPtr proc = NULL; @@ -8137,9 +8138,11 @@ qemuProcessQmpNew(const char *binary, if (VIR_STRDUP(proc->binary, binary) < 0) goto error; + proc->runUid = runUid; proc->runGid = runGid; proc->qmperr = qmperr; + proc->forceTCG = forceTCG; /* the ".sock" sufix is important to avoid a possible clash with a qemu * domain called "capabilities" @@ -8178,15 +8181,14 @@ qemuProcessQmpNew(const char *binary, * 1 when probing QEMU failed */ int -qemuProcessQmpRun(qemuProcessQmpPtr proc, - bool forceTCG) +qemuProcessQmpRun(qemuProcessQmpPtr proc) { virDomainXMLOptionPtr xmlopt = NULL; const char *machine; int status = 0; int ret = -1; - if (forceTCG) + if (proc->forceTCG) machine = "none,accel=tcg"; else machine = "none,accel=kvm:tcg"; diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h index 9a72db9a08..51598b402e 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -229,18 +229,19 @@ struct _qemuProcessQmp { virDomainChrSourceDef config; pid_t pid; virDomainObjPtr vm; + bool forceTCG; }; qemuProcessQmpPtr qemuProcessQmpNew(const char *binary, const char *libDir, uid_t runUid, gid_t runGid, - char **qmperr); + char **qmperr, + bool forceTCG); void qemuProcessQmpFree(qemuProcessQmpPtr proc); -int qemuProcessQmpRun(qemuProcessQmpPtr cmd, - bool forceTCG); +int qemuProcessQmpRun(qemuProcessQmpPtr cmd); void qemuProcessQmpStop(qemuProcessQmpPtr proc); -- 2.17.1

In the past capabilities could be determined in other ways if QMP messaging didn't succeed so a non-fatal error case was included in the capabilities and QMP Process code. For a while now, QMP capabilities failure has been a fatal case. This patch makes QMP process failures return as a fatal error in all cases consistent with 1) all failures actually being fatal in QMP capabilities code and 2) the QMP process code being made generic. The process changes impact the capabilities code because non-fatal return codes are no longer returned. The rest of the QMP associated capabilities code is updated to make all errors fatal for consistency. The process changes also force a change in QMP process stderr reporting because the previous triggers for stderr reporting are no longer passed through the function interfaces. As best I can tell the only case where QMP process stderr should be explicitly reported is when the QMP process exits with a non-zero status. Error reporting is moved to virQEMUCapsInitQMP and the QMP process stderr is reported only when the QMP process status code is non-zero (and is only reported for the primary QMP query not the secondary TCG specific QMP query.) Signed-off-by: Chris Venteicher <cventeic@redhat.com> --- src/qemu/qemu_capabilities.c | 83 ++++++++++++++++-------------------- src/qemu/qemu_process.c | 24 ++++------- src/qemu/qemu_process.h | 1 + 3 files changed, 45 insertions(+), 63 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index bed9ca26a1..1efec85b57 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -4038,7 +4038,6 @@ virQEMUCapsInitQMPMonitor(virQEMUCapsPtr qemuCaps, if (qemuMonitorSetCapabilities(mon) < 0) { VIR_DEBUG("Failed to set monitor capabilities %s", virGetLastErrorMessage()); - ret = 0; goto cleanup; } @@ -4047,7 +4046,6 @@ virQEMUCapsInitQMPMonitor(virQEMUCapsPtr qemuCaps, &package) < 0) { VIR_DEBUG("Failed to query monitor version %s", virGetLastErrorMessage()); - ret = 0; goto cleanup; } @@ -4218,7 +4216,6 @@ virQEMUCapsInitQMPMonitorTCG(virQEMUCapsPtr qemuCaps ATTRIBUTE_UNUSED, if (qemuMonitorSetCapabilities(mon) < 0) { VIR_DEBUG("Failed to set monitor capabilities %s", virGetLastErrorMessage()); - ret = 0; goto cleanup; } @@ -4234,25 +4231,47 @@ virQEMUCapsInitQMPMonitorTCG(virQEMUCapsPtr qemuCaps ATTRIBUTE_UNUSED, } +#define MESSAGE_ID_CAPS_PROBE_FAILURE "8ae2f3fb-2dbe-498e-8fbd-012d40afa361" + +static void +virQEMUCapsLogProbeFailure(const char *binary) +{ + virLogMetadata meta[] = { + { .key = "MESSAGE_ID", .s = MESSAGE_ID_CAPS_PROBE_FAILURE, .iv = 0 }, + { .key = "LIBVIRT_QEMU_BINARY", .s = binary, .iv = 0 }, + { .key = NULL }, + }; + + virLogMessage(&virLogSelf, + VIR_LOG_WARN, + __FILE__, __LINE__, __func__, + meta, + _("Failed to probe capabilities for %s: %s"), + binary, virGetLastErrorMessage()); +} + + static int virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, const char *libDir, uid_t runUid, - gid_t runGid, - char **qmperr) + gid_t runGid) { qemuProcessQmpPtr proc = NULL; qemuProcessQmpPtr procTCG = NULL; + char *qmperr = NULL; int ret = -1; - int rc; if (!(proc = qemuProcessQmpNew(qemuCaps->binary, libDir, - runUid, runGid, qmperr, false))) + runUid, runGid, &qmperr, false))) goto cleanup; - if ((rc = qemuProcessQmpRun(proc)) != 0) { - if (rc == 1) - ret = 0; + if (qemuProcessQmpRun(proc) < 0) { + if (proc->status != 0) + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to probe QEMU binary with QMP: %s"), + qmperr ? qmperr : _("uknown error")); + goto cleanup; } @@ -4270,11 +4289,8 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, procTCG = qemuProcessQmpNew(qemuCaps->binary, libDir, runUid, runGid, NULL, true); - if ((rc = qemuProcessQmpRun(procTCG)) != 0) { - if (rc == 1) - ret = 0; + if (qemuProcessQmpRun(procTCG) < 0) goto cleanup; - } if (virQEMUCapsInitQMPMonitorTCG(qemuCaps, procTCG->mon) < 0) goto cleanup; @@ -4283,34 +4299,19 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, ret = 0; cleanup: + if (ret < 0) + virQEMUCapsLogProbeFailure(qemuCaps->binary); + qemuProcessQmpStop(proc); qemuProcessQmpStop(procTCG); qemuProcessQmpFree(proc); qemuProcessQmpFree(procTCG); + VIR_FREE(qmperr); + return ret; } -#define MESSAGE_ID_CAPS_PROBE_FAILURE "8ae2f3fb-2dbe-498e-8fbd-012d40afa361" - -static void -virQEMUCapsLogProbeFailure(const char *binary) -{ - virLogMetadata meta[] = { - { .key = "MESSAGE_ID", .s = MESSAGE_ID_CAPS_PROBE_FAILURE, .iv = 0 }, - { .key = "LIBVIRT_QEMU_BINARY", .s = binary, .iv = 0 }, - { .key = NULL }, - }; - - virLogMessage(&virLogSelf, - VIR_LOG_WARN, - __FILE__, __LINE__, __func__, - meta, - _("Failed to probe capabilities for %s: %s"), - binary, virGetLastErrorMessage()); -} - - virQEMUCapsPtr virQEMUCapsNewForBinaryInternal(virArch hostArch, const char *binary, @@ -4322,7 +4323,6 @@ virQEMUCapsNewForBinaryInternal(virArch hostArch, { virQEMUCapsPtr qemuCaps; struct stat sb; - char *qmperr = NULL; if (!(qemuCaps = virQEMUCapsNew())) goto error; @@ -4349,18 +4349,8 @@ virQEMUCapsNewForBinaryInternal(virArch hostArch, goto error; } - if (virQEMUCapsInitQMP(qemuCaps, libDir, runUid, runGid, &qmperr) < 0) { - virQEMUCapsLogProbeFailure(binary); + if (virQEMUCapsInitQMP(qemuCaps, libDir, runUid, runGid) < 0) goto error; - } - - if (!qemuCaps->usedQMP) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to probe QEMU binary with QMP: %s"), - qmperr ? qmperr : _("unknown error")); - virQEMUCapsLogProbeFailure(binary); - goto error; - } qemuCaps->libvirtCtime = virGetSelfLastChanged(); qemuCaps->libvirtVersion = LIBVIR_VERSION_NUMBER; @@ -4376,7 +4366,6 @@ virQEMUCapsNewForBinaryInternal(virArch hostArch, } cleanup: - VIR_FREE(qmperr); return qemuCaps; error: diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 324151a7c3..2ec0d5340d 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8176,16 +8176,11 @@ qemuProcessQmpNew(const char *binary, } -/* Returns -1 on fatal error, - * 0 on success, - * 1 when probing QEMU failed - */ int qemuProcessQmpRun(qemuProcessQmpPtr proc) { virDomainXMLOptionPtr xmlopt = NULL; const char *machine; - int status = 0; int ret = -1; if (proc->forceTCG) @@ -8220,19 +8215,20 @@ qemuProcessQmpRun(qemuProcessQmpPtr proc) virCommandSetErrorBuffer(proc->cmd, proc->qmperr); - /* Log, but otherwise ignore, non-zero status. */ - if (virCommandRun(proc->cmd, &status) < 0) + proc->status = 0; + + if (virCommandRun(proc->cmd, &proc->status) < 0) goto cleanup; - if (status != 0) { + if (proc->status != 0) { VIR_DEBUG("QEMU %s exited with status %d: %s", - proc->binary, status, *proc->qmperr); - goto ignore; + proc->binary, proc->status, *proc->qmperr); + goto cleanup; } if (virPidFileReadPath(proc->pidfile, &proc->pid) < 0) { VIR_DEBUG("Failed to read pidfile %s", proc->pidfile); - goto ignore; + goto cleanup; } if (!(xmlopt = virDomainXMLOptionNew(NULL, NULL, NULL, NULL, NULL)) || @@ -8243,7 +8239,7 @@ qemuProcessQmpRun(qemuProcessQmpPtr proc) if (!(proc->mon = qemuMonitorOpen(proc->vm, &proc->config, true, true, 0, &callbacks, NULL))) - goto ignore; + goto cleanup; virObjectLock(proc->mon); @@ -8255,10 +8251,6 @@ qemuProcessQmpRun(qemuProcessQmpPtr proc) virObjectUnref(xmlopt); return ret; - - ignore: - ret = 1; - goto cleanup; } diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h index 51598b402e..a56e9608cd 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -220,6 +220,7 @@ struct _qemuProcessQmp { char *binary; uid_t runUid; gid_t runGid; + int status; char **qmperr; char *monarg; char *monpath; -- 2.17.1

A qemuProcessQmp struct tracks the entire lifespan of a single QEMU Process including storing error output when the process terminates or activation fails. Error output remains available until qemuProcessQmpFree is called. The qmperr variable is renamed stderr (captures stderr from process.) The stderr buffer no longer needs to be maintained outside of the qemuProcessQmp structure because the structure is used for a single QEMU process and the structures can be maintained as long as required to retrieve the process error info. Signed-off-by: Chris Venteicher <cventeic@redhat.com> --- src/qemu/qemu_capabilities.c | 8 +++----- src/qemu/qemu_process.c | 9 +++------ src/qemu/qemu_process.h | 3 +-- 3 files changed, 7 insertions(+), 13 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 1efec85b57..997f8c19d5 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -4259,18 +4259,17 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, { qemuProcessQmpPtr proc = NULL; qemuProcessQmpPtr procTCG = NULL; - char *qmperr = NULL; int ret = -1; if (!(proc = qemuProcessQmpNew(qemuCaps->binary, libDir, - runUid, runGid, &qmperr, false))) + runUid, runGid, false))) goto cleanup; if (qemuProcessQmpRun(proc) < 0) { if (proc->status != 0) virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to probe QEMU binary with QMP: %s"), - qmperr ? qmperr : _("uknown error")); + proc->stderr ? proc->stderr : _("uknown error")); goto cleanup; } @@ -4287,7 +4286,7 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, qemuProcessQmpStop(proc); procTCG = qemuProcessQmpNew(qemuCaps->binary, libDir, - runUid, runGid, NULL, true); + runUid, runGid, true); if (qemuProcessQmpRun(procTCG) < 0) goto cleanup; @@ -4306,7 +4305,6 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, qemuProcessQmpStop(procTCG); qemuProcessQmpFree(proc); qemuProcessQmpFree(procTCG); - VIR_FREE(qmperr); return ret; } diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 2ec0d5340d..8ad685f3ea 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8118,6 +8118,7 @@ qemuProcessQmpFree(qemuProcessQmpPtr proc) VIR_FREE(proc->monpath); VIR_FREE(proc->monarg); VIR_FREE(proc->pidfile); + VIR_FREE(proc->stderr); VIR_FREE(proc); } @@ -8127,7 +8128,6 @@ qemuProcessQmpNew(const char *binary, const char *libDir, uid_t runUid, gid_t runGid, - char **qmperr, bool forceTCG) { qemuProcessQmpPtr proc = NULL; @@ -8141,7 +8141,6 @@ qemuProcessQmpNew(const char *binary, proc->runUid = runUid; proc->runGid = runGid; - proc->qmperr = qmperr; proc->forceTCG = forceTCG; /* the ".sock" sufix is important to avoid a possible clash with a qemu @@ -8213,7 +8212,7 @@ qemuProcessQmpRun(qemuProcessQmpPtr proc) virCommandSetGID(proc->cmd, proc->runGid); virCommandSetUID(proc->cmd, proc->runUid); - virCommandSetErrorBuffer(proc->cmd, proc->qmperr); + virCommandSetErrorBuffer(proc->cmd, &(proc->stderr)); proc->status = 0; @@ -8222,7 +8221,7 @@ qemuProcessQmpRun(qemuProcessQmpPtr proc) if (proc->status != 0) { VIR_DEBUG("QEMU %s exited with status %d: %s", - proc->binary, proc->status, *proc->qmperr); + proc->binary, proc->status, proc->stderr); goto cleanup; } @@ -8283,8 +8282,6 @@ qemuProcessQmpStop(qemuProcessQmpPtr proc) (long long)proc->pid, virStrerror(errno, ebuf, sizeof(ebuf))); - VIR_FREE(*proc->qmperr); - proc->pid = 0; } diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h index a56e9608cd..3ddfa82918 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -221,7 +221,7 @@ struct _qemuProcessQmp { uid_t runUid; gid_t runGid; int status; - char **qmperr; + char *stderr; char *monarg; char *monpath; char *pidfile; @@ -237,7 +237,6 @@ qemuProcessQmpPtr qemuProcessQmpNew(const char *binary, const char *libDir, uid_t runUid, gid_t runGid, - char **qmperr, bool forceTCG); void qemuProcessQmpFree(qemuProcessQmpPtr proc); -- 2.17.1

Move a step closer to the function structure used elsewhere in qemu_process where qemuProcessStart and qemuProcessStop are the exposed functions. qemuProcessQmpStart mirrors qemuProcessStart in calling sub functions to initialize, launch the process and connect the monitor to the QEMU process. static functions qemuProcessQmpInit, qemuProcessQmpLaunch and qemuProcessQmpConnectMonitor are introduced. qemuProcessQmpLaunch is just renamed from qemuProcessQmpRun and encapsulates all of the original code. qemuProcessQmpInit and qemuProcessQmpMonitor are nearly empty functions acting as placeholders for later patches where blocks of semi-complicated code are cut/pasted into these functions without modification (hopefully making review easier.) Looking forward, the patch series ultimately moves the code into this partitioning: - qemuProcessQmpInit Becomes the location of ~25 lines of code to create storage directory, in thread safe way, and initialize paths for monpath, monarg and pidfile. - qemuProcessQmpLaunch Becomes the location of ~48 lines of code used to create and run the QEMU command. - qemuProcessQmpConnectMonitor Becomes the final location of ~58 lines of code used to open and initialize the monitor connection between libvirt and qemu. Three smaller, purpose-identifying, functions of ~60 lines or less seem better than a single large process "start" function of > 130 lines. Being able to compare and contrast between the domain and non-domain versions of process code is useful too. There is some significant overlap between what the non-domain and domain functions do. There is also significant additional functionality in the domain functions that might be useful in the non-domain functions in the future. Possibly there could be sharing between non-domain and domain process code in the future but common code would have to be carefully extracted from the domain process code (not trivial.) Mirroring the domain process code has some value, but partitioning the code into logical chunks of < 60 lines is the main reason for the static functions. Signed-off-by: Chris Venteicher <cventeic@redhat.com> --- src/qemu/qemu_capabilities.c | 4 +- src/qemu/qemu_process.c | 94 +++++++++++++++++++++++++++++++++++- src/qemu/qemu_process.h | 2 +- 3 files changed, 95 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 997f8c19d5..ce60648897 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -4265,7 +4265,7 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, runUid, runGid, false))) goto cleanup; - if (qemuProcessQmpRun(proc) < 0) { + if (qemuProcessQmpStart(proc) < 0) { if (proc->status != 0) virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to probe QEMU binary with QMP: %s"), @@ -4288,7 +4288,7 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, procTCG = qemuProcessQmpNew(qemuCaps->binary, libDir, runUid, runGid, true); - if (qemuProcessQmpRun(procTCG) < 0) + if (qemuProcessQmpStart(procTCG) < 0) goto cleanup; if (virQEMUCapsInitQMPMonitorTCG(qemuCaps, procTCG->mon) < 0) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 8ad685f3ea..938d328235 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8175,8 +8175,28 @@ qemuProcessQmpNew(const char *binary, } -int -qemuProcessQmpRun(qemuProcessQmpPtr proc) +/* Initialize configuration and paths prior to starting QEMU + */ +static int +qemuProcessQmpInit(qemuProcessQmpPtr proc) +{ + int ret = -1; + + VIR_DEBUG("Beginning VM startup process" + " proc=%p, emulator=%s", + proc, proc->binary); + + ret = 0; + + VIR_DEBUG("ret=%i", ret); + return ret; +} + + +/* Launch QEMU Process + */ +static int +qemuProcessQmpLaunch(qemuProcessQmpPtr proc) { virDomainXMLOptionPtr xmlopt = NULL; const char *machine; @@ -8253,6 +8273,76 @@ qemuProcessQmpRun(qemuProcessQmpPtr proc) } +/* Connect Monitor to QEMU Process + */ +static int +qemuProcessQmpConnectMonitor(qemuProcessQmpPtr proc) +{ + int ret = -1; + + VIR_DEBUG("proc=%p, emulator=%s, proc->pid=%lld", + proc, NULLSTR(proc->binary), (long long)proc->pid); + + ret = 0; + + VIR_DEBUG("ret=%i", ret); + return ret; +} + + +/** + * qemuProcessQmpStart: + * @proc: Stores Process and Connection State + * + * Start and connect to QEMU binary so QMP queries can be made. + * + * Usage: + * proc = qemuProcessQmpNew(binary, libDir, runUid, runGid, forceTCG); + * qemuProcessQmpStart(proc); + * ** Send QMP Queries to QEMU using monitor (proc->mon) ** + * qemuProcessQmpStop(proc); + * qemuProcessQmpFree(proc); + * + * Check monitor is not NULL before using. + * + * QEMU probing failure results in monitor being NULL but is not considered + * an error case and does not result in a negative return value. + * + * Both qemuProcessQmpStop and qemuProcessQmpFree must be called to cleanup and + * free resources, even in activation failure cases. + * + * Process error output (proc->qmperr) remains available in qemuProcessQmp + * struct until qemuProcessQmpFree is called. + */ +int +qemuProcessQmpStart(qemuProcessQmpPtr proc) +{ + int ret = -1; + + VIR_DEBUG("proc=%p, emulator=%s", + proc, proc->binary); + + if (qemuProcessQmpInit(proc) < 0) + goto cleanup; + + if (qemuProcessQmpLaunch(proc) < 0) + goto stop; + + if (qemuProcessQmpConnectMonitor(proc) < 0) + goto stop; + + ret = 0; + + cleanup: + VIR_DEBUG("ret=%i", ret); + return ret; + + stop: + qemuProcessQmpStop(proc); + goto cleanup; +} + + void qemuProcessQmpStop(qemuProcessQmpPtr proc) { diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h index 3ddfa82918..1b64aeef4e 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -241,7 +241,7 @@ qemuProcessQmpPtr qemuProcessQmpNew(const char *binary, void qemuProcessQmpFree(qemuProcessQmpPtr proc); -int qemuProcessQmpRun(qemuProcessQmpPtr cmd); +int qemuProcessQmpStart(qemuProcessQmpPtr proc); void qemuProcessQmpStop(qemuProcessQmpPtr proc); -- 2.17.1

Multiple QEMU processes for QMP commands can operate concurrently. Use a unique directory under libDir for each QEMU processes to avoid pidfile and unix socket collision between processes. The pid file name is changed from "capabilities.pidfile" to "qmp.pid" because we no longer need to avoid a possible clash with a qemu domain called "capabilities" now that the processes artifacts are stored in their own unique temporary directories. "Capabilities" was changed to "qmp" in the pid file name because these processes are no longer specific to the capabilities usecase and are more generic in terms of being used for any general purpose QMP message exchanges with a QEMU process that is not associated with a domain. Signed-off-by: Chris Venteicher <cventeic@redhat.com> --- src/qemu/qemu_process.c | 24 ++++++++++++++---------- src/qemu/qemu_process.h | 1 + 2 files changed, 15 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 80b938cc0b..26ba59143d 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8119,6 +8119,7 @@ qemuProcessQmpFree(qemuProcessQmpPtr proc) VIR_FREE(proc->binary); VIR_FREE(proc->libDir); + VIR_FREE(proc->uniqDir); VIR_FREE(proc->monpath); VIR_FREE(proc->monarg); VIR_FREE(proc->pidfile); @@ -8181,33 +8182,33 @@ qemuProcessQmpNew(const char *binary, static int qemuProcessQmpInit(qemuProcessQmpPtr proc) { + char *template = NULL; int ret = -1; VIR_DEBUG("Beginning VM startup process" " proc=%p, emulator=%s", proc, proc->binary); - /* the ".sock" sufix is important to avoid a possible clash with a qemu - * domain called "capabilities" - */ - if (virAsprintf(&proc->monpath, "%s/%s", proc->libDir, - "capabilities.monitor.sock") < 0) + if (virAsprintf(&template, "%s/qemu.XXXXXX", proc->libDir) < 0) + goto cleanup; + + proc->uniqDir = mkdtemp(template); + + if (virAsprintf(&proc->monpath, "%s/%s", proc->uniqDir, + "qmp.monitor") < 0) goto cleanup; if (virAsprintf(&proc->monarg, "unix:%s,server,nowait", proc->monpath) < 0) goto cleanup; - /* ".pidfile" suffix is used rather than ".pid" to avoid a possible clash - * with a qemu domain called "capabilities" + /* * Normally we'd use runDir for pid files, but because we're using * -daemonize we need QEMU to be allowed to create them, rather * than libvirtd. So we're using libDir which QEMU can write to */ - if (virAsprintf(&proc->pidfile, "%s/%s", proc->libDir, "capabilities.pidfile") < 0) + if (virAsprintf(&proc->pidfile, "%s/%s", proc->uniqDir, "qmp.pid") < 0) goto cleanup; - virPidFileForceCleanupPath(proc->pidfile); - ret = 0; cleanup: @@ -8446,4 +8447,7 @@ void qemuProcessQmpStop(qemuProcessQmpPtr proc) if (proc->pidfile) unlink(proc->pidfile); + + if (proc->uniqDir) + rmdir(proc->uniqDir); } diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h index 0a79d15290..2ef521d825 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -226,6 +226,7 @@ struct _qemuProcessQmp { char *monarg; char *monpath; char *pidfile; + char *uniqDir; virCommandPtr cmd; qemuMonitorPtr mon; pid_t pid; -- 2.17.1

Locking the monitor object immediately after call to qemuMonitorOpen doesn't make sense now that we have expanded the QEMU process code to cover more than the original capabilities usecase. Removing the monitor lock makes the qemuProcessQmpConnectMonitor code consistent with the qemuConnectMonitor code used to establish the monitor when QEMU process is started for domains. Signed-off-by: Chris Venteicher <cventeic@redhat.com> --- src/qemu/qemu_process.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 26ba59143d..b491f9f91a 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8331,8 +8331,6 @@ qemuProcessQmpConnectMonitor(qemuProcessQmpPtr proc) VIR_STEAL_PTR(proc->mon, mon); - virObjectLock(proc->mon); - /* Exit capabilities negotiation mode and enter QEMU command mode * by issuing qmp_capabilities command to QEMU */ if (qemuMonitorSetCapabilities(proc->mon) < 0) { -- 2.17.1

Use a helper function to allocate and initializes CPU Model Info structs. Signed-off-by: Chris Venteicher <cventeic@redhat.com> --- src/qemu/qemu_capabilities.c | 2 +- src/qemu/qemu_monitor.c | 32 +++++++++++++++++++++++++++----- src/qemu/qemu_monitor.h | 2 ++ 3 files changed, 30 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 038e3ecf7a..cd685298e6 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -3047,7 +3047,7 @@ virQEMUCapsLoadHostCPUModelInfo(virQEMUCapsPtr qemuCaps, goto cleanup; } - if (VIR_ALLOC(hostCPU) < 0) + if (!(hostCPU = qemuMonitorCPUModelInfoNew(NULL))) goto cleanup; if (!(hostCPU->name = virXMLPropString(hostCPUNode, "model"))) { diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 84065c59dc..5effc74736 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -3670,6 +3670,31 @@ qemuMonitorGetCPUModelExpansion(qemuMonitorPtr mon, } +qemuMonitorCPUModelInfoPtr +qemuMonitorCPUModelInfoNew(const char *name) +{ + qemuMonitorCPUModelInfoPtr ret = NULL; + qemuMonitorCPUModelInfoPtr model; + + if (VIR_ALLOC(model) < 0) + return NULL; + + model->name = NULL; + model->nprops = 0; + model->props = NULL; + model->migratability = false; + + if (VIR_STRDUP(model->name, name) < 0) + goto cleanup; + + VIR_STEAL_PTR(ret, model); + + cleanup: + qemuMonitorCPUModelInfoFree(model); + return ret; +} + + void qemuMonitorCPUModelInfoFree(qemuMonitorCPUModelInfoPtr model_info) { @@ -3693,18 +3718,15 @@ qemuMonitorCPUModelInfoFree(qemuMonitorCPUModelInfoPtr model_info) qemuMonitorCPUModelInfoPtr qemuMonitorCPUModelInfoCopy(const qemuMonitorCPUModelInfo *orig) { - qemuMonitorCPUModelInfoPtr copy; + qemuMonitorCPUModelInfoPtr copy = NULL; size_t i; - if (VIR_ALLOC(copy) < 0) + if (!orig || !(copy = qemuMonitorCPUModelInfoNew(orig->name))) goto error; if (VIR_ALLOC_N(copy->props, orig->nprops) < 0) goto error; - if (VIR_STRDUP(copy->name, orig->name) < 0) - goto error; - copy->migratability = orig->migratability; copy->nprops = orig->nprops; diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 66bfdb0e5c..d486af201a 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -1042,6 +1042,8 @@ int qemuMonitorGetCPUModelExpansion(qemuMonitorPtr mon, void qemuMonitorCPUModelInfoFree(qemuMonitorCPUModelInfoPtr model_info); +qemuMonitorCPUModelInfoPtr qemuMonitorCPUModelInfoNew(const char *name); + qemuMonitorCPUModelInfoPtr qemuMonitorCPUModelInfoCopy(const qemuMonitorCPUModelInfo *orig); -- 2.17.1

Conversion functions are used convert CPUModelInfo structs into QMP JSON and the reverse. QMP JSON is of form: {"model": {"name": "IvyBridge", "props": {}}} qemuMonitorCPUModelInfoBoolPropAdd is used to add boolean properties to CPUModelInfo struct. qemuMonitorJSONGetCPUModelExpansion makes full use of conversions and propAdd in prep to support input of full cpu model in future. Signed-off-by: Chris Venteicher <cventeic@redhat.com> --- src/qemu/qemu_monitor.c | 24 ++++++ src/qemu/qemu_monitor.h | 5 ++ src/qemu/qemu_monitor_json.c | 154 +++++++++++++++++++++++++---------- 3 files changed, 138 insertions(+), 45 deletions(-) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 5effc74736..ddf4d96799 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -3764,6 +3764,30 @@ qemuMonitorCPUModelInfoCopy(const qemuMonitorCPUModelInfo *orig) } +int +qemuMonitorCPUModelInfoBoolPropAdd(qemuMonitorCPUModelInfoPtr model, + const char *prop_name, + bool prop_value) +{ + int ret = -1; + qemuMonitorCPUProperty prop; + prop.type = QEMU_MONITOR_CPU_PROPERTY_BOOLEAN; + prop.value.boolean = prop_value; + + if (VIR_STRDUP(prop.name, prop_name) < 0) + goto cleanup; + + if (VIR_APPEND_ELEMENT(model->props, model->nprops, prop) < 0) + goto cleanup; + + ret = 0; + + cleanup: + VIR_FREE(prop.name); + return ret; +} + + int qemuMonitorGetCommands(qemuMonitorPtr mon, char ***commands) diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index d486af201a..d0c54f60a9 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -1047,6 +1047,11 @@ qemuMonitorCPUModelInfoPtr qemuMonitorCPUModelInfoNew(const char *name); qemuMonitorCPUModelInfoPtr qemuMonitorCPUModelInfoCopy(const qemuMonitorCPUModelInfo *orig); +int qemuMonitorCPUModelInfoBoolPropAdd(qemuMonitorCPUModelInfoPtr model, + const char *prop_name, + bool prop_value) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); + int qemuMonitorGetCommands(qemuMonitorPtr mon, char ***commands); int qemuMonitorGetEvents(qemuMonitorPtr mon, diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 5a806f6c0e..abfa4155ee 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -5505,6 +5505,101 @@ qemuMonitorJSONParseCPUModelProperty(const char *key, return 0; } + +/* model_json: {"name": "z13-base", "props": {}} + */ +static virJSONValuePtr +qemuMonitorJSONBuildCPUModelInfoToJSON(qemuMonitorCPUModelInfoPtr model) +{ + virJSONValuePtr cpu_props = NULL; + virJSONValuePtr model_json = NULL; + size_t i; + + if (!model) + goto cleanup; + + if (model->nprops > 0 && !(cpu_props = virJSONValueNewObject())) + goto cleanup; + + for (i = 0; i < model->nprops; i++) { + qemuMonitorCPUPropertyPtr prop = &(model->props[i]); + + switch (prop->type) { + case QEMU_MONITOR_CPU_PROPERTY_BOOLEAN: + if (virJSONValueObjectAppendBoolean(cpu_props, prop->name, + prop->value.boolean) < 0) + goto cleanup; + break; + + case QEMU_MONITOR_CPU_PROPERTY_STRING: + if (virJSONValueObjectAppendString(cpu_props, prop->name, + prop->value.string) < 0) + goto cleanup; + break; + + case QEMU_MONITOR_CPU_PROPERTY_NUMBER: + if (virJSONValueObjectAppendNumberLong(cpu_props, prop->name, + prop->value.number) < 0) + goto cleanup; + break; + + case QEMU_MONITOR_CPU_PROPERTY_LAST: + default: + virReportEnumRangeError(qemuMonitorCPUPropertyPtr, prop->type); + goto cleanup; + } + } + + ignore_value(virJSONValueObjectCreate(&model_json, "s:name", model->name, + "A:props", &cpu_props, NULL)); + + cleanup: + virJSONValueFree(cpu_props); + return model_json; +} + + +/* model_json: {"name": "IvyBridge", "props": {}} + */ +static qemuMonitorCPUModelInfoPtr +qemuMonitorJSONBuildCPUModelInfoFromJSON(virJSONValuePtr cpu_model) +{ + virJSONValuePtr cpu_props; + qemuMonitorCPUModelInfoPtr model = NULL; + qemuMonitorCPUModelInfoPtr ret = NULL; + char const *cpu_name; + + if (!(cpu_name = virJSONValueObjectGetString(cpu_model, "name"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Parsed JSON reply missing 'name'")); + goto cleanup; + } + + if (VIR_ALLOC(model) < 0) + goto cleanup; + + if (VIR_STRDUP(model->name, cpu_name) < 0) + goto cleanup; + + if ((cpu_props = virJSONValueObjectGetObject(cpu_model, "props"))) { + if (VIR_ALLOC_N(model->props, + virJSONValueObjectKeysNumber(cpu_props)) < 0) + goto cleanup; + + if (virJSONValueObjectForeachKeyValue(cpu_props, + qemuMonitorJSONParseCPUModelProperty, + model) < 0) + goto cleanup; + } + + VIR_STEAL_PTR(ret, model); + + cleanup: + qemuMonitorCPUModelInfoFree(model); + + return ret; +} + int qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr mon, qemuMonitorCPUModelExpansionType type, @@ -5513,32 +5608,25 @@ qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr mon, qemuMonitorCPUModelInfoPtr *model_info) { int ret = -1; - virJSONValuePtr model = NULL; - virJSONValuePtr props = NULL; + virJSONValuePtr json_model_in = NULL; virJSONValuePtr cmd = NULL; virJSONValuePtr reply = NULL; virJSONValuePtr data; virJSONValuePtr cpu_model; - virJSONValuePtr cpu_props; - qemuMonitorCPUModelInfoPtr machine_model = NULL; - char const *cpu_name; + qemuMonitorCPUModelInfoPtr model_in = NULL; const char *typeStr = ""; *model_info = NULL; - if (!(model = virJSONValueNewObject())) + if (!(model_in = qemuMonitorCPUModelInfoNew(model_name))) goto cleanup; - if (virJSONValueObjectAppendString(model, "name", model_name) < 0) + if (!migratable && + qemuMonitorCPUModelInfoBoolPropAdd(model_in, "migratable", false) < 0) goto cleanup; - if (!migratable) { - if (!(props = virJSONValueNewObject()) || - virJSONValueObjectAppendBoolean(props, "migratable", false) < 0 || - virJSONValueObjectAppend(model, "props", props) < 0) - goto cleanup; - props = NULL; - } + if (!(json_model_in = qemuMonitorJSONBuildCPUModelInfoToJSON(model_in))) + goto cleanup; retry: switch (type) { @@ -5554,7 +5642,7 @@ qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr mon, if (!(cmd = qemuMonitorJSONMakeCommand("query-cpu-model-expansion", "s:type", typeStr, - "a:model", &model, + "a:model", &json_model_in, NULL))) goto cleanup; @@ -5585,7 +5673,9 @@ qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr mon, * on the result of the initial "static" expansion. */ if (type == QEMU_MONITOR_CPU_MODEL_EXPANSION_STATIC_FULL) { - if (!(model = virJSONValueCopy(cpu_model))) + virJSONValueFree(json_model_in); + + if (!(json_model_in = virJSONValueCopy(cpu_model))) goto cleanup; virJSONValueFree(cmd); @@ -5594,42 +5684,16 @@ qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr mon, goto retry; } - if (!(cpu_name = virJSONValueObjectGetString(cpu_model, "name"))) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("query-cpu-model-expansion reply data was missing 'name'")); - goto cleanup; - } - - if (!(cpu_props = virJSONValueObjectGetObject(cpu_model, "props"))) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("query-cpu-model-expansion reply data was missing 'props'")); - goto cleanup; - } - - if (VIR_ALLOC(machine_model) < 0) - goto cleanup; - - if (VIR_STRDUP(machine_model->name, cpu_name) < 0) - goto cleanup; - - if (VIR_ALLOC_N(machine_model->props, virJSONValueObjectKeysNumber(cpu_props)) < 0) - goto cleanup; - - if (virJSONValueObjectForeachKeyValue(cpu_props, - qemuMonitorJSONParseCPUModelProperty, - machine_model) < 0) + if (!(*model_info = qemuMonitorJSONBuildCPUModelInfoFromJSON(cpu_model))) goto cleanup; ret = 0; - *model_info = machine_model; - machine_model = NULL; cleanup: - qemuMonitorCPUModelInfoFree(machine_model); + qemuMonitorCPUModelInfoFree(model_in); virJSONValueFree(cmd); virJSONValueFree(reply); - virJSONValueFree(model); - virJSONValueFree(props); + virJSONValueFree(json_model_in); return ret; } -- 2.17.1

Create an augmented CPUModelInfo identifying which props are/aren't migratable based on a diff between migratable and non-migratable inputs. This patch pulls existing logic out of virQEMUCapsProbeQMPHostCPU and wraps the existing logic in a standalone function hopefully simplifying both functions and making the inputs and outputs clearer. The patch also sets cpuData->info = NULL to make sure bad data does not remain in failure cases. Q) Can the right people quickly determine if they should review this? Signed-off-by: Chris Venteicher <cventeic@redhat.com> --- src/qemu/qemu_capabilities.c | 131 ++++++++++++++++++++++++----------- 1 file changed, 92 insertions(+), 39 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index cd685298e6..bd75f82e70 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -2378,14 +2378,91 @@ virQEMUCapsProbeQMPCPUDefinitions(virQEMUCapsPtr qemuCaps, } +/* virQEMUCapsMigratablePropsDiff + * @migratable: input where all migratable props have value true + * and non-migratable and unsupported props have value false + * + * @nonMigratable: input where all migratable & non-migratable props + * have value true and unsupported props have value false + * + * @augmented: output including all props listed in @migratable but + * both migratable & non-migratable props have value true, + * unsupported props have value false, + * and prop->migratable is set to VIR_TRISTATE_BOOL_{YES/NO} + * for each supported prop + * + * Differences in expanded CPUModelInfo inputs @migratable and @nonMigratable are + * used to create output @augmented where individual props have prop->migratable + * set to indicate if prop is or isn't migratable. + */ +static int +virQEMUCapsMigratablePropsDiff(qemuMonitorCPUModelInfoPtr migratable, + qemuMonitorCPUModelInfoPtr nonMigratable, + qemuMonitorCPUModelInfoPtr *augmented) +{ + int ret = -1; + qemuMonitorCPUModelInfoPtr tmp; + qemuMonitorCPUPropertyPtr prop; + qemuMonitorCPUPropertyPtr mProp; + qemuMonitorCPUPropertyPtr nmProp; + virHashTablePtr hash = NULL; + size_t i; + + *augmented = NULL; + + if (!(tmp = qemuMonitorCPUModelInfoCopy(migratable))) + goto cleanup; + + if (!nonMigratable) + goto done; + + if (!(hash = virHashCreate(0, NULL))) + goto cleanup; + + for (i = 0; i < tmp->nprops; i++) { + prop = tmp->props + i; + + if (virHashAddEntry(hash, prop->name, prop) < 0) + goto cleanup; + } + + for (i = 0; i < nonMigratable->nprops; i++) { + nmProp = nonMigratable->props + i; + + if (!(mProp = virHashLookup(hash, nmProp->name)) || + mProp->type != QEMU_MONITOR_CPU_PROPERTY_BOOLEAN || + mProp->type != nmProp->type) + continue; /* In non-migratable list but not in migratable list */ + + if (mProp->value.boolean) { + mProp->migratable = VIR_TRISTATE_BOOL_YES; + } else if (nmProp->value.boolean) { + mProp->value.boolean = true; + mProp->migratable = VIR_TRISTATE_BOOL_NO; + } + } + + tmp->migratability = true; + + done: + VIR_STEAL_PTR(*augmented, tmp); + ret = 0; + + cleanup: + qemuMonitorCPUModelInfoFree(tmp); + virHashFree(hash); + return ret; +} + + static int virQEMUCapsProbeQMPHostCPU(virQEMUCapsPtr qemuCaps, qemuMonitorPtr mon, bool tcg) { - qemuMonitorCPUModelInfoPtr modelInfo = NULL; + qemuMonitorCPUModelInfoPtr migratable = NULL; qemuMonitorCPUModelInfoPtr nonMigratable = NULL; - virHashTablePtr hash = NULL; + qemuMonitorCPUModelInfoPtr augmented = NULL; const char *model; qemuMonitorCPUModelExpansionType type; virDomainVirtType virtType; @@ -2404,6 +2481,7 @@ virQEMUCapsProbeQMPHostCPU(virQEMUCapsPtr qemuCaps, } cpuData = virQEMUCapsGetHostCPUData(qemuCaps, virtType); + cpuData->info = NULL; /* Some x86_64 features defined in cpu_map.xml use spelling which differ * from the one preferred by QEMU. Static expansion would give us only the @@ -2415,54 +2493,29 @@ virQEMUCapsProbeQMPHostCPU(virQEMUCapsPtr qemuCaps, else type = QEMU_MONITOR_CPU_MODEL_EXPANSION_STATIC; - if (qemuMonitorGetCPUModelExpansion(mon, type, model, true, &modelInfo) < 0) + if (qemuMonitorGetCPUModelExpansion(mon, type, model, true, &migratable) < 0) goto cleanup; + if (!migratable) { + ret = 0; /* Qemu can't expand the model name, exit without error */ + goto cleanup; + } + /* Try to check migratability of each feature. */ - if (modelInfo && - qemuMonitorGetCPUModelExpansion(mon, type, model, false, + if (qemuMonitorGetCPUModelExpansion(mon, type, model, false, &nonMigratable) < 0) goto cleanup; - if (nonMigratable) { - qemuMonitorCPUPropertyPtr prop; - qemuMonitorCPUPropertyPtr nmProp; - size_t i; + if (virQEMUCapsMigratablePropsDiff(migratable, nonMigratable, &augmented) < 0) + goto cleanup; - if (!(hash = virHashCreate(0, NULL))) - goto cleanup; - - for (i = 0; i < modelInfo->nprops; i++) { - prop = modelInfo->props + i; - if (virHashAddEntry(hash, prop->name, prop) < 0) - goto cleanup; - } - - for (i = 0; i < nonMigratable->nprops; i++) { - nmProp = nonMigratable->props + i; - if (!(prop = virHashLookup(hash, nmProp->name)) || - prop->type != QEMU_MONITOR_CPU_PROPERTY_BOOLEAN || - prop->type != nmProp->type) - continue; - - if (prop->value.boolean) { - prop->migratable = VIR_TRISTATE_BOOL_YES; - } else if (nmProp->value.boolean) { - prop->value.boolean = true; - prop->migratable = VIR_TRISTATE_BOOL_NO; - } - } - - modelInfo->migratability = true; - } - - VIR_STEAL_PTR(cpuData->info, modelInfo); + VIR_STEAL_PTR(cpuData->info, augmented); ret = 0; cleanup: - virHashFree(hash); + qemuMonitorCPUModelInfoFree(migratable); qemuMonitorCPUModelInfoFree(nonMigratable); - qemuMonitorCPUModelInfoFree(modelInfo); + qemuMonitorCPUModelInfoFree(augmented); return ret; } -- 2.17.1

A Full CPUModelInfo structure with props is sent to QEMU for expansion. virQEMUCapsProbeQMPHostCPU migratability logic partitioned into new function for clarity. Signed-off-by: Chris Venteicher <cventeic@redhat.com> --- src/qemu/qemu_capabilities.c | 8 +++++--- src/qemu/qemu_monitor.c | 31 ++++++++++++++++++++++++++----- src/qemu/qemu_monitor.h | 5 +++-- src/qemu/qemu_monitor_json.c | 16 +++++++++++----- src/qemu/qemu_monitor_json.h | 6 +++--- tests/cputest.c | 11 ++++++++--- 6 files changed, 56 insertions(+), 21 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index bd75f82e70..8c5ec4cc9a 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -2460,6 +2460,7 @@ virQEMUCapsProbeQMPHostCPU(virQEMUCapsPtr qemuCaps, qemuMonitorPtr mon, bool tcg) { + qemuMonitorCPUModelInfoPtr input; qemuMonitorCPUModelInfoPtr migratable = NULL; qemuMonitorCPUModelInfoPtr nonMigratable = NULL; qemuMonitorCPUModelInfoPtr augmented = NULL; @@ -2493,7 +2494,8 @@ virQEMUCapsProbeQMPHostCPU(virQEMUCapsPtr qemuCaps, else type = QEMU_MONITOR_CPU_MODEL_EXPANSION_STATIC; - if (qemuMonitorGetCPUModelExpansion(mon, type, model, true, &migratable) < 0) + if (!(input = qemuMonitorCPUModelInfoNew(model)) || + qemuMonitorGetCPUModelExpansion(mon, type, true, input, &migratable) < 0) goto cleanup; if (!migratable) { @@ -2502,8 +2504,7 @@ virQEMUCapsProbeQMPHostCPU(virQEMUCapsPtr qemuCaps, } /* Try to check migratability of each feature. */ - if (qemuMonitorGetCPUModelExpansion(mon, type, model, false, - &nonMigratable) < 0) + if (qemuMonitorGetCPUModelExpansion(mon, type, false, input, &nonMigratable) < 0) goto cleanup; if (virQEMUCapsMigratablePropsDiff(migratable, nonMigratable, &augmented) < 0) @@ -2513,6 +2514,7 @@ virQEMUCapsProbeQMPHostCPU(virQEMUCapsPtr qemuCaps, ret = 0; cleanup: + qemuMonitorCPUModelInfoFree(input); qemuMonitorCPUModelInfoFree(migratable); qemuMonitorCPUModelInfoFree(nonMigratable); qemuMonitorCPUModelInfoFree(augmented); diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index ddf4d96799..bed6a2f90a 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -3653,20 +3653,41 @@ qemuMonitorCPUDefInfoFree(qemuMonitorCPUDefInfoPtr cpu) } +/** + * qemuMonitorGetCPUModelExpansion: + * @mon: + * @type: qemuMonitorCPUModelExpansionType + * @migratable: Prompt QEMU to include non-migratable props for X86 models if false + * @input: Input model + * @expansion: Expanded output model (or NULL if QEMU rejects model or request) + * + * Re-represent @input CPU props using a new CPUModelInfo constructed + * by naming a STATIC or DYNAMIC model cooresponding to a set of properties and + * a FULL or PARTIAL (only deltas from model) property list. + * + * if @type == QEMU_MONITOR_CPU_MODEL_EXPANSION_STATIC + * construct @expansion using STATIC model name and a PARTIAL (delta) property list + * + * if @type == QEMU_MONITOR_CPU_MODEL_EXPANSION_FULL + * construct @expansion using DYNAMIC model name and a FULL property list + * + * if @type == QEMU_MONITOR_CPU_MODEL_EXPANSION_STATIC_FULL + * construct @expansion using STATIC model name and a FULL property list + */ int qemuMonitorGetCPUModelExpansion(qemuMonitorPtr mon, qemuMonitorCPUModelExpansionType type, - const char *model_name, bool migratable, - qemuMonitorCPUModelInfoPtr *model_info) + qemuMonitorCPUModelInfoPtr input, + qemuMonitorCPUModelInfoPtr *expansion + ) { VIR_DEBUG("type=%d model_name=%s migratable=%d", - type, model_name, migratable); + type, input->name, migratable); QEMU_CHECK_MONITOR(mon); - return qemuMonitorJSONGetCPUModelExpansion(mon, type, model_name, - migratable, model_info); + return qemuMonitorJSONGetCPUModelExpansion(mon, type, migratable, input, expansion); } diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index d0c54f60a9..78ad8ae428 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -1036,9 +1036,10 @@ typedef enum { int qemuMonitorGetCPUModelExpansion(qemuMonitorPtr mon, qemuMonitorCPUModelExpansionType type, - const char *model_name, bool migratable, - qemuMonitorCPUModelInfoPtr *model_info); + qemuMonitorCPUModelInfoPtr input, + qemuMonitorCPUModelInfoPtr *expansion) + ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(5); void qemuMonitorCPUModelInfoFree(qemuMonitorCPUModelInfoPtr model_info); diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index abfa4155ee..96e9f0ea3c 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -5600,12 +5600,13 @@ qemuMonitorJSONBuildCPUModelInfoFromJSON(virJSONValuePtr cpu_model) return ret; } + int qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr mon, qemuMonitorCPUModelExpansionType type, - const char *model_name, bool migratable, - qemuMonitorCPUModelInfoPtr *model_info) + qemuMonitorCPUModelInfoPtr input, + qemuMonitorCPUModelInfoPtr *expansion) { int ret = -1; virJSONValuePtr json_model_in = NULL; @@ -5613,12 +5614,13 @@ qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr mon, virJSONValuePtr reply = NULL; virJSONValuePtr data; virJSONValuePtr cpu_model; + qemuMonitorCPUModelInfoPtr expanded_model = NULL; qemuMonitorCPUModelInfoPtr model_in = NULL; const char *typeStr = ""; - *model_info = NULL; + *expansion = NULL; - if (!(model_in = qemuMonitorCPUModelInfoNew(model_name))) + if (!(model_in = qemuMonitorCPUModelInfoCopy(input))) goto cleanup; if (!migratable && @@ -5684,13 +5686,17 @@ qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr mon, goto retry; } - if (!(*model_info = qemuMonitorJSONBuildCPUModelInfoFromJSON(cpu_model))) + if (!(expanded_model = qemuMonitorJSONBuildCPUModelInfoFromJSON(cpu_model))) goto cleanup; + VIR_STEAL_PTR(*expansion, expanded_model); ret = 0; cleanup: + VIR_FREE(expanded_model); /* Free structure but not reused contents */ qemuMonitorCPUModelInfoFree(model_in); + qemuMonitorCPUModelInfoFree(expanded_model); + virJSONValueFree(cmd); virJSONValueFree(reply); virJSONValueFree(json_model_in); diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index c3abd0ddf0..942dd85ee1 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -367,10 +367,10 @@ int qemuMonitorJSONGetCPUDefinitions(qemuMonitorPtr mon, int qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr mon, qemuMonitorCPUModelExpansionType type, - const char *model_name, bool migratable, - qemuMonitorCPUModelInfoPtr *model_info) - ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(5); + qemuMonitorCPUModelInfoPtr input, + qemuMonitorCPUModelInfoPtr *expansion) + ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(5); int qemuMonitorJSONGetCommands(qemuMonitorPtr mon, char ***commands) diff --git a/tests/cputest.c b/tests/cputest.c index 339119c63f..c438a8d09e 100644 --- a/tests/cputest.c +++ b/tests/cputest.c @@ -483,6 +483,7 @@ cpuTestMakeQEMUCaps(const struct data *data) virQEMUCapsPtr qemuCaps = NULL; qemuMonitorTestPtr testMon = NULL; qemuMonitorCPUModelInfoPtr model = NULL; + qemuMonitorCPUModelInfoPtr expansion = NULL; char *json = NULL; if (virAsprintf(&json, "%s/cputestdata/%s-cpuid-%s.json", @@ -492,9 +493,12 @@ cpuTestMakeQEMUCaps(const struct data *data) if (!(testMon = qemuMonitorTestNewFromFile(json, driver.xmlopt, true))) goto error; + if (!(model = qemuMonitorCPUModelInfoNew("host"))) + goto cleanup; + if (qemuMonitorGetCPUModelExpansion(qemuMonitorTestGetMonitor(testMon), QEMU_MONITOR_CPU_MODEL_EXPANSION_STATIC, - "host", true, &model) < 0) + true, model, &expansion) < 0) goto error; if (!(qemuCaps = virQEMUCapsNew())) @@ -506,8 +510,8 @@ cpuTestMakeQEMUCaps(const struct data *data) virQEMUCapsSet(qemuCaps, QEMU_CAPS_QUERY_CPU_DEFINITIONS); virQEMUCapsSetArch(qemuCaps, data->arch); - virQEMUCapsSetCPUModelInfo(qemuCaps, VIR_DOMAIN_VIRT_KVM, model); - model = NULL; + virQEMUCapsSetCPUModelInfo(qemuCaps, VIR_DOMAIN_VIRT_KVM, expansion); + expansion = NULL; if (virQEMUCapsProbeQMPCPUDefinitions(qemuCaps, qemuMonitorTestGetMonitor(testMon), @@ -516,6 +520,7 @@ cpuTestMakeQEMUCaps(const struct data *data) cleanup: qemuMonitorCPUModelInfoFree(model); + qemuMonitorCPUModelInfoFree(expansion); qemuMonitorTestFree(testMon); VIR_FREE(json); -- 2.17.1

Move existing code to convert between cpu model info structures (qemuMonitorCPUModelInfoPtr into virCPUDef) into a reusable function. The new function is used in this and future patches. Signed-off-by: Chris Venteicher <cventeic@redhat.com> --- src/qemu/qemu_capabilities.c | 84 ++++++++++++++++++++++++++---------- src/qemu/qemu_capabilities.h | 3 ++ 2 files changed, 64 insertions(+), 23 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 8c5ec4cc9a..74f670459f 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -2819,7 +2819,8 @@ virQEMUCapsInitCPUModelS390(virQEMUCapsPtr qemuCaps, virCPUDefPtr cpu, bool migratable) { - size_t i; + virCPUDefPtr tmp = NULL; + int ret = -1; if (!modelInfo) { if (type == VIR_DOMAIN_VIRT_KVM) { @@ -2832,32 +2833,18 @@ virQEMUCapsInitCPUModelS390(virQEMUCapsPtr qemuCaps, return 2; } - if (VIR_STRDUP(cpu->model, modelInfo->name) < 0 || - VIR_ALLOC_N(cpu->features, modelInfo->nprops) < 0) - return -1; + if (!(tmp = virQEMUCapsCPUModelInfoToCPUDef(migratable, modelInfo))) + goto cleanup; - cpu->nfeatures_max = modelInfo->nprops; - cpu->nfeatures = 0; + /* Free original then copy over model, vendor, vendor_id and features */ + virCPUDefStealModel(cpu, tmp, true); - for (i = 0; i < modelInfo->nprops; i++) { - virCPUFeatureDefPtr feature = cpu->features + cpu->nfeatures; - qemuMonitorCPUPropertyPtr prop = modelInfo->props + i; + ret = 0; - if (prop->type != QEMU_MONITOR_CPU_PROPERTY_BOOLEAN) - continue; + cleanup: + virCPUDefFree(tmp); - if (VIR_STRDUP(feature->name, prop->name) < 0) - return -1; - - if (!prop->value.boolean || - (migratable && prop->migratable == VIR_TRISTATE_BOOL_NO)) - feature->policy = VIR_CPU_FEATURE_DISABLE; - else - feature->policy = VIR_CPU_FEATURE_REQUIRE; - cpu->nfeatures++; - } - - return 0; + return ret; } @@ -3655,6 +3642,57 @@ virQEMUCapsLoadCache(virArch hostArch, } +/* qemuMonitorCPUModelInfo name => virCPUDef model + * qemuMonitorCPUModelInfo boolean properties => virCPUDef features + * + * migratable true: mark non-migratable features as disabled + * false: allow all features as required + */ +virCPUDefPtr +virQEMUCapsCPUModelInfoToCPUDef(bool migratable, qemuMonitorCPUModelInfoPtr model) +{ + virCPUDefPtr cpu = NULL; + virCPUDefPtr ret = NULL; + size_t i; + + if (!model || VIR_ALLOC(cpu) < 0) + goto cleanup; + + VIR_DEBUG("model->name= %s", NULLSTR(model->name)); + + if (VIR_STRDUP(cpu->model, model->name) < 0 || + VIR_ALLOC_N(cpu->features, model->nprops) < 0) + goto cleanup; + + cpu->nfeatures_max = model->nprops; + cpu->nfeatures = 0; + + for (i = 0; i < model->nprops; i++) { + virCPUFeatureDefPtr feature = cpu->features + cpu->nfeatures; + qemuMonitorCPUPropertyPtr prop = model->props + i; + + if (prop->type != QEMU_MONITOR_CPU_PROPERTY_BOOLEAN) + continue; + + if (VIR_STRDUP(feature->name, prop->name) < 0) + goto cleanup; + + if (!prop->value.boolean || + (migratable && prop->migratable == VIR_TRISTATE_BOOL_NO)) + feature->policy = VIR_CPU_FEATURE_DISABLE; + else + feature->policy = VIR_CPU_FEATURE_REQUIRE; + + cpu->nfeatures++; + } + + VIR_STEAL_PTR(ret, cpu); + + cleanup: + virCPUDefFree(cpu); + return ret; +} + static void virQEMUCapsFormatHostCPUModelInfo(virQEMUCapsPtr qemuCaps, virBufferPtr buf, diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index b1990b6bb8..52e36e76b6 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -579,6 +579,9 @@ int virQEMUCapsGetMachineTypesCaps(virQEMUCapsPtr qemuCaps, void virQEMUCapsFilterByMachineType(virQEMUCapsPtr qemuCaps, const char *machineType); +virCPUDefPtr virQEMUCapsCPUModelInfoToCPUDef(bool migratable, + qemuMonitorCPUModelInfoPtr model); + virFileCachePtr virQEMUCapsCacheNew(const char *libDir, const char *cacheDir, uid_t uid, -- 2.17.1

Create public function to convert virCPUDef data structure into qemuMonitorCPUModelInfoPtr data structure. There was no existing code to reuse to create this function so this new virQEMUCapsCPUModelInfoFromCPUDef function was based on reversing the action of the existing virQEMUCapsCPUModelInfoToCPUDef function. Signed-off-by: Chris Venteicher <cventeic@redhat.com> --- src/qemu/qemu_capabilities.c | 46 ++++++++++++++++++++++++++++++++++++ src/qemu/qemu_capabilities.h | 1 + 2 files changed, 47 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 74f670459f..b36ccda090 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -3642,6 +3642,51 @@ virQEMUCapsLoadCache(virArch hostArch, } +/* virCPUDef model => qemuMonitorCPUModelInfo name + * virCPUDef features => qemuMonitorCPUModelInfo boolean properties */ +qemuMonitorCPUModelInfoPtr +virQEMUCapsCPUModelInfoFromCPUDef(const virCPUDef *cpuDef) +{ + size_t i; + qemuMonitorCPUModelInfoPtr cpuModel = NULL; + qemuMonitorCPUModelInfoPtr ret = NULL; + + if (!cpuDef || (VIR_ALLOC(cpuModel) < 0)) + goto cleanup; + + VIR_DEBUG("cpuDef->model = %s", NULLSTR(cpuDef->model)); + + if (VIR_STRDUP(cpuModel->name, cpuDef->model) < 0 || + VIR_ALLOC_N(cpuModel->props, cpuDef->nfeatures) < 0) + goto cleanup; + + cpuModel->nprops = 0; + + for (i = 0; i < cpuDef->nfeatures; i++) { + qemuMonitorCPUPropertyPtr prop = &(cpuModel->props[cpuModel->nprops]); + virCPUFeatureDefPtr feature = &(cpuDef->features[i]); + + if (!(feature->name) || + VIR_STRDUP(prop->name, feature->name) < 0) + goto cleanup; + + prop->type = QEMU_MONITOR_CPU_PROPERTY_BOOLEAN; + + prop->value.boolean = feature->policy == -1 || /* policy undefined */ + feature->policy == VIR_CPU_FEATURE_FORCE || + feature->policy == VIR_CPU_FEATURE_REQUIRE; + + cpuModel->nprops++; + } + + VIR_STEAL_PTR(ret, cpuModel); + + cleanup: + qemuMonitorCPUModelInfoFree(cpuModel); + return ret; +} + + /* qemuMonitorCPUModelInfo name => virCPUDef model * qemuMonitorCPUModelInfo boolean properties => virCPUDef features * @@ -3693,6 +3738,7 @@ virQEMUCapsCPUModelInfoToCPUDef(bool migratable, qemuMonitorCPUModelInfoPtr mode return ret; } + static void virQEMUCapsFormatHostCPUModelInfo(virQEMUCapsPtr qemuCaps, virBufferPtr buf, diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 52e36e76b6..9bc6773263 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -579,6 +579,7 @@ int virQEMUCapsGetMachineTypesCaps(virQEMUCapsPtr qemuCaps, void virQEMUCapsFilterByMachineType(virQEMUCapsPtr qemuCaps, const char *machineType); +qemuMonitorCPUModelInfoPtr virQEMUCapsCPUModelInfoFromCPUDef(const virCPUDef *cpuDef); virCPUDefPtr virQEMUCapsCPUModelInfoToCPUDef(bool migratable, qemuMonitorCPUModelInfoPtr model); -- 2.17.1

Introduce monitor functions to use QEMU to compute baseline cpu from an input of two cpu models. Signed-off-by: Chris Venteicher <cventeic@redhat.com> --- src/qemu/qemu_monitor.c | 12 ++++++++ src/qemu/qemu_monitor.h | 6 ++++ src/qemu/qemu_monitor_json.c | 60 ++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 6 ++++ 4 files changed, 84 insertions(+) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index bed6a2f90a..c5266b6767 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -4514,3 +4514,15 @@ qemuMonitorGetPRManagerInfo(qemuMonitorPtr mon, virHashFree(info); return ret; } + + +int +qemuMonitorGetCPUModelBaseline(qemuMonitorPtr mon, + qemuMonitorCPUModelInfoPtr model_a, + qemuMonitorCPUModelInfoPtr model_b, + qemuMonitorCPUModelInfoPtr *model_baseline) +{ + QEMU_CHECK_MONITOR(mon); + + return qemuMonitorJSONGetCPUModelBaseline(mon, model_a, model_b, model_baseline); +} diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 78ad8ae428..c87a2d5e47 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -1205,4 +1205,10 @@ struct _qemuMonitorPRManagerInfo { int qemuMonitorGetPRManagerInfo(qemuMonitorPtr mon, virHashTablePtr *retinfo); +int qemuMonitorGetCPUModelBaseline(qemuMonitorPtr mon, + qemuMonitorCPUModelInfoPtr model_a, + qemuMonitorCPUModelInfoPtr model_b, + qemuMonitorCPUModelInfoPtr *model_baseline) + ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4); + #endif /* QEMU_MONITOR_H */ diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 96e9f0ea3c..bd0e940325 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -8514,3 +8514,63 @@ qemuMonitorJSONGetPRManagerInfo(qemuMonitorPtr mon, return ret; } + + +int +qemuMonitorJSONGetCPUModelBaseline(qemuMonitorPtr mon, + qemuMonitorCPUModelInfoPtr model_a, + qemuMonitorCPUModelInfoPtr model_b, + qemuMonitorCPUModelInfoPtr *model_baseline) +{ + int ret = -1; + virJSONValuePtr cmd = NULL; + virJSONValuePtr reply = NULL; + virJSONValuePtr data = NULL; + virJSONValuePtr modela = NULL; + virJSONValuePtr modelb = NULL; + virJSONValuePtr cpu_model = NULL; + + *model_baseline = NULL; + + if (!(modela = qemuMonitorJSONBuildCPUModelInfoToJSON(model_a)) || + !(modelb = qemuMonitorJSONBuildCPUModelInfoToJSON(model_b))) + goto cleanup; + + if (!(cmd = qemuMonitorJSONMakeCommand("query-cpu-model-baseline", + "a:modela", &modela, + "a:modelb", &modelb, + NULL))) + goto cleanup; + + if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) + goto cleanup; + + if (qemuMonitorJSONCheckReply(cmd, reply, VIR_JSON_TYPE_OBJECT) < 0) { + virReportError(VIR_ERR_INVALID_ARG, + _("QEMU doesn't support baseline or recognize model %s or %s"), + model_a->name, + model_b->name); + goto cleanup; + } + + data = virJSONValueObjectGetObject(reply, "return"); + + if (!(cpu_model = virJSONValueObjectGetObject(data, "model"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("query-cpu-model-baseline reply data was missing 'model'")); + goto cleanup; + } + + if (!(*model_baseline = qemuMonitorJSONBuildCPUModelInfoFromJSON(cpu_model))) + goto cleanup; + + ret = 0; + + cleanup: + virJSONValueFree(cmd); + virJSONValueFree(reply); + virJSONValueFree(modela); + virJSONValueFree(modelb); + + return ret; +} diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index 942dd85ee1..7bd161af58 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -581,4 +581,10 @@ int qemuMonitorJSONGetPRManagerInfo(qemuMonitorPtr mon, virHashTablePtr info) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); +int qemuMonitorJSONGetCPUModelBaseline(qemuMonitorPtr mon, + qemuMonitorCPUModelInfoPtr model_a, + qemuMonitorCPUModelInfoPtr model_b, + qemuMonitorCPUModelInfoPtr *model_baseline) + ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4); + #endif /* QEMU_MONITOR_JSON_H */ -- 2.17.1

Create utility function encapsulating code to calculate hypervisor baseline cpu using the local libvirt utility functions. Similar function encapsulating code to calculating hypervisor baseline using QEMU QMP messages will be introduced in later commit. Patch is a cut and paste of existing code into a utility function wrapper. s/cpu/*baseline/ (change output variable name ) and initialize variable "rc" are the only code changes. Signed-off-by: Chris Venteicher <cventeic@redhat.com> --- src/qemu/qemu_driver.c | 78 ++++++++++++++++++++++++++++-------------- 1 file changed, 52 insertions(+), 26 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 007f64d6ad..86fc5aa249 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -13642,6 +13642,55 @@ qemuConnectBaselineCPU(virConnectPtr conn ATTRIBUTE_UNUSED, } +static int +qemuConnectBaselineHypervisorCPUViaLibvirt( + virQEMUCapsPtr qemuCaps, + bool migratable, + virDomainVirtType virttype, + virArch arch, + virCPUDefPtr *cpus, + unsigned int ncpus, + virCPUDefPtr *baseline) +{ + char **features = NULL; + int ret = -1; + int rc = -1; + virDomainCapsCPUModelsPtr cpuModels; + + *baseline = NULL; + + if (!(cpuModels = virQEMUCapsGetCPUDefinitions(qemuCaps, virttype)) || + cpuModels->nmodels == 0) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("QEMU '%s' does not support any CPU models for " + "virttype '%s'"), + virQEMUCapsGetBinary(qemuCaps), + virDomainVirtTypeToString(virttype)); + goto cleanup; + } + + rc = virQEMUCapsGetCPUFeatures(qemuCaps, virttype, + migratable, &features); + if (rc < 0) + goto cleanup; + if (features && rc == 0) { + /* We got only migratable features from QEMU if we asked for them, + * no further filtering in virCPUBaseline is desired. */ + migratable = false; + } + + if (!(*baseline = virCPUBaseline(arch, cpus, ncpus, cpuModels, + (const char **)features, migratable))) + goto cleanup; + + ret = 0; + + cleanup: + virStringListFree(features); + return ret; +} + + static char * qemuConnectBaselineHypervisorCPU(virConnectPtr conn, const char *emulator, @@ -13660,7 +13709,6 @@ qemuConnectBaselineHypervisorCPU(virConnectPtr conn, bool migratable; virCPUDefPtr cpu = NULL; char *cpustr = NULL; - char **features = NULL; virCheckFlags(VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES | VIR_CONNECT_BASELINE_CPU_MIGRATABLE, NULL); @@ -13683,30 +13731,9 @@ qemuConnectBaselineHypervisorCPU(virConnectPtr conn, if (ARCH_IS_X86(arch)) { migratable = !!(flags & VIR_CONNECT_BASELINE_CPU_MIGRATABLE); - virDomainCapsCPUModelsPtr cpuModels; - - if (!(cpuModels = virQEMUCapsGetCPUDefinitions(qemuCaps, virttype)) || - cpuModels->nmodels == 0) { - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, - _("QEMU '%s' does not support any CPU models for " - "virttype '%s'"), - virQEMUCapsGetBinary(qemuCaps), - virDomainVirtTypeToString(virttype)); - goto cleanup; - } - - int rc = virQEMUCapsGetCPUFeatures(qemuCaps, virttype, - migratable, &features); - if (rc < 0) - goto cleanup; - if (features && rc == 0) { - /* We got only migratable features from QEMU if we asked for them, - * no further filtering in virCPUBaseline is desired. */ - migratable = false; - } - - if (!(cpu = virCPUBaseline(arch, cpus, ncpus, cpuModels, - (const char **)features, migratable))) + if (qemuConnectBaselineHypervisorCPUViaLibvirt(qemuCaps, migratable, + virttype, arch, + cpus, ncpus, &cpu) < 0) goto cleanup; } else { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, @@ -13727,7 +13754,6 @@ qemuConnectBaselineHypervisorCPU(virConnectPtr conn, virCPUDefListFree(cpus); virCPUDefFree(cpu); virObjectUnref(qemuCaps); - virStringListFree(features); return cpustr; } -- 2.17.1

Add capability to calculate hypervisor baseline using QMP message exchanges with QEMU in addition to existing capability to calculate baseline using libvirt utility functions. A new utility function encapsulates the core logic for interacting with QEMU using QMP baseline messages. The QMP messages only allow two cpu models to be evaluated at a time so a sequence of QMP baseline messages must be used to include all the models in the baseline when more than 2 cpu models are input. A QEMU process must be started prior to sending the series of QEMU messages to compute baseline. The QEMU process is started and maintained in the main hypervisor baseline function and only a pointer to the QEMU process monitor is passed to the baseline utility function. The QEMU process is maintained in the main hypervisor baseline function because the process will also be used for feature expansion (via QEMU) in a later patch. Signed-off-by: Chris Venteicher <cventeic@redhat.com> --- src/qemu/qemu_driver.c | 97 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 97 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 4e8a3902d3..8c6838f584 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -13642,6 +13642,78 @@ qemuConnectBaselineCPU(virConnectPtr conn ATTRIBUTE_UNUSED, } +/* in: + * cpus[0]->model = "z14"; + * cpus[0]->features[0].name = "xxx"; + * cpus[0]->features[1].name = "yyy"; + * *** + * cpus[n]->model = "z13"; + * cpus[n]->features[0].name = "xxx"; + * cpus[n]->features[1].name = "yyy"; + * + * out: + * *baseline->model = "z13-base"; + * *baseline->features[0].name = "yyy"; + */ +static int +qemuConnectBaselineHypervisorCPUViaQEMU(qemuMonitorPtr mon, + virCPUDefPtr *cpus, + virCPUDefPtr *baseline) +{ + qemuMonitorCPUModelInfoPtr model_baseline = NULL; + qemuMonitorCPUModelInfoPtr new_model_baseline = NULL; + qemuMonitorCPUModelInfoPtr next_model = NULL; + bool migratable = true; + int ret = -1; + size_t i; + + *baseline = NULL; + + if (!cpus || !cpus[0]) { + virReportError(VIR_ERR_INVALID_ARG, "%s", _("no cpus")); + goto cleanup; + } + + for (i = 0; cpus[i]; i++) { /* cpus terminated by NULL element */ + virCPUDefPtr cpu = cpus[i]; + + VIR_DEBUG("cpu[%lu]->model = %s", i, NULLSTR(cpu->model)); + + if (!(next_model = virQEMUCapsCPUModelInfoFromCPUDef(cpu))) + goto cleanup; + + if (i == 0) { + model_baseline = next_model; + continue; + } + + if (qemuMonitorGetCPUModelBaseline(mon, model_baseline, + next_model, &new_model_baseline) < 0) + goto cleanup; + + qemuMonitorCPUModelInfoFree(model_baseline); + qemuMonitorCPUModelInfoFree(next_model); + + next_model = NULL; + + model_baseline = new_model_baseline; + } + + if (!(*baseline = virQEMUCapsCPUModelInfoToCPUDef(migratable, model_baseline))) + goto cleanup; + + VIR_DEBUG("baseline->model = %s", (*baseline)->model); + + ret = 0; + + cleanup: + qemuMonitorCPUModelInfoFree(model_baseline); + qemuMonitorCPUModelInfoFree(next_model); + + return ret; +} + + static int qemuConnectBaselineHypervisorCPUViaLibvirt( virQEMUCapsPtr qemuCaps, @@ -13710,6 +13782,11 @@ qemuConnectBaselineHypervisorCPU(virConnectPtr conn, virCPUDefPtr cpu = NULL; char *cpustr = NULL; bool useLibvirt = false; + bool useQemu = false; + bool forceTCG = false; + qemuMonitorCPUModelInfoPtr modelInfo = NULL; + qemuMonitorCPUModelInfoPtr expansion = NULL; + qemuProcessQmpPtr proc = NULL; virCheckFlags(VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES | VIR_CONNECT_BASELINE_CPU_MIGRATABLE, NULL); @@ -13730,6 +13807,7 @@ qemuConnectBaselineHypervisorCPU(virConnectPtr conn, goto cleanup; useLibvirt = ARCH_IS_X86(arch); + useQemu = ARCH_IS_S390(arch); if (useLibvirt) { migratable = !!(flags & VIR_CONNECT_BASELINE_CPU_MIGRATABLE); @@ -13738,6 +13816,21 @@ qemuConnectBaselineHypervisorCPU(virConnectPtr conn, virttype, arch, cpus, ncpus, &cpu) < 0) goto cleanup; + + } else if (useQemu) { + const char *binary = virQEMUCapsGetBinary(qemuCaps); + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + + if (!(proc = qemuProcessQmpNew(binary, cfg->libDir, + cfg->user, cfg->group, forceTCG))) + goto cleanup; + + if (qemuProcessQmpStart(proc) < 0) + goto cleanup; + + if ((qemuConnectBaselineHypervisorCPUViaQEMU(proc->mon, cpus, &cpu) < 0) || !cpu) + goto cleanup; + } else { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, _("computing baseline hypervisor CPU is not supported " @@ -13765,6 +13858,10 @@ qemuConnectBaselineHypervisorCPU(virConnectPtr conn, virCPUDefListFree(cpus); virCPUDefFree(cpu); virObjectUnref(qemuCaps); + qemuMonitorCPUModelInfoFree(modelInfo); + qemuMonitorCPUModelInfoFree(expansion); + qemuProcessQmpStop(proc); + qemuProcessQmpFree(proc); return cpustr; } -- 2.17.1

Hypervisor baseline has an expansion mode where the cpu property / feature list is fully expanded to list all supported (true) features. The output (expanded) cpu model should not list properties that are false (unsupported.) QEMU expansion output enumerates both true (supported) and false (unsupported) properties. This patch removes the false (unsupported) entries from the output cpu property list. This change makes the expanded output consistent when both the internal libvirt utility functions and QEMU QMP commands are used to expand the property list. qemu_monitor: Introduce removal of true/false CPUModelInfo props A utility function is created to squash CPU properties list in qemuMonitorCPUmodelInfo structure by removing boolean properties of matching value. Signed-off-by: Chris Venteicher <cventeic@redhat.com> --- src/qemu/qemu_driver.c | 4 ++++ src/qemu/qemu_monitor.c | 30 ++++++++++++++++++++++++++++++ src/qemu/qemu_monitor.h | 4 ++++ 3 files changed, 38 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 4149e4794a..4f338f1b37 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -13861,6 +13861,10 @@ qemuConnectBaselineHypervisorCPU(virConnectPtr conn, migratable, modelInfo, &expansion) < 0) goto cleanup; + /* Expansion enumerates all features + * Baselines output enumerates only in-model (true) features */ + qemuMonitorCPUModelInfoRemovePropByBoolValue(expansion, false); + if (!(cpu = virQEMUCapsCPUModelInfoToCPUDef(migratable, expansion))) goto cleanup; diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index c5266b6767..0c7d7b4b55 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -3785,6 +3785,36 @@ qemuMonitorCPUModelInfoCopy(const qemuMonitorCPUModelInfo *orig) } +/* Squash CPU Model Info property list + * removing props of type boolean matching value */ +void +qemuMonitorCPUModelInfoRemovePropByBoolValue(qemuMonitorCPUModelInfoPtr model, + bool value) +{ + qemuMonitorCPUPropertyPtr src; + qemuMonitorCPUPropertyPtr dst; + size_t i; + size_t dst_nprops = 0; + + for (i = 0; i < model->nprops; i++) { + src = &(model->props[i]); + dst = &(model->props[dst_nprops]); + + if (src->type == QEMU_MONITOR_CPU_PROPERTY_BOOLEAN && + src->value.boolean == value) + continue; + + *dst = *src; + + dst_nprops++; + } + + model->nprops = dst_nprops; + + ignore_value(VIR_REALLOC_N_QUIET(model->props, dst_nprops)); +} + + int qemuMonitorCPUModelInfoBoolPropAdd(qemuMonitorCPUModelInfoPtr model, const char *prop_name, diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index c87a2d5e47..7aaecd97dd 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -1053,6 +1053,10 @@ int qemuMonitorCPUModelInfoBoolPropAdd(qemuMonitorCPUModelInfoPtr model, bool prop_value) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); +void qemuMonitorCPUModelInfoRemovePropByBoolValue(qemuMonitorCPUModelInfoPtr model, + bool value) + ATTRIBUTE_NONNULL(1); + int qemuMonitorGetCommands(qemuMonitorPtr mon, char ***commands); int qemuMonitorGetEvents(qemuMonitorPtr mon, -- 2.17.1