[libvirt] [PATCH v7 00/20] Refactor virQEMUCapsInitQMPCommand (for BaselineHypervisorCPU using QEMU QMP exchanges)
This series is a replacement for the refactoring part (the first 22 patches) of the "BaselineHypervisorCPU using QEMU QMP exchanges" series [1] from Chris. In a nutshell this series moves virQEMUCapsInitQMPCommand related code to qemu_process.c and refactors it so that it can be used by both the QEMU capabilities probing code and the upcoming code for calling QMP commands internally for some CPU related APIs on s390. I'm sending this refactoring part separated because it's mostly ready for being pushed. I ACKed most of the patches in v6, but some patches needed to be modified a bit or replaced by a bit different code. Thus I'm sending them for review rather than pushing this part right away. See individual patches for changes since v6. [1] https://www.redhat.com/archives/libvir-list/2019-January/msg00310.html Chris Venteicher (15): qemu_process: Move process code from qemu_capabilities qemu_process: Rename identifiers moved from qemu_capabilities qemu_process: Limit qemuProcessQMPNew to const input strings qemu_process: Refer to proc not cmd in process code qemu_process: Use consistent name for stop process function qemu_capabilities: Stop QEMU process before freeing qemu_process: Use qemuProcessQMP struct for a single process qemu_process: Introduce qemuProcessQMPStart qemu_process: Move monitor code to qemuProcessQMPConnectMonitor qemu_process: Store libDir in qemuProcessQMP struct qemu_process: Setup paths within qemuProcessQMPInit qemu_process: Stop retaining monitor config in qemuProcessQMP qemu_process: Document and cleanup qemuProcessQMPNew qemu_process: Use unique directories for QMP processes qemu_process: Enter QMP command mode when starting QEMU Process Jiri Denemark (5): qemu_capabilities: Refactor virQEMUCapsInitQMP qemu_process: Don't ignore errors in virQEMUCapsInit qemu_capabilities: Log probe failure in virQEMUCapsInitQMPSingle qemu_process: Hide qmperr inside qemuProcessQMP qemu_process: Hide qemuProcessQMPStop src/qemu/qemu_capabilities.c | 356 ++++++----------------------------- src/qemu/qemu_process.c | 324 +++++++++++++++++++++++++++++++ src/qemu/qemu_process.h | 29 +++ src/qemu/qemu_processpriv.h | 2 + tests/qemucapabilitiestest.c | 9 + 5 files changed, 419 insertions(+), 301 deletions(-) -- 2.20.1
From: Chris Venteicher <cventeic@redhat.com> QEMU process code in qemu_capabilities.c is moved to qemu_process.c in order to make the code usable outside the original capabilities use cases. The moved code activates and manages QEMU processes without establishing a guest domain. This patch is a straight cut/paste move between files. Signed-off-by: Chris Venteicher <cventeic@redhat.com> Reviewed-by: Jiri Denemark <jdenemar@redhat.com> Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- Notes: Version 7: - rebased src/qemu/qemu_capabilities.c | 230 +---------------------------------- src/qemu/qemu_process.c | 211 ++++++++++++++++++++++++++++++++ src/qemu/qemu_process.h | 30 +++++ 3 files changed, 242 insertions(+), 229 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 9d9c8096ba..26f83eb1d3 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -45,6 +45,7 @@ #define LIBVIRT_QEMU_CAPSPRIV_H_ALLOW #include "qemu_capspriv.h" #include "qemu_qapi.h" +#include "qemu_process.h" #include <fcntl.h> #include <sys/stat.h> @@ -53,10 +54,6 @@ #include <stdarg.h> #include <sys/utsname.h> -#if WITH_CAPNG -# include <cap-ng.h> -#endif - #define VIR_FROM_THIS VIR_FROM_QEMU VIR_LOG_INIT("qemu.qemu_capabilities"); @@ -4049,18 +4046,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 @@ -4369,219 +4354,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); - -#if WITH_CAPNG - /* QEMU might run into permission issues, e.g. /dev/sev (0600), override - * them just for the purpose of probing */ - if (geteuid() == 0) - virCommandAllowCap(cmd->cmd, CAP_DAC_OVERRIDE); -#endif - - 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 a62d89320b..c45a15a1c1 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -34,6 +34,10 @@ #include <sys/utsname.h> +#if WITH_CAPNG +# include <cap-ng.h> +#endif + #include "qemu_process.h" #define LIBVIRT_QEMU_PROCESSPRIV_H_ALLOW #include "qemu_processpriv.h" @@ -8306,3 +8310,210 @@ 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); + +#if WITH_CAPNG + /* QEMU might run into permission issues, e.g. /dev/sev (0600), override + * them just for the purpose of probing */ + if (geteuid() == 0) + virCommandAllowCap(cmd->cmd, CAP_DAC_OVERRIDE); +#endif + + 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 67a4d7139f..f76613a7f7 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 /* LIBVIRT_QEMU_PROCESS_H */ -- 2.20.1
On Tue, Feb 19, 2019 at 10:04:44AM +0100, Jiri Denemark wrote:
From: Chris Venteicher <cventeic@redhat.com>
QEMU process code in qemu_capabilities.c is moved to qemu_process.c in order to make the code usable outside the original capabilities use cases.
The moved code activates and manages QEMU processes without establishing a guest domain.
This patch is a straight cut/paste move between files.
Signed-off-by: Chris Venteicher <cventeic@redhat.com> Reviewed-by: Jiri Denemark <jdenemar@redhat.com> Signed-off-by: Jiri Denemark <jdenemar@redhat.com> ---
Notes: Version 7: - rebased
src/qemu/qemu_capabilities.c | 230 +---------------------------------- src/qemu/qemu_process.c | 211 ++++++++++++++++++++++++++++++++ src/qemu/qemu_process.h | 30 +++++ 3 files changed, 242 insertions(+), 229 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
On Tue, Feb 19, 2019 at 15:54:40 +0100, Ján Tomko wrote:
On Tue, Feb 19, 2019 at 10:04:44AM +0100, Jiri Denemark wrote:
From: Chris Venteicher <cventeic@redhat.com>
QEMU process code in qemu_capabilities.c is moved to qemu_process.c in order to make the code usable outside the original capabilities use cases.
The moved code activates and manages QEMU processes without establishing a guest domain.
This patch is a straight cut/paste move between files.
Signed-off-by: Chris Venteicher <cventeic@redhat.com> Reviewed-by: Jiri Denemark <jdenemar@redhat.com> Signed-off-by: Jiri Denemark <jdenemar@redhat.com> ---
Notes: Version 7: - rebased
src/qemu/qemu_capabilities.c | 230 +---------------------------------- src/qemu/qemu_process.c | 211 ++++++++++++++++++++++++++++++++ src/qemu/qemu_process.h | 30 +++++ 3 files changed, 242 insertions(+), 229 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Thanks for the reviews, especially when mailing list delivery is unreliable again. Jirka
From: Chris Venteicher <cventeic@redhat.com> s/virQEMUCapsInitQMPCommand/qemuProcessQMP/ Signed-off-by: Chris Venteicher <cventeic@redhat.com> Reviewed-by: Jiri Denemark <jdenemar@redhat.com> Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- Notes: Version 7: - no change 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 26f83eb1d3..e7442ac010 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -4361,15 +4361,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; @@ -4379,8 +4379,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; @@ -4393,7 +4393,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 c45a15a1c1..05acdc6013 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8325,12 +8325,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); @@ -8339,14 +8339,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; @@ -8385,7 +8385,7 @@ virQEMUCapsInitQMPCommandNew(char *binary, return cmd; error: - virQEMUCapsInitQMPCommandFree(cmd); + qemuProcessQMPFree(cmd); return NULL; } @@ -8395,8 +8395,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; @@ -8474,7 +8474,7 @@ virQEMUCapsInitQMPCommandRun(virQEMUCapsInitQMPCommandPtr cmd, cleanup: if (!cmd->mon) - virQEMUCapsInitQMPCommandAbort(cmd); + qemuProcessQMPAbort(cmd); virObjectUnref(xmlopt); return ret; @@ -8486,7 +8486,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 f76613a7f7..4843210f76 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 /* LIBVIRT_QEMU_PROCESS_H */ -- 2.20.1
On Tue, Feb 19, 2019 at 10:04:45AM +0100, Jiri Denemark wrote:
From: Chris Venteicher <cventeic@redhat.com>
s/virQEMUCapsInitQMPCommand/qemuProcessQMP/
Signed-off-by: Chris Venteicher <cventeic@redhat.com> Reviewed-by: Jiri Denemark <jdenemar@redhat.com> Signed-off-by: Jiri Denemark <jdenemar@redhat.com> ---
Notes: Version 7: - no change
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(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
From: Chris Venteicher <cventeic@redhat.com> 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> Reviewed-by: Jiri Denemark <jdenemar@redhat.com> Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- Notes: Version 7: - no change 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 05acdc6013..6b698c0622 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8340,7 +8340,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 4843210f76..fe79873967 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.20.1
On Tue, Feb 19, 2019 at 10:04:46AM +0100, Jiri Denemark wrote:
From: Chris Venteicher <cventeic@redhat.com>
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> Reviewed-by: Jiri Denemark <jdenemar@redhat.com> Signed-off-by: Jiri Denemark <jdenemar@redhat.com> ---
Notes: Version 7: - no change
src/qemu/qemu_process.c | 2 +- src/qemu/qemu_process.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
From: Chris Venteicher <cventeic@redhat.com> s/cmd/proc/ in process code imported from qemu_capabilities. Signed-off-by: Chris Venteicher <cventeic@redhat.com> Reviewed-by: Jiri Denemark <jdenemar@redhat.com> Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- Notes: Version 7: - s/cmd/proc/ in qemuProcessQMPRun prototype src/qemu/qemu_capabilities.c | 18 ++--- src/qemu/qemu_process.c | 140 +++++++++++++++++------------------ src/qemu/qemu_process.h | 6 +- 3 files changed, 82 insertions(+), 82 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index e7442ac010..aa6571457e 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -4361,39 +4361,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 6b698c0622..8e45dbc0ce 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8325,17 +8325,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); } @@ -8346,25 +8346,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 @@ -8373,19 +8373,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; } @@ -8395,7 +8395,7 @@ qemuProcessQMPNew(const char *binary, * 1 when probing QEMU failed */ int -qemuProcessQMPRun(qemuProcessQMPPtr cmd, +qemuProcessQMPRun(qemuProcessQMPPtr proc, bool forceTCG) { virDomainXMLOptionPtr xmlopt = NULL; @@ -8409,7 +8409,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 @@ -8418,63 +8418,63 @@ 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", + proc->cmd = virCommandNewArgList(proc->binary, + "-S", + "-no-user-config", + "-nodefaults", + "-nographic", + "-machine", machine, + "-qmp", proc->monarg, + "-pidfile", proc->pidfile, + "-daemonize", NULL); - virCommandAddEnvPassCommon(cmd->cmd); - virCommandClearCaps(cmd->cmd); + virCommandAddEnvPassCommon(proc->cmd); + virCommandClearCaps(proc->cmd); #if WITH_CAPNG /* QEMU might run into permission issues, e.g. /dev/sev (0600), override * them just for the purpose of probing */ if (geteuid() == 0) - virCommandAllowCap(cmd->cmd, CAP_DAC_OVERRIDE); + virCommandAllowCap(proc->cmd, CAP_DAC_OVERRIDE); #endif - virCommandSetGID(cmd->cmd, cmd->runGid); - virCommandSetUID(cmd->cmd, cmd->runUid); + 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, - 0, &callbacks, NULL))) + 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; @@ -8486,34 +8486,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 fe79873967..a480d006f9 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, +int qemuProcessQMPRun(qemuProcessQMPPtr proc, bool forceTCG); -void qemuProcessQMPAbort(qemuProcessQMPPtr cmd); +void qemuProcessQMPAbort(qemuProcessQMPPtr proc); #endif /* LIBVIRT_QEMU_PROCESS_H */ -- 2.20.1
On Tue, Feb 19, 2019 at 10:04:47AM +0100, Jiri Denemark wrote:
From: Chris Venteicher <cventeic@redhat.com>
s/cmd/proc/ in process code imported from qemu_capabilities.
Signed-off-by: Chris Venteicher <cventeic@redhat.com> Reviewed-by: Jiri Denemark <jdenemar@redhat.com> Signed-off-by: Jiri Denemark <jdenemar@redhat.com> ---
Notes: Version 7: - s/cmd/proc/ in qemuProcessQMPRun prototype
src/qemu/qemu_capabilities.c | 18 ++--- src/qemu/qemu_process.c | 140 +++++++++++++++++------------------ src/qemu/qemu_process.h | 6 +- 3 files changed, 82 insertions(+), 82 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
From: Chris Venteicher <cventeic@redhat.com> 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> Reviewed-by: Jiri Denemark <jdenemar@redhat.com> Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- Notes: Version 7: - no change 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 aa6571457e..d187e35a38 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -4379,7 +4379,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 8e45dbc0ce..d463a41ca0 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8330,7 +8330,7 @@ qemuProcessQMPFree(qemuProcessQMPPtr proc) if (!proc) return; - qemuProcessQMPAbort(proc); + qemuProcessQMPStop(proc); VIR_FREE(proc->binary); VIR_FREE(proc->monpath); VIR_FREE(proc->monarg); @@ -8474,7 +8474,7 @@ qemuProcessQMPRun(qemuProcessQMPPtr proc, cleanup: if (!proc->mon) - qemuProcessQMPAbort(proc); + qemuProcessQMPStop(proc); virObjectUnref(xmlopt); return ret; @@ -8486,7 +8486,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 a480d006f9..c59379facb 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -242,6 +242,6 @@ void qemuProcessQMPFree(qemuProcessQMPPtr proc); int qemuProcessQMPRun(qemuProcessQMPPtr proc, bool forceTCG); -void qemuProcessQMPAbort(qemuProcessQMPPtr proc); +void qemuProcessQMPStop(qemuProcessQMPPtr proc); #endif /* LIBVIRT_QEMU_PROCESS_H */ -- 2.20.1
On Tue, Feb 19, 2019 at 10:04:48AM +0100, Jiri Denemark wrote:
From: Chris Venteicher <cventeic@redhat.com>
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> Reviewed-by: Jiri Denemark <jdenemar@redhat.com> Signed-off-by: Jiri Denemark <jdenemar@redhat.com> ---
Notes: Version 7: - no change
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(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
From: Chris Venteicher <cventeic@redhat.com> 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> Reviewed-by: Jiri Denemark <jdenemar@redhat.com> Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- Notes: Version 7: - no change src/qemu/qemu_capabilities.c | 1 + src/qemu/qemu_process.c | 24 ++++++++++++++++-------- 2 files changed, 17 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index d187e35a38..f578d4a5ae 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -4393,6 +4393,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 d463a41ca0..4ae2067782 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8488,14 +8488,20 @@ qemuProcessQMPRun(qemuProcessQMPPtr proc, void qemuProcessQMPStop(qemuProcessQMPPtr proc) { - if (proc->mon) - virObjectUnlock(proc->mon); - qemuMonitorClose(proc->mon); - proc->mon = NULL; + if (!proc) + return; - virCommandAbort(proc->cmd); - virCommandFree(proc->cmd); - proc->cmd = NULL; + if (proc->mon) { + virObjectUnlock(proc->mon); + qemuMonitorClose(proc->mon); + proc->mon = NULL; + } + + if (proc->cmd) { + virCommandAbort(proc->cmd); + virCommandFree(proc->cmd); + proc->cmd = NULL; + } if (proc->monpath) unlink(proc->monpath); @@ -8512,8 +8518,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.20.1
On Tue, Feb 19, 2019 at 10:04:49AM +0100, Jiri Denemark wrote:
From: Chris Venteicher <cventeic@redhat.com>
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> Reviewed-by: Jiri Denemark <jdenemar@redhat.com> Signed-off-by: Jiri Denemark <jdenemar@redhat.com> ---
Notes: Version 7: - no change
src/qemu/qemu_capabilities.c | 1 + src/qemu/qemu_process.c | 24 ++++++++++++++++-------- 2 files changed, 17 insertions(+), 8 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
From: Chris Venteicher <cventeic@redhat.com> 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> Reviewed-by: Jiri Denemark <jdenemar@redhat.com> Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- Notes: Version 7: - no change src/qemu/qemu_capabilities.c | 19 +++++++++++++++---- src/qemu/qemu_process.c | 9 +++++---- src/qemu/qemu_process.h | 7 ++++--- 3 files changed, 24 insertions(+), 11 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index f578d4a5ae..6716e62e4a 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -4362,14 +4362,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; @@ -4379,14 +4380,22 @@ 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; } @@ -4394,7 +4403,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 4ae2067782..6c0f7165c7 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8344,7 +8344,8 @@ qemuProcessQMPNew(const char *binary, const char *libDir, uid_t runUid, gid_t runGid, - char **qmperr) + char **qmperr, + bool forceTCG) { qemuProcessQMPPtr proc = NULL; @@ -8357,6 +8358,7 @@ 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 * domain called "capabilities" @@ -8395,15 +8397,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 c59379facb..46a0bd2475 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 proc, - bool forceTCG); +int qemuProcessQMPRun(qemuProcessQMPPtr proc); void qemuProcessQMPStop(qemuProcessQMPPtr proc); -- 2.20.1
On Tue, Feb 19, 2019 at 10:04:50AM +0100, Jiri Denemark wrote:
From: Chris Venteicher <cventeic@redhat.com>
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> Reviewed-by: Jiri Denemark <jdenemar@redhat.com> Signed-off-by: Jiri Denemark <jdenemar@redhat.com> ---
Notes: Version 7: - no change
src/qemu/qemu_capabilities.c | 19 +++++++++++++++---- src/qemu/qemu_process.c | 9 +++++---- src/qemu/qemu_process.h | 7 ++++--- 3 files changed, 24 insertions(+), 11 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
The function contains two almost identical parts. Let's consolidate them into a single helper function and call it twice. Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- Notes: Version 7: - new patch src/qemu/qemu_capabilities.c | 67 ++++++++++++++++++------------------ 1 file changed, 34 insertions(+), 33 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 6716e62e4a..b695b23fcc 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -4355,19 +4355,19 @@ virQEMUCapsInitQMPMonitorTCG(virQEMUCapsPtr qemuCaps ATTRIBUTE_UNUSED, static int -virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, - const char *libDir, - uid_t runUid, - gid_t runGid, - char **qmperr) +virQEMUCapsInitQMPSingle(virQEMUCapsPtr qemuCaps, + const char *libDir, + uid_t runUid, + gid_t runGid, + char **qmperr, + bool onlyTCG) { qemuProcessQMPPtr proc = NULL; - qemuProcessQMPPtr procTCG = NULL; int ret = -1; int rc; if (!(proc = qemuProcessQMPNew(qemuCaps->binary, libDir, - runUid, runGid, qmperr, false))) + runUid, runGid, qmperr, onlyTCG))) goto cleanup; if ((rc = qemuProcessQMPRun(proc)) != 0) { @@ -4376,40 +4376,41 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, goto cleanup; } - if (virQEMUCapsInitQMPMonitor(qemuCaps, proc->mon) < 0) - 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); - - procTCG = qemuProcessQMPNew(qemuCaps->binary, libDir, - runUid, runGid, NULL, true); - - if ((rc = qemuProcessQMPRun(procTCG)) != 0) { - if (rc == 1) - ret = 0; - goto cleanup; - } - - if (virQEMUCapsInitQMPMonitorTCG(qemuCaps, procTCG->mon) < 0) - goto cleanup; - } - - ret = 0; + if (onlyTCG) + ret = virQEMUCapsInitQMPMonitorTCG(qemuCaps, proc->mon); + else + ret = virQEMUCapsInitQMPMonitor(qemuCaps, proc->mon); cleanup: qemuProcessQMPStop(proc); - qemuProcessQMPStop(procTCG); qemuProcessQMPFree(proc); - qemuProcessQMPFree(procTCG); return ret; } +static int +virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, + const char *libDir, + uid_t runUid, + gid_t runGid, + char **qmperr) +{ + if (virQEMUCapsInitQMPSingle(qemuCaps, libDir, runUid, runGid, qmperr, false) < 0) + return -1; + + /* + * If KVM was enabled during the first probe, we need to explicitly probe + * for TCG capabilities by asking the same binary again and turning KVM + * off. + */ + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM) && + virQEMUCapsInitQMPSingle(qemuCaps, libDir, runUid, runGid, NULL, true) < 0) + return -1; + + return 0; +} + + #define MESSAGE_ID_CAPS_PROBE_FAILURE "8ae2f3fb-2dbe-498e-8fbd-012d40afa361" static void -- 2.20.1
On Tue, Feb 19, 2019 at 10:04:51AM +0100, Jiri Denemark wrote:
The function contains two almost identical parts. Let's consolidate them into a single helper function and call it twice.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> ---
Notes: Version 7: - new patch
src/qemu/qemu_capabilities.c | 67 ++++++++++++++++++------------------ 1 file changed, 34 insertions(+), 33 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
While qemuProcessQMPRun and virQEMUCapsInitQMPMonitor* functions called from virQEMUCapsInit ignore some errors, the caller of virQEMUCapsInit would report an error unless usedQMP is true anyway. And since usedQMP can only be true if the probing code really succeeded (i.e., no errors were ignored), we can just simplify the logic by not ignoring the errors in the first place. Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- Notes: Version 7: - this is a rewritten version of [PATCH 08/33] qemu_process: All ProcessQMP errors are fatal from version 6 src/qemu/qemu_capabilities.c | 17 +---------------- src/qemu/qemu_process.c | 27 +++++++++++---------------- 2 files changed, 12 insertions(+), 32 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index b695b23fcc..bb1920146e 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -4152,7 +4152,6 @@ virQEMUCapsInitQMPMonitor(virQEMUCapsPtr qemuCaps, if (qemuMonitorSetCapabilities(mon) < 0) { VIR_DEBUG("Failed to set monitor capabilities %s", virGetLastErrorMessage()); - ret = 0; goto cleanup; } @@ -4161,7 +4160,6 @@ virQEMUCapsInitQMPMonitor(virQEMUCapsPtr qemuCaps, &package) < 0) { VIR_DEBUG("Failed to query monitor version %s", virGetLastErrorMessage()); - ret = 0; goto cleanup; } @@ -4338,7 +4336,6 @@ virQEMUCapsInitQMPMonitorTCG(virQEMUCapsPtr qemuCaps ATTRIBUTE_UNUSED, if (qemuMonitorSetCapabilities(mon) < 0) { VIR_DEBUG("Failed to set monitor capabilities %s", virGetLastErrorMessage()); - ret = 0; goto cleanup; } @@ -4364,17 +4361,13 @@ virQEMUCapsInitQMPSingle(virQEMUCapsPtr qemuCaps, { qemuProcessQMPPtr proc = NULL; int ret = -1; - int rc; if (!(proc = qemuProcessQMPNew(qemuCaps->binary, libDir, runUid, runGid, qmperr, onlyTCG))) goto cleanup; - if ((rc = qemuProcessQMPRun(proc)) != 0) { - if (rc == 1) - ret = 0; + if (qemuProcessQMPRun(proc) < 0) goto cleanup; - } if (onlyTCG) ret = virQEMUCapsInitQMPMonitorTCG(qemuCaps, proc->mon); @@ -4474,14 +4467,6 @@ virQEMUCapsNewForBinaryInternal(virArch hostArch, 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; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 6c0f7165c7..728176bbdf 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8392,10 +8392,6 @@ qemuProcessQMPNew(const char *binary, } -/* Returns -1 on fatal error, - * 0 on success, - * 1 when probing QEMU failed - */ int qemuProcessQMPRun(qemuProcessQMPPtr proc) { @@ -8403,6 +8399,7 @@ qemuProcessQMPRun(qemuProcessQMPPtr proc) const char *machine; int status = 0; int ret = -1; + int rc; if (proc->forceTCG) machine = "none,accel=tcg"; @@ -8444,19 +8441,21 @@ qemuProcessQMPRun(qemuProcessQMPPtr proc) virCommandSetErrorBuffer(proc->cmd, proc->qmperr); - /* Log, but otherwise ignore, non-zero status. */ if (virCommandRun(proc->cmd, &status) < 0) goto cleanup; if (status != 0) { - VIR_DEBUG("QEMU %s exited with status %d: %s", - proc->binary, status, *proc->qmperr); - goto ignore; + VIR_DEBUG("QEMU %s exited with status %d", proc->binary, status); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to start QEMU binary %s for probing: %s"), + proc->binary, + *proc->qmperr ? *proc->qmperr : _("unknown error")); + goto cleanup; } - if (virPidFileReadPath(proc->pidfile, &proc->pid) < 0) { - VIR_DEBUG("Failed to read pidfile %s", proc->pidfile); - goto ignore; + if ((rc = virPidFileReadPath(proc->pidfile, &proc->pid)) < 0) { + virReportSystemError(-rc, _("Failed to read pidfile %s"), proc->pidfile); + goto cleanup; } if (!(xmlopt = virDomainXMLOptionNew(NULL, NULL, NULL, NULL, NULL)) || @@ -8467,7 +8466,7 @@ qemuProcessQMPRun(qemuProcessQMPPtr proc) if (!(proc->mon = qemuMonitorOpen(proc->vm, &proc->config, true, true, 0, &callbacks, NULL))) - goto ignore; + goto cleanup; virObjectLock(proc->mon); @@ -8479,10 +8478,6 @@ qemuProcessQMPRun(qemuProcessQMPPtr proc) virObjectUnref(xmlopt); return ret; - - ignore: - ret = 1; - goto cleanup; } -- 2.20.1
On Tue, Feb 19, 2019 at 10:04:52AM +0100, Jiri Denemark wrote:
While qemuProcessQMPRun and virQEMUCapsInitQMPMonitor* functions called from virQEMUCapsInit ignore some errors, the caller of virQEMUCapsInit would report an error unless usedQMP is true anyway. And since usedQMP can only be true if the probing code really succeeded (i.e., no errors were ignored), we can just simplify the logic by not ignoring the errors in the first place.
Thank you.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> ---
Notes: Version 7: - this is a rewritten version of [PATCH 08/33] qemu_process: All ProcessQMP errors are fatal from version 6
src/qemu/qemu_capabilities.c | 17 +---------------- src/qemu/qemu_process.c | 27 +++++++++++---------------- 2 files changed, 12 insertions(+), 32 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Let's push the call to virQEMUCapsLogProbeFailure down the stack to where the probing failure is detected. Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- Notes: Version 7: - this patch was originally included in [PATCH 09/33] qemu_process: Expose process exit status code in version 6 with a bunch of other unrelated changes - modified after virQEMUCapsInitQMP was refactored src/qemu/qemu_capabilities.c | 47 ++++++++++++++++++------------------ 1 file changed, 24 insertions(+), 23 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index bb1920146e..264dcae983 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -4351,6 +4351,26 @@ 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 virQEMUCapsInitQMPSingle(virQEMUCapsPtr qemuCaps, const char *libDir, @@ -4375,6 +4395,9 @@ virQEMUCapsInitQMPSingle(virQEMUCapsPtr qemuCaps, ret = virQEMUCapsInitQMPMonitor(qemuCaps, proc->mon); cleanup: + if (ret < 0) + virQEMUCapsLogProbeFailure(qemuCaps->binary); + qemuProcessQMPStop(proc); qemuProcessQMPFree(proc); return ret; @@ -4404,26 +4427,6 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, } -#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, @@ -4462,10 +4465,8 @@ virQEMUCapsNewForBinaryInternal(virArch hostArch, goto error; } - if (virQEMUCapsInitQMP(qemuCaps, libDir, runUid, runGid, &qmperr) < 0) { - virQEMUCapsLogProbeFailure(binary); + if (virQEMUCapsInitQMP(qemuCaps, libDir, runUid, runGid, &qmperr) < 0) goto error; - } qemuCaps->libvirtCtime = virGetSelfLastChanged(); qemuCaps->libvirtVersion = LIBVIR_VERSION_NUMBER; -- 2.20.1
On Tue, Feb 19, 2019 at 10:04:53AM +0100, Jiri Denemark wrote:
Let's push the call to virQEMUCapsLogProbeFailure down the stack to where the probing failure is detected.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> ---
Notes: Version 7: - this patch was originally included in [PATCH 09/33] qemu_process: Expose process exit status code in version 6 with a bunch of other unrelated changes - modified after virQEMUCapsInitQMP was refactored
src/qemu/qemu_capabilities.c | 47 ++++++++++++++++++------------------ 1 file changed, 24 insertions(+), 23 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Keep the pointer to QEMU stderr output in qemuProcessQMP struct instead of requiring the caller to provide it (and free it). Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- Notes: Version 7: - this is a combination of the relevant parts of [PATCH 09/33] qemu_process: Expose process exit status code and [PATCH 10/33] qemu_process: Persist stderr in qemuProcessQMP struct adapted to the changes made in the previous patches src/qemu/qemu_capabilities.c | 14 +++++--------- src/qemu/qemu_process.c | 9 +++------ src/qemu/qemu_process.h | 3 +-- 3 files changed, 9 insertions(+), 17 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 264dcae983..7460b452a0 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -4376,14 +4376,13 @@ virQEMUCapsInitQMPSingle(virQEMUCapsPtr qemuCaps, const char *libDir, uid_t runUid, gid_t runGid, - char **qmperr, bool onlyTCG) { qemuProcessQMPPtr proc = NULL; int ret = -1; if (!(proc = qemuProcessQMPNew(qemuCaps->binary, libDir, - runUid, runGid, qmperr, onlyTCG))) + runUid, runGid, onlyTCG))) goto cleanup; if (qemuProcessQMPRun(proc) < 0) @@ -4408,10 +4407,9 @@ static int virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, const char *libDir, uid_t runUid, - gid_t runGid, - char **qmperr) + gid_t runGid) { - if (virQEMUCapsInitQMPSingle(qemuCaps, libDir, runUid, runGid, qmperr, false) < 0) + if (virQEMUCapsInitQMPSingle(qemuCaps, libDir, runUid, runGid, false) < 0) return -1; /* @@ -4420,7 +4418,7 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, * off. */ if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM) && - virQEMUCapsInitQMPSingle(qemuCaps, libDir, runUid, runGid, NULL, true) < 0) + virQEMUCapsInitQMPSingle(qemuCaps, libDir, runUid, runGid, true) < 0) return -1; return 0; @@ -4438,7 +4436,6 @@ virQEMUCapsNewForBinaryInternal(virArch hostArch, { virQEMUCapsPtr qemuCaps; struct stat sb; - char *qmperr = NULL; if (!(qemuCaps = virQEMUCapsNew())) goto error; @@ -4465,7 +4462,7 @@ virQEMUCapsNewForBinaryInternal(virArch hostArch, goto error; } - if (virQEMUCapsInitQMP(qemuCaps, libDir, runUid, runGid, &qmperr) < 0) + if (virQEMUCapsInitQMP(qemuCaps, libDir, runUid, runGid) < 0) goto error; qemuCaps->libvirtCtime = virGetSelfLastChanged(); @@ -4484,7 +4481,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 728176bbdf..d2859da2ec 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8335,6 +8335,7 @@ qemuProcessQMPFree(qemuProcessQMPPtr proc) VIR_FREE(proc->monpath); VIR_FREE(proc->monarg); VIR_FREE(proc->pidfile); + VIR_FREE(proc->stderr); VIR_FREE(proc); } @@ -8344,7 +8345,6 @@ qemuProcessQMPNew(const char *binary, const char *libDir, uid_t runUid, gid_t runGid, - char **qmperr, bool forceTCG) { qemuProcessQMPPtr proc = NULL; @@ -8357,7 +8357,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 @@ -8439,7 +8438,7 @@ qemuProcessQMPRun(qemuProcessQMPPtr proc) virCommandSetGID(proc->cmd, proc->runGid); virCommandSetUID(proc->cmd, proc->runUid); - virCommandSetErrorBuffer(proc->cmd, proc->qmperr); + virCommandSetErrorBuffer(proc->cmd, &(proc->stderr)); if (virCommandRun(proc->cmd, &status) < 0) goto cleanup; @@ -8449,7 +8448,7 @@ qemuProcessQMPRun(qemuProcessQMPPtr proc) virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to start QEMU binary %s for probing: %s"), proc->binary, - *proc->qmperr ? *proc->qmperr : _("unknown error")); + proc->stderr ? proc->stderr : _("unknown error")); goto cleanup; } @@ -8513,8 +8512,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 46a0bd2475..e7d4ca0c92 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -220,7 +220,7 @@ struct _qemuProcessQMP { char *binary; uid_t runUid; gid_t runGid; - char **qmperr; + char *stderr; char *monarg; char *monpath; char *pidfile; @@ -236,7 +236,6 @@ qemuProcessQMPPtr qemuProcessQMPNew(const char *binary, const char *libDir, uid_t runUid, gid_t runGid, - char **qmperr, bool forceTCG); void qemuProcessQMPFree(qemuProcessQMPPtr proc); -- 2.20.1
On Tue, Feb 19, 2019 at 10:04:54AM +0100, Jiri Denemark wrote:
Keep the pointer to QEMU stderr output in qemuProcessQMP struct instead of requiring the caller to provide it (and free it).
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> ---
Notes: Version 7: - this is a combination of the relevant parts of [PATCH 09/33] qemu_process: Expose process exit status code and [PATCH 10/33] qemu_process: Persist stderr in qemuProcessQMP struct adapted to the changes made in the previous patches
src/qemu/qemu_capabilities.c | 14 +++++--------- src/qemu/qemu_process.c | 9 +++------ src/qemu/qemu_process.h | 3 +-- 3 files changed, 9 insertions(+), 17 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
From: Chris Venteicher <cventeic@redhat.com> This is a replacement for qemuProcessQMPRun to make the name consistent with qemuProcessStart. The original qemuProcessQMPRun function is renamed as qemuProcessQMPLaunch and becomes one of the simpler functions called from the main qemuProcessQMPStart entry point. The following patches will move parts of the code in qemuProcessQMPLaunch to the other functions (qemuProcessQMPInit and qemuProcessQMPConnectMonitor). Signed-off-by: Chris Venteicher <cventeic@redhat.com> Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- Notes: Version 7: - removed some fairy tales from the commit message and the documentation of qemuProcessQMPStart src/qemu/qemu_capabilities.c | 2 +- src/qemu/qemu_process.c | 74 +++++++++++++++++++++++++++++++++++- src/qemu/qemu_process.h | 2 +- 3 files changed, 74 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 7460b452a0..4fd2169d49 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -4385,7 +4385,7 @@ virQEMUCapsInitQMPSingle(virQEMUCapsPtr qemuCaps, runUid, runGid, onlyTCG))) goto cleanup; - if (qemuProcessQMPRun(proc) < 0) + if (qemuProcessQMPStart(proc) < 0) goto cleanup; if (onlyTCG) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index d2859da2ec..19bc804d18 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8391,8 +8391,21 @@ qemuProcessQMPNew(const char *binary, } -int -qemuProcessQMPRun(qemuProcessQMPPtr proc) +static int +qemuProcessQMPInit(qemuProcessQMPPtr proc) +{ + int ret = -1; + + VIR_DEBUG("proc=%p, emulator=%s", proc, proc->binary); + + ret = 0; + + return ret; +} + + +static int +qemuProcessQMPLaunch(qemuProcessQMPPtr proc) { virDomainXMLOptionPtr xmlopt = NULL; const char *machine; @@ -8480,6 +8493,63 @@ qemuProcessQMPRun(qemuProcessQMPPtr proc) } +static int +qemuProcessQMPConnectMonitor(qemuProcessQMPPtr proc) +{ + int ret = -1; + + VIR_DEBUG("proc=%p, emulator=%s, proc->pid=%lld", + proc, proc->binary, (long long)proc->pid); + + ret = 0; + + return ret; +} + + +/** + * qemuProcessQMPStart: + * @proc: QEMU process and connection state created by qemuProcessQMPNew() + * + * 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); + * + * Process error output (proc->stderr) 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: + 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 e7d4ca0c92..c075ae6445 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -240,7 +240,7 @@ qemuProcessQMPPtr qemuProcessQMPNew(const char *binary, void qemuProcessQMPFree(qemuProcessQMPPtr proc); -int qemuProcessQMPRun(qemuProcessQMPPtr proc); +int qemuProcessQMPStart(qemuProcessQMPPtr proc); void qemuProcessQMPStop(qemuProcessQMPPtr proc); -- 2.20.1
On Tue, Feb 19, 2019 at 10:04:55AM +0100, Jiri Denemark wrote:
From: Chris Venteicher <cventeic@redhat.com>
This is a replacement for qemuProcessQMPRun to make the name consistent with qemuProcessStart. The original qemuProcessQMPRun function is renamed as qemuProcessQMPLaunch and becomes one of the simpler functions called from the main qemuProcessQMPStart entry point. The following patches will move parts of the code in qemuProcessQMPLaunch to the other functions (qemuProcessQMPInit and qemuProcessQMPConnectMonitor).
Signed-off-by: Chris Venteicher <cventeic@redhat.com> Signed-off-by: Jiri Denemark <jdenemar@redhat.com> ---
Notes: Version 7: - removed some fairy tales from the commit message and the documentation
Thank you.
of qemuProcessQMPStart
src/qemu/qemu_capabilities.c | 2 +- src/qemu/qemu_process.c | 74 +++++++++++++++++++++++++++++++++++- src/qemu/qemu_process.h | 2 +- 3 files changed, 74 insertions(+), 4 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
From: Chris Venteicher <cventeic@redhat.com> All code related to QEMU monitor is moved from qemuProcessQMPNew and qemuProcessQMPInit into qemuProcessQMPConnectMonitor. Signed-off-by: Chris Venteicher <cventeic@redhat.com> Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- Notes: Version 7: - simplified commit message - adapted to changes in the preceding patches src/qemu/qemu_process.c | 42 ++++++++++++++++++++--------------------- 1 file changed, 20 insertions(+), 22 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 19bc804d18..85079c8c15 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8379,10 +8379,6 @@ qemuProcessQMPNew(const char *binary, virPidFileForceCleanupPath(proc->pidfile); - proc->config.type = VIR_DOMAIN_CHR_TYPE_UNIX; - proc->config.data.nix.path = proc->monpath; - proc->config.data.nix.listen = false; - return proc; error: @@ -8407,7 +8403,6 @@ qemuProcessQMPInit(qemuProcessQMPPtr proc) static int qemuProcessQMPLaunch(qemuProcessQMPPtr proc) { - virDomainXMLOptionPtr xmlopt = NULL; const char *machine; int status = 0; int ret = -1; @@ -8470,6 +8465,26 @@ qemuProcessQMPLaunch(qemuProcessQMPPtr proc) goto cleanup; } + ret = 0; + + cleanup: + return ret; +} + + +static int +qemuProcessQMPConnectMonitor(qemuProcessQMPPtr proc) +{ + virDomainXMLOptionPtr xmlopt = NULL; + int ret = -1; + + VIR_DEBUG("proc=%p, emulator=%s, proc->pid=%lld", + proc, proc->binary, (long long)proc->pid); + + proc->config.type = VIR_DOMAIN_CHR_TYPE_UNIX; + proc->config.data.nix.path = proc->monpath; + proc->config.data.nix.listen = false; + if (!(xmlopt = virDomainXMLOptionNew(NULL, NULL, NULL, NULL, NULL)) || !(proc->vm = virDomainObjNew(xmlopt))) goto cleanup; @@ -8485,24 +8500,7 @@ qemuProcessQMPLaunch(qemuProcessQMPPtr proc) ret = 0; cleanup: - if (!proc->mon) - qemuProcessQMPStop(proc); virObjectUnref(xmlopt); - - return ret; -} - - -static int -qemuProcessQMPConnectMonitor(qemuProcessQMPPtr proc) -{ - int ret = -1; - - VIR_DEBUG("proc=%p, emulator=%s, proc->pid=%lld", - proc, proc->binary, (long long)proc->pid); - - ret = 0; - return ret; } -- 2.20.1
On Tue, Feb 19, 2019 at 10:04:56AM +0100, Jiri Denemark wrote:
From: Chris Venteicher <cventeic@redhat.com>
All code related to QEMU monitor is moved from qemuProcessQMPNew and qemuProcessQMPInit into qemuProcessQMPConnectMonitor.
Signed-off-by: Chris Venteicher <cventeic@redhat.com> Signed-off-by: Jiri Denemark <jdenemar@redhat.com> ---
Notes: Version 7: - simplified commit message - adapted to changes in the preceding patches
src/qemu/qemu_process.c | 42 ++++++++++++++++++++--------------------- 1 file changed, 20 insertions(+), 22 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
From: Chris Venteicher <cventeic@redhat.com> Store libDir path in the qemuProcessQMP struct in anticipation of moving path construction code into qemuProcessQMPInit function. Signed-off-by: Chris Venteicher <cventeic@redhat.com> Reviewed-by: Jiri Denemark <jdenemar@redhat.com> Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- Notes: Version 7: - no change src/qemu/qemu_process.c | 8 +++++--- src/qemu/qemu_process.h | 1 + 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 85079c8c15..c4cbd7c807 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8332,6 +8332,7 @@ qemuProcessQMPFree(qemuProcessQMPPtr proc) qemuProcessQMPStop(proc); VIR_FREE(proc->binary); + VIR_FREE(proc->libDir); VIR_FREE(proc->monpath); VIR_FREE(proc->monarg); VIR_FREE(proc->pidfile); @@ -8352,7 +8353,8 @@ qemuProcessQMPNew(const char *binary, if (VIR_ALLOC(proc) < 0) goto error; - if (VIR_STRDUP(proc->binary, binary) < 0) + if (VIR_STRDUP(proc->binary, binary) < 0 || + VIR_STRDUP(proc->libDir, libDir) < 0) goto error; proc->runUid = runUid; @@ -8362,7 +8364,7 @@ qemuProcessQMPNew(const char *binary, /* the ".sock" sufix is important to avoid a possible clash with a qemu * domain called "capabilities" */ - if (virAsprintf(&proc->monpath, "%s/%s", libDir, + if (virAsprintf(&proc->monpath, "%s/%s", proc->libDir, "capabilities.monitor.sock") < 0) goto error; if (virAsprintf(&proc->monarg, "unix:%s,server,nowait", proc->monpath) < 0) @@ -8374,7 +8376,7 @@ 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(&proc->pidfile, "%s/%s", libDir, "capabilities.pidfile") < 0) + if (virAsprintf(&proc->pidfile, "%s/%s", proc->libDir, "capabilities.pidfile") < 0) goto error; virPidFileForceCleanupPath(proc->pidfile); diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h index c075ae6445..9ab885c6c3 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -218,6 +218,7 @@ typedef struct _qemuProcessQMP qemuProcessQMP; typedef qemuProcessQMP *qemuProcessQMPPtr; struct _qemuProcessQMP { char *binary; + char *libDir; uid_t runUid; gid_t runGid; char *stderr; -- 2.20.1
On Tue, Feb 19, 2019 at 10:04:57AM +0100, Jiri Denemark wrote:
From: Chris Venteicher <cventeic@redhat.com>
Store libDir path in the qemuProcessQMP struct in anticipation of moving path construction code into qemuProcessQMPInit function.
Signed-off-by: Chris Venteicher <cventeic@redhat.com> Reviewed-by: Jiri Denemark <jdenemar@redhat.com> Signed-off-by: Jiri Denemark <jdenemar@redhat.com> ---
Notes: Version 7: - no change
src/qemu/qemu_process.c | 8 +++++--- src/qemu/qemu_process.h | 1 + 2 files changed, 6 insertions(+), 3 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
From: Chris Venteicher <cventeic@redhat.com> Move code for setting paths and prepping file system from qemuProcessQMPNew to qemuProcessQMPInit. This keeps qemuProcessQMPNew limited to data structures and path initialization is done in qemuProcessQMPInit. The patch is a non-functional, cut / paste change, however goto is now "cleanup" rather than "error". Signed-off-by: Chris Venteicher <cventeic@redhat.com> Reviewed-by: Jiri Denemark <jdenemar@redhat.com> Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- Notes: Version 7: - no change src/qemu/qemu_process.c | 42 +++++++++++++++++++++-------------------- 1 file changed, 22 insertions(+), 20 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index c4cbd7c807..9341aa43a7 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8361,26 +8361,6 @@ qemuProcessQMPNew(const char *binary, proc->runGid = runGid; proc->forceTCG = forceTCG; - /* 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) - goto error; - 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 - * 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) - goto error; - - virPidFileForceCleanupPath(proc->pidfile); - return proc; error: @@ -8396,8 +8376,30 @@ qemuProcessQMPInit(qemuProcessQMPPtr proc) VIR_DEBUG("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) + 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) + goto cleanup; + + virPidFileForceCleanupPath(proc->pidfile); + ret = 0; + cleanup: return ret; } -- 2.20.1
On Tue, Feb 19, 2019 at 10:04:58AM +0100, Jiri Denemark wrote:
From: Chris Venteicher <cventeic@redhat.com>
Move code for setting paths and prepping file system from qemuProcessQMPNew to qemuProcessQMPInit.
This keeps qemuProcessQMPNew limited to data structures and path initialization is done in qemuProcessQMPInit.
The patch is a non-functional, cut / paste change, however goto is now "cleanup" rather than "error".
Signed-off-by: Chris Venteicher <cventeic@redhat.com> Reviewed-by: Jiri Denemark <jdenemar@redhat.com> Signed-off-by: Jiri Denemark <jdenemar@redhat.com> ---
Notes: Version 7: - no change
src/qemu/qemu_process.c | 42 +++++++++++++++++++++-------------------- 1 file changed, 22 insertions(+), 20 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
From: Chris Venteicher <cventeic@redhat.com> The monitor config data is removed from the qemuProcessQMP struct. The monitor config data can be initialized immediately before call to qemuMonitorOpen and does not need to be maintained after the call because qemuMonitorOpen copies any strings it needs. Signed-off-by: Chris Venteicher <cventeic@redhat.com> Reviewed-by: Jiri Denemark <jdenemar@redhat.com> Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- Notes: Version 7: - no change src/qemu/qemu_process.c | 9 +++++---- src/qemu/qemu_process.h | 1 - 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 9341aa43a7..79896b6232 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8480,14 +8480,15 @@ static int qemuProcessQMPConnectMonitor(qemuProcessQMPPtr proc) { virDomainXMLOptionPtr xmlopt = NULL; + virDomainChrSourceDef monConfig; int ret = -1; VIR_DEBUG("proc=%p, emulator=%s, proc->pid=%lld", proc, proc->binary, (long long)proc->pid); - proc->config.type = VIR_DOMAIN_CHR_TYPE_UNIX; - proc->config.data.nix.path = proc->monpath; - proc->config.data.nix.listen = false; + monConfig.type = VIR_DOMAIN_CHR_TYPE_UNIX; + monConfig.data.nix.path = proc->monpath; + monConfig.data.nix.listen = false; if (!(xmlopt = virDomainXMLOptionNew(NULL, NULL, NULL, NULL, NULL)) || !(proc->vm = virDomainObjNew(xmlopt))) @@ -8495,7 +8496,7 @@ qemuProcessQMPConnectMonitor(qemuProcessQMPPtr proc) proc->vm->pid = proc->pid; - if (!(proc->mon = qemuMonitorOpen(proc->vm, &proc->config, true, true, + if (!(proc->mon = qemuMonitorOpen(proc->vm, &monConfig, true, true, 0, &callbacks, NULL))) goto cleanup; diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h index 9ab885c6c3..2422d1eb7c 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -227,7 +227,6 @@ struct _qemuProcessQMP { char *pidfile; virCommandPtr cmd; qemuMonitorPtr mon; - virDomainChrSourceDef config; pid_t pid; virDomainObjPtr vm; bool forceTCG; -- 2.20.1
On Tue, Feb 19, 2019 at 10:04:59AM +0100, Jiri Denemark wrote:
From: Chris Venteicher <cventeic@redhat.com>
The monitor config data is removed from the qemuProcessQMP struct.
The monitor config data can be initialized immediately before call to qemuMonitorOpen and does not need to be maintained after the call because qemuMonitorOpen copies any strings it needs.
Signed-off-by: Chris Venteicher <cventeic@redhat.com> Reviewed-by: Jiri Denemark <jdenemar@redhat.com> Signed-off-by: Jiri Denemark <jdenemar@redhat.com> ---
Notes: Version 7: - no change
src/qemu/qemu_process.c | 9 +++++---- src/qemu/qemu_process.h | 1 - 2 files changed, 5 insertions(+), 5 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
From: Chris Venteicher <cventeic@redhat.com> qemuProcessQMPNew is one of the public functions used to create and manage a QEMU process for QMP command exchanges outside of domain operations. Add descriptive comment block, debug statement and make source consistent with the cleanup / VIR_STEAL_PTR format used elsewhere. Signed-off-by: Chris Venteicher <cventeic@redhat.com> Reviewed-by: Jiri Denemark <jdenemar@redhat.com> Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- Notes: Version 7: - just a few small tweaks, mainly in the commit message and comments src/qemu/qemu_process.c | 25 ++++++++++++++++++++----- 1 file changed, 20 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 79896b6232..b5d127b7d3 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8341,6 +8341,17 @@ qemuProcessQMPFree(qemuProcessQMPPtr proc) } +/** + * qemuProcessQMPNew: + * @binary: QEMU binary + * @libDir: Directory for process and connection artifacts + * @runUid: UserId for QEMU process + * @runGid: GroupId for QEMU process + * @forceTCG: Force TCG mode if true + * + * Allocate and initialize domain structure encapsulating QEMU process state + * and monitor connection for completing QMP queries. + */ qemuProcessQMPPtr qemuProcessQMPNew(const char *binary, const char *libDir, @@ -8348,24 +8359,28 @@ qemuProcessQMPNew(const char *binary, gid_t runGid, bool forceTCG) { + qemuProcessQMPPtr ret = NULL; qemuProcessQMPPtr proc = NULL; + VIR_DEBUG("exec=%s, libDir=%s, runUid=%u, runGid=%u, forceTCG=%d", + binary, libDir, runUid, runGid, forceTCG); + if (VIR_ALLOC(proc) < 0) - goto error; + goto cleanup; if (VIR_STRDUP(proc->binary, binary) < 0 || VIR_STRDUP(proc->libDir, libDir) < 0) - goto error; + goto cleanup; proc->runUid = runUid; proc->runGid = runGid; proc->forceTCG = forceTCG; - return proc; + VIR_STEAL_PTR(ret, proc); - error: + cleanup: qemuProcessQMPFree(proc); - return NULL; + return ret; } -- 2.20.1
On Tue, Feb 19, 2019 at 10:05:00AM +0100, Jiri Denemark wrote:
From: Chris Venteicher <cventeic@redhat.com>
qemuProcessQMPNew is one of the public functions used to create and manage a QEMU process for QMP command exchanges outside of domain operations.
Add descriptive comment block, debug statement and make source consistent with the cleanup / VIR_STEAL_PTR format used elsewhere.
Signed-off-by: Chris Venteicher <cventeic@redhat.com> Reviewed-by: Jiri Denemark <jdenemar@redhat.com> Signed-off-by: Jiri Denemark <jdenemar@redhat.com> ---
Notes: Version 7: - just a few small tweaks, mainly in the commit message and comments
src/qemu/qemu_process.c | 25 ++++++++++++++++++++----- 1 file changed, 20 insertions(+), 5 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Users qemuProcessQMP struct were always forced to call both qemuProcessQMPStop and qemuProcessQMPFree when they are done with the process. We can just call qemuProcessQMPStop from qemuProcessQMPFree and let users call qemuProcessQMPFree only. Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- Notes: Version 7: - new patch src/qemu/qemu_capabilities.c | 1 - src/qemu/qemu_process.c | 92 ++++++++++++++++++------------------ src/qemu/qemu_process.h | 2 - 3 files changed, 45 insertions(+), 50 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 4fd2169d49..a546d9b5a1 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -4397,7 +4397,6 @@ virQEMUCapsInitQMPSingle(virQEMUCapsPtr qemuCaps, if (ret < 0) virQEMUCapsLogProbeFailure(qemuCaps->binary); - qemuProcessQMPStop(proc); qemuProcessQMPFree(proc); return ret; } diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index b5d127b7d3..ca5911bacd 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8324,6 +8324,49 @@ static qemuMonitorCallbacks callbacks = { }; +static void +qemuProcessQMPStop(qemuProcessQMPPtr proc) +{ + if (proc->mon) { + virObjectUnlock(proc->mon); + qemuMonitorClose(proc->mon); + proc->mon = NULL; + } + + if (proc->cmd) { + virCommandAbort(proc->cmd); + virCommandFree(proc->cmd); + proc->cmd = NULL; + } + + if (proc->monpath) + unlink(proc->monpath); + + virDomainObjEndAPI(&proc->vm); + + if (proc->pid != 0) { + char ebuf[1024]; + + 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)proc->pid, + virStrerror(errno, ebuf, sizeof(ebuf))); + + proc->pid = 0; + } + + if (proc->pidfile) + unlink(proc->pidfile); +} + + +/** + * qemuProcessQMPFree: + * @proc: Stores process and connection state + * + * Kill QEMU process and free process data structure. + */ void qemuProcessQMPFree(qemuProcessQMPPtr proc) { @@ -8535,7 +8578,6 @@ qemuProcessQMPConnectMonitor(qemuProcessQMPPtr proc) * proc = qemuProcessQMPNew(binary, libDir, runUid, runGid, forceTCG); * qemuProcessQMPStart(proc); * ** Send QMP Queries to QEMU using monitor (proc->mon) ** - * qemuProcessQMPStop(proc); * qemuProcessQMPFree(proc); * * Process error output (proc->stderr) remains available in qemuProcessQMP @@ -8552,57 +8594,13 @@ qemuProcessQMPStart(qemuProcessQMPPtr proc) goto cleanup; if (qemuProcessQMPLaunch(proc) < 0) - goto stop; + goto cleanup; if (qemuProcessQMPConnectMonitor(proc) < 0) - goto stop; + goto cleanup; ret = 0; cleanup: return ret; - - stop: - qemuProcessQMPStop(proc); - goto cleanup; -} - - -void -qemuProcessQMPStop(qemuProcessQMPPtr proc) -{ - if (!proc) - return; - - if (proc->mon) { - virObjectUnlock(proc->mon); - qemuMonitorClose(proc->mon); - proc->mon = NULL; - } - - if (proc->cmd) { - virCommandAbort(proc->cmd); - virCommandFree(proc->cmd); - proc->cmd = NULL; - } - - if (proc->monpath) - unlink(proc->monpath); - - virDomainObjEndAPI(&proc->vm); - - if (proc->pid != 0) { - char ebuf[1024]; - - 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)proc->pid, - virStrerror(errno, ebuf, sizeof(ebuf))); - - proc->pid = 0; - } - - if (proc->pidfile) - unlink(proc->pidfile); } diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h index 2422d1eb7c..c435a19726 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -242,6 +242,4 @@ void qemuProcessQMPFree(qemuProcessQMPPtr proc); int qemuProcessQMPStart(qemuProcessQMPPtr proc); -void qemuProcessQMPStop(qemuProcessQMPPtr proc); - #endif /* LIBVIRT_QEMU_PROCESS_H */ -- 2.20.1
On Tue, Feb 19, 2019 at 10:05:01AM +0100, Jiri Denemark wrote:
Users qemuProcessQMP struct were always forced to call both qemuProcessQMPStop and qemuProcessQMPFree when they are done with the process. We can just call qemuProcessQMPStop from qemuProcessQMPFree and let users call qemuProcessQMPFree only.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> ---
Notes: Version 7: - new patch
src/qemu/qemu_capabilities.c | 1 - src/qemu/qemu_process.c | 92 ++++++++++++++++++------------------ src/qemu/qemu_process.h | 2 - 3 files changed, 45 insertions(+), 50 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
From: Chris Venteicher <cventeic@redhat.com> Multiple QEMU processes for QMP commands can operate concurrently. Use a unique directory under libDir for each QEMU process 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> Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- Notes: Version 7: - report error if mkdtemp fails src/qemu/qemu_process.c | 30 ++++++++++++++++++++---------- src/qemu/qemu_process.h | 1 + 2 files changed, 21 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index ca5911bacd..820476dbb1 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8358,6 +8358,9 @@ qemuProcessQMPStop(qemuProcessQMPPtr proc) if (proc->pidfile) unlink(proc->pidfile); + + if (proc->uniqDir) + rmdir(proc->uniqDir); } @@ -8376,6 +8379,7 @@ qemuProcessQMPFree(qemuProcessQMPPtr proc) qemuProcessQMPStop(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); @@ -8430,31 +8434,37 @@ qemuProcessQMPNew(const char *binary, static int qemuProcessQMPInit(qemuProcessQMPPtr proc) { + char *template = NULL; int ret = -1; VIR_DEBUG("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/qmp-XXXXXX", proc->libDir) < 0) + goto cleanup; + + if (!(proc->uniqDir = mkdtemp(template))) { + virReportSystemError(errno, + _("Failed to create unique directory with " + "template '%s' for probing QEMU"), + template); + goto cleanup; + } + + 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: diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h index c435a19726..3367cd3fe5 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -225,6 +225,7 @@ struct _qemuProcessQMP { char *monarg; char *monpath; char *pidfile; + char *uniqDir; virCommandPtr cmd; qemuMonitorPtr mon; pid_t pid; -- 2.20.1
On Tue, Feb 19, 2019 at 10:05:02AM +0100, Jiri Denemark wrote:
From: Chris Venteicher <cventeic@redhat.com>
Multiple QEMU processes for QMP commands can operate concurrently.
Use a unique directory under libDir for each QEMU process 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> Signed-off-by: Jiri Denemark <jdenemar@redhat.com> ---
Notes: Version 7: - report error if mkdtemp fails
src/qemu/qemu_process.c | 30 ++++++++++++++++++++---------- src/qemu/qemu_process.h | 1 + 2 files changed, 21 insertions(+), 10 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
From: Chris Venteicher <cventeic@redhat.com> qemuProcessQMPStart starts a QEMU process and monitor connection that can be used by multiple functions possibly for multiple QMP commands. The QMP exchange to exit capabilities negotiation mode and enter command mode can only be performed once after the monitor connection is established. Move responsibility for entering QMP command mode into the qemuProcessQMP code so multiple functions can issue QMP commands in arbitrary orders. This also simplifies the functions using the connection provided by qemuProcessQMPStart to issue QMP commands. Test code now needs to call qemuMonitorSetCapabilities to send the message to switch to command mode because the test code does not use the qemuProcessQMP command that internally calls qemuMonitorSetCapabilities. Signed-off-by: Chris Venteicher <cventeic@redhat.com> Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- Notes: Version 7: - qemuMonitorSetCapabilities call was moved into a new qemuProcessQMPInitMonitor function, which is called from qemuProcessQMPConnectMonitor and the tests src/qemu/qemu_capabilities.c | 12 ------------ src/qemu/qemu_process.c | 16 ++++++++++++++++ src/qemu/qemu_processpriv.h | 2 ++ tests/qemucapabilitiestest.c | 9 +++++++++ 4 files changed, 27 insertions(+), 12 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index a546d9b5a1..b48bcbebee 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -4149,12 +4149,6 @@ virQEMUCapsInitQMPMonitor(virQEMUCapsPtr qemuCaps, /* @mon is supposed to be locked by callee */ - if (qemuMonitorSetCapabilities(mon) < 0) { - VIR_DEBUG("Failed to set monitor capabilities %s", - virGetLastErrorMessage()); - goto cleanup; - } - if (qemuMonitorGetVersion(mon, &major, &minor, µ, &package) < 0) { @@ -4333,12 +4327,6 @@ virQEMUCapsInitQMPMonitorTCG(virQEMUCapsPtr qemuCaps ATTRIBUTE_UNUSED, { int ret = -1; - if (qemuMonitorSetCapabilities(mon) < 0) { - VIR_DEBUG("Failed to set monitor capabilities %s", - virGetLastErrorMessage()); - goto cleanup; - } - if (virQEMUCapsProbeQMPCPUDefinitions(qemuCaps, mon, true) < 0) goto cleanup; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 820476dbb1..85952b9975 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8544,6 +8544,19 @@ qemuProcessQMPLaunch(qemuProcessQMPPtr proc) } +int +qemuProcessQMPInitMonitor(qemuMonitorPtr mon) +{ + if (qemuMonitorSetCapabilities(mon) < 0) { + VIR_DEBUG("Failed to set monitor capabilities %s", + virGetLastErrorMessage()); + return -1; + } + + return 0; +} + + static int qemuProcessQMPConnectMonitor(qemuProcessQMPPtr proc) { @@ -8570,6 +8583,9 @@ qemuProcessQMPConnectMonitor(qemuProcessQMPPtr proc) virObjectLock(proc->mon); + if (qemuProcessQMPInitMonitor(proc->mon) < 0) + goto cleanup; + ret = 0; cleanup: diff --git a/src/qemu/qemu_processpriv.h b/src/qemu/qemu_processpriv.h index 17f981829b..237d7bad58 100644 --- a/src/qemu/qemu_processpriv.h +++ b/src/qemu/qemu_processpriv.h @@ -38,4 +38,6 @@ int qemuProcessHandleDeviceDeleted(qemuMonitorPtr mon, const char *devAlias, void *opaque); +int qemuProcessQMPInitMonitor(qemuMonitorPtr mon); + #endif /* LIBVIRT_QEMU_PROCESSPRIV_H */ diff --git a/tests/qemucapabilitiestest.c b/tests/qemucapabilitiestest.c index fab77db7ce..8d47133e6f 100644 --- a/tests/qemucapabilitiestest.c +++ b/tests/qemucapabilitiestest.c @@ -26,6 +26,8 @@ #include "qemu/qemu_capspriv.h" #define LIBVIRT_QEMU_MONITOR_PRIV_H_ALLOW #include "qemu/qemu_monitor_priv.h" +#define LIBVIRT_QEMU_PROCESSPRIV_H_ALLOW +#include "qemu/qemu_processpriv.h" #define VIR_FROM_THIS VIR_FROM_NONE @@ -60,6 +62,9 @@ testQemuCaps(const void *opaque) if (!(mon = qemuMonitorTestNewFromFileFull(repliesFile, &data->driver, NULL))) goto cleanup; + if (qemuProcessQMPInitMonitor(qemuMonitorTestGetMonitor(mon)) < 0) + goto cleanup; + if (!(capsActual = virQEMUCapsNew()) || virQEMUCapsInitQMPMonitor(capsActual, qemuMonitorTestGetMonitor(mon)) < 0) @@ -67,6 +72,10 @@ testQemuCaps(const void *opaque) if (virQEMUCapsGet(capsActual, QEMU_CAPS_KVM)) { qemuMonitorResetCommandID(qemuMonitorTestGetMonitor(mon)); + + if (qemuProcessQMPInitMonitor(qemuMonitorTestGetMonitor(mon)) < 0) + goto cleanup; + if (virQEMUCapsInitQMPMonitorTCG(capsActual, qemuMonitorTestGetMonitor(mon)) < 0) goto cleanup; -- 2.20.1
On Tue, Feb 19, 2019 at 10:05:03AM +0100, Jiri Denemark wrote:
From: Chris Venteicher <cventeic@redhat.com>
qemuProcessQMPStart starts a QEMU process and monitor connection that can be used by multiple functions possibly for multiple QMP commands.
The QMP exchange to exit capabilities negotiation mode and enter command mode can only be performed once after the monitor connection is established.
Move responsibility for entering QMP command mode into the qemuProcessQMP code so multiple functions can issue QMP commands in arbitrary orders.
This also simplifies the functions using the connection provided by qemuProcessQMPStart to issue QMP commands.
Test code now needs to call qemuMonitorSetCapabilities to send the message to switch to command mode because the test code does not use the qemuProcessQMP command that internally calls qemuMonitorSetCapabilities.
Signed-off-by: Chris Venteicher <cventeic@redhat.com> Signed-off-by: Jiri Denemark <jdenemar@redhat.com> ---
Notes: Version 7: - qemuMonitorSetCapabilities call was moved into a new qemuProcessQMPInitMonitor function, which is called from qemuProcessQMPConnectMonitor and the tests
src/qemu/qemu_capabilities.c | 12 ------------ src/qemu/qemu_process.c | 16 ++++++++++++++++ src/qemu/qemu_processpriv.h | 2 ++ tests/qemucapabilitiestest.c | 9 +++++++++ 4 files changed, 27 insertions(+), 12 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (2)
-
Jiri Denemark -
Ján Tomko