[libvirt] [PATCH RFC 00/22] Move process code to qemu_process
Make process code usable outside qemu_capabilities by moving code from qemu_capabilities to qemu_process and exposing public functions. The process code is used to activate a non domain QEMU process for QMP message exchanges. This patch set modifies capabilities to use the new public functions. -- The process code is being decoupled from qemu_capabilities now to support hypervisor baseline and comparison using QMP commands. This patch set was originally submitted as part of the baseline patch set: [libvirt] [PATCH v4 00/37] BaselineHypervisorCPU using QEMU QMP exchanges https://www.redhat.com/archives/libvir-list/2018-November/msg00091.html The baseline and comparison requirements are described here: https://bugzilla.redhat.com/show_bug.cgi?id=1511999 https://bugzilla.redhat.com/show_bug.cgi?id=1511996 I am extracting and resubmitting just the process changes as a stand alone series to try to make review easier. The patch set shows capabilities using the public functions. To see baseline using the public functions... Look at the "qemu_driver:" patches at the end of https://www.redhat.com/archives/libvir-list/2018-November/msg00091.html Also, The "qemu_driver: Support feature expansion via QEMU when baselining cpu" patch might be of particular interest because the same QEMU process is used for both baseline and expansion using QMP commands. -- Many patches were used to isolate code moves and name changes from other actual implementation changes. The patches reuse the pattern of public qemuProcess{Start,Stop} functions and internal static qemuProcess{Init,Launch,ConnectMonitor} functions but adds a "Qmp" suffix to make them unique. A number of patches are about re-partitioning the code into static functions for initialization, process launch and connection monitor stuff. This matches the established pattern in qemu_process and seemed to make sense to do. For concurrency... A thread safe library function creates a unique directory under libDir for each QEMU process (for QMP messaging) to decouple processes in terms of sockets and file system footprint. Every patch should compile independently if applied in sequence. Chris Venteicher (22): qemu_process: Move process code from qemu_capabilities to qemu_process qemu_process: Use qemuProcess prefix qemu_process: Limit qemuProcessNew 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 qemuProcess struct for a single process qemu_process: Persist stderr in qemuProcess struct qemu_capabilities: Detect caps probe failure by checking monitor ptr qemu_process: Introduce qemuProcessStartQmp qemu_process: Collect monitor code in single function qemu_process: Store libDir in qemuProcess struct qemu_process: Setup paths within qemuProcessInitQmp qemu_process: Stop retaining Qemu Monitor config in qemuProcess qemu_process: Don't open monitor if process failed qemu_process: Cleanup qemuProcess alloc function qemu_process: Cleanup qemuProcessStopQmp function qemu_process: Catch process free before process stop qemu_monitor: Make monitor callbacks optional qemu_process: Enter QMP command mode when starting QEMU Process qemu_process: Use unique directories for QMP processes qemu_process: Stop locking QMP process monitor immediately src/qemu/qemu_capabilities.c | 300 +++++------------------------ src/qemu/qemu_monitor.c | 4 +- src/qemu/qemu_process.c | 356 +++++++++++++++++++++++++++++++++++ src/qemu/qemu_process.h | 37 ++++ tests/qemucapabilitiestest.c | 7 + 5 files changed, 444 insertions(+), 260 deletions(-) -- 2.17.1
Qemu process code in qemu_capabilities.c is moved to qemu_process.c in order to make the code usable outside the original capabilities usecases. This process code activates and manages Qemu processes without establishing a guest domain. This patch is a straight cut/paste move between files. Following patches modify the process code making it more generic and consistent with qemu_process. Signed-off-by: Chris Venteicher <cventeic@redhat.com> --- src/qemu/qemu_capabilities.c | 218 +---------------------------------- src/qemu/qemu_process.c | 201 ++++++++++++++++++++++++++++++++ src/qemu/qemu_process.h | 29 +++++ 3 files changed, 231 insertions(+), 217 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 2ca5af3297..0f70fdf46d 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -47,6 +47,7 @@ #define __QEMU_CAPSPRIV_H_ALLOW__ #include "qemu_capspriv.h" #include "qemu_qapi.h" +#include "qemu_process.h" #include <fcntl.h> #include <sys/stat.h> @@ -3917,18 +3918,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 @@ -4223,211 +4212,6 @@ virQEMUCapsInitQMPMonitorTCG(virQEMUCapsPtr qemuCaps ATTRIBUTE_UNUSED, } -typedef struct _virQEMUCapsInitQMPCommand virQEMUCapsInitQMPCommand; -typedef virQEMUCapsInitQMPCommand *virQEMUCapsInitQMPCommandPtr; -struct _virQEMUCapsInitQMPCommand { - char *binary; - uid_t runUid; - gid_t runGid; - char **qmperr; - char *monarg; - char *monpath; - char *pidfile; - virCommandPtr cmd; - qemuMonitorPtr mon; - virDomainChrSourceDef config; - pid_t pid; - virDomainObjPtr vm; -}; - - -static void -virQEMUCapsInitQMPCommandAbort(virQEMUCapsInitQMPCommandPtr cmd) -{ - if (cmd->mon) - virObjectUnlock(cmd->mon); - qemuMonitorClose(cmd->mon); - cmd->mon = NULL; - - virCommandAbort(cmd->cmd); - virCommandFree(cmd->cmd); - cmd->cmd = NULL; - - if (cmd->monpath) - unlink(cmd->monpath); - - virDomainObjEndAPI(&cmd->vm); - - if (cmd->pid != 0) { - char ebuf[1024]; - - VIR_DEBUG("Killing QMP caps process %lld", (long long)cmd->pid); - if (virProcessKill(cmd->pid, SIGKILL) < 0 && errno != ESRCH) - VIR_ERROR(_("Failed to kill process %lld: %s"), - (long long)cmd->pid, - virStrerror(errno, ebuf, sizeof(ebuf))); - - VIR_FREE(*cmd->qmperr); - } - if (cmd->pidfile) - unlink(cmd->pidfile); - cmd->pid = 0; -} - - -static void -virQEMUCapsInitQMPCommandFree(virQEMUCapsInitQMPCommandPtr cmd) -{ - if (!cmd) - return; - - virQEMUCapsInitQMPCommandAbort(cmd); - VIR_FREE(cmd->binary); - VIR_FREE(cmd->monpath); - VIR_FREE(cmd->monarg); - VIR_FREE(cmd->pidfile); - VIR_FREE(cmd); -} - - -static virQEMUCapsInitQMPCommandPtr -virQEMUCapsInitQMPCommandNew(char *binary, - const char *libDir, - uid_t runUid, - gid_t runGid, - char **qmperr) -{ - virQEMUCapsInitQMPCommandPtr cmd = NULL; - - if (VIR_ALLOC(cmd) < 0) - goto error; - - if (VIR_STRDUP(cmd->binary, binary) < 0) - goto error; - - cmd->runUid = runUid; - cmd->runGid = runGid; - cmd->qmperr = qmperr; - - /* the ".sock" sufix is important to avoid a possible clash with a qemu - * domain called "capabilities" - */ - if (virAsprintf(&cmd->monpath, "%s/%s", libDir, - "capabilities.monitor.sock") < 0) - goto error; - if (virAsprintf(&cmd->monarg, "unix:%s,server,nowait", cmd->monpath) < 0) - goto error; - - /* ".pidfile" suffix is used rather than ".pid" to avoid a possible clash - * with a qemu domain called "capabilities" - * Normally we'd use runDir for pid files, but because we're using - * -daemonize we need QEMU to be allowed to create them, rather - * than libvirtd. So we're using libDir which QEMU can write to - */ - if (virAsprintf(&cmd->pidfile, "%s/%s", libDir, "capabilities.pidfile") < 0) - goto error; - - virPidFileForceCleanupPath(cmd->pidfile); - - cmd->config.type = VIR_DOMAIN_CHR_TYPE_UNIX; - cmd->config.data.nix.path = cmd->monpath; - cmd->config.data.nix.listen = false; - - return cmd; - - error: - virQEMUCapsInitQMPCommandFree(cmd); - return NULL; -} - - -/* Returns -1 on fatal error, - * 0 on success, - * 1 when probing QEMU failed - */ -static int -virQEMUCapsInitQMPCommandRun(virQEMUCapsInitQMPCommandPtr cmd, - bool forceTCG) -{ - virDomainXMLOptionPtr xmlopt = NULL; - const char *machine; - int status = 0; - int ret = -1; - - if (forceTCG) - machine = "none,accel=tcg"; - else - machine = "none,accel=kvm:tcg"; - - VIR_DEBUG("Try to probe capabilities of '%s' via QMP, machine %s", - cmd->binary, machine); - - /* - * We explicitly need to use -daemonize here, rather than - * virCommandDaemonize, because we need to synchronize - * with QEMU creating its monitor socket API. Using - * daemonize guarantees control won't return to libvirt - * until the socket is present. - */ - cmd->cmd = virCommandNewArgList(cmd->binary, - "-S", - "-no-user-config", - "-nodefaults", - "-nographic", - "-machine", machine, - "-qmp", cmd->monarg, - "-pidfile", cmd->pidfile, - "-daemonize", - NULL); - virCommandAddEnvPassCommon(cmd->cmd); - virCommandClearCaps(cmd->cmd); - virCommandSetGID(cmd->cmd, cmd->runGid); - virCommandSetUID(cmd->cmd, cmd->runUid); - - virCommandSetErrorBuffer(cmd->cmd, cmd->qmperr); - - /* Log, but otherwise ignore, non-zero status. */ - if (virCommandRun(cmd->cmd, &status) < 0) - goto cleanup; - - if (status != 0) { - VIR_DEBUG("QEMU %s exited with status %d: %s", - cmd->binary, status, *cmd->qmperr); - goto ignore; - } - - if (virPidFileReadPath(cmd->pidfile, &cmd->pid) < 0) { - VIR_DEBUG("Failed to read pidfile %s", cmd->pidfile); - goto ignore; - } - - if (!(xmlopt = virDomainXMLOptionNew(NULL, NULL, NULL, NULL, NULL)) || - !(cmd->vm = virDomainObjNew(xmlopt))) - goto cleanup; - - cmd->vm->pid = cmd->pid; - - if (!(cmd->mon = qemuMonitorOpen(cmd->vm, &cmd->config, true, true, - 0, &callbacks, NULL))) - goto ignore; - - virObjectLock(cmd->mon); - - ret = 0; - - cleanup: - if (!cmd->mon) - virQEMUCapsInitQMPCommandAbort(cmd); - virObjectUnref(xmlopt); - - return ret; - - ignore: - ret = 1; - goto cleanup; -} - - static int virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, const char *libDir, diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 06a65b44e4..0b3922fa39 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8064,3 +8064,204 @@ qemuProcessReconnectAll(virQEMUDriverPtr driver) struct qemuProcessReconnectData data = {.driver = driver}; virDomainObjListForEach(driver->domains, qemuProcessReconnectHelper, &data); } + + +static void virQEMUCapsMonitorNotify(qemuMonitorPtr mon ATTRIBUTE_UNUSED, + virDomainObjPtr vm ATTRIBUTE_UNUSED, + void *opaque ATTRIBUTE_UNUSED) +{ +} + +static qemuMonitorCallbacks callbacks = { + .eofNotify = virQEMUCapsMonitorNotify, + .errorNotify = virQEMUCapsMonitorNotify, +}; + + + + +void +virQEMUCapsInitQMPCommandFree(virQEMUCapsInitQMPCommandPtr cmd) +{ + if (!cmd) + return; + + virQEMUCapsInitQMPCommandAbort(cmd); + VIR_FREE(cmd->binary); + VIR_FREE(cmd->monpath); + VIR_FREE(cmd->monarg); + VIR_FREE(cmd->pidfile); + VIR_FREE(cmd); +} + + +virQEMUCapsInitQMPCommandPtr +virQEMUCapsInitQMPCommandNew(char *binary, + const char *libDir, + uid_t runUid, + gid_t runGid, + char **qmperr) +{ + virQEMUCapsInitQMPCommandPtr cmd = NULL; + + if (VIR_ALLOC(cmd) < 0) + goto error; + + if (VIR_STRDUP(cmd->binary, binary) < 0) + goto error; + + cmd->runUid = runUid; + cmd->runGid = runGid; + cmd->qmperr = qmperr; + + /* the ".sock" sufix is important to avoid a possible clash with a qemu + * domain called "capabilities" + */ + if (virAsprintf(&cmd->monpath, "%s/%s", libDir, + "capabilities.monitor.sock") < 0) + goto error; + if (virAsprintf(&cmd->monarg, "unix:%s,server,nowait", cmd->monpath) < 0) + goto error; + + /* ".pidfile" suffix is used rather than ".pid" to avoid a possible clash + * with a qemu domain called "capabilities" + * Normally we'd use runDir for pid files, but because we're using + * -daemonize we need QEMU to be allowed to create them, rather + * than libvirtd. So we're using libDir which QEMU can write to + */ + if (virAsprintf(&cmd->pidfile, "%s/%s", libDir, "capabilities.pidfile") < 0) + goto error; + + virPidFileForceCleanupPath(cmd->pidfile); + + cmd->config.type = VIR_DOMAIN_CHR_TYPE_UNIX; + cmd->config.data.nix.path = cmd->monpath; + cmd->config.data.nix.listen = false; + + return cmd; + + error: + virQEMUCapsInitQMPCommandFree(cmd); + return NULL; +} + + +/* Returns -1 on fatal error, + * 0 on success, + * 1 when probing QEMU failed + */ +int +virQEMUCapsInitQMPCommandRun(virQEMUCapsInitQMPCommandPtr cmd, + bool forceTCG) +{ + virDomainXMLOptionPtr xmlopt = NULL; + const char *machine; + int status = 0; + int ret = -1; + + if (forceTCG) + machine = "none,accel=tcg"; + else + machine = "none,accel=kvm:tcg"; + + VIR_DEBUG("Try to probe capabilities of '%s' via QMP, machine %s", + cmd->binary, machine); + + /* + * We explicitly need to use -daemonize here, rather than + * virCommandDaemonize, because we need to synchronize + * with QEMU creating its monitor socket API. Using + * daemonize guarantees control won't return to libvirt + * until the socket is present. + */ + cmd->cmd = virCommandNewArgList(cmd->binary, + "-S", + "-no-user-config", + "-nodefaults", + "-nographic", + "-machine", machine, + "-qmp", cmd->monarg, + "-pidfile", cmd->pidfile, + "-daemonize", + NULL); + virCommandAddEnvPassCommon(cmd->cmd); + virCommandClearCaps(cmd->cmd); + virCommandSetGID(cmd->cmd, cmd->runGid); + virCommandSetUID(cmd->cmd, cmd->runUid); + + virCommandSetErrorBuffer(cmd->cmd, cmd->qmperr); + + /* Log, but otherwise ignore, non-zero status. */ + if (virCommandRun(cmd->cmd, &status) < 0) + goto cleanup; + + if (status != 0) { + VIR_DEBUG("QEMU %s exited with status %d: %s", + cmd->binary, status, *cmd->qmperr); + goto ignore; + } + + if (virPidFileReadPath(cmd->pidfile, &cmd->pid) < 0) { + VIR_DEBUG("Failed to read pidfile %s", cmd->pidfile); + goto ignore; + } + + if (!(xmlopt = virDomainXMLOptionNew(NULL, NULL, NULL, NULL, NULL)) || + !(cmd->vm = virDomainObjNew(xmlopt))) + goto cleanup; + + cmd->vm->pid = cmd->pid; + + if (!(cmd->mon = qemuMonitorOpen(cmd->vm, &cmd->config, true, true, + 0, &callbacks, NULL))) + goto ignore; + + virObjectLock(cmd->mon); + + ret = 0; + + cleanup: + if (!cmd->mon) + virQEMUCapsInitQMPCommandAbort(cmd); + virObjectUnref(xmlopt); + + return ret; + + ignore: + ret = 1; + goto cleanup; +} + + +void +virQEMUCapsInitQMPCommandAbort(virQEMUCapsInitQMPCommandPtr cmd) +{ + if (cmd->mon) + virObjectUnlock(cmd->mon); + qemuMonitorClose(cmd->mon); + cmd->mon = NULL; + + virCommandAbort(cmd->cmd); + virCommandFree(cmd->cmd); + cmd->cmd = NULL; + + if (cmd->monpath) + unlink(cmd->monpath); + + virDomainObjEndAPI(&cmd->vm); + + if (cmd->pid != 0) { + char ebuf[1024]; + + VIR_DEBUG("Killing QMP caps process %lld", (long long)cmd->pid); + if (virProcessKill(cmd->pid, SIGKILL) < 0 && errno != ESRCH) + VIR_ERROR(_("Failed to kill process %lld: %s"), + (long long)cmd->pid, + virStrerror(errno, ebuf, sizeof(ebuf))); + + VIR_FREE(*cmd->qmperr); + } + if (cmd->pidfile) + unlink(cmd->pidfile); + cmd->pid = 0; +} diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h index 2037467c94..4ba3988e3d 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -214,4 +214,33 @@ int qemuProcessStartManagedPRDaemon(virDomainObjPtr vm); void qemuProcessKillManagedPRDaemon(virDomainObjPtr vm); +typedef struct _virQEMUCapsInitQMPCommand virQEMUCapsInitQMPCommand; +typedef virQEMUCapsInitQMPCommand *virQEMUCapsInitQMPCommandPtr; +struct _virQEMUCapsInitQMPCommand { + char *binary; + uid_t runUid; + gid_t runGid; + char **qmperr; + char *monarg; + char *monpath; + char *pidfile; + virCommandPtr cmd; + qemuMonitorPtr mon; + virDomainChrSourceDef config; + pid_t pid; + virDomainObjPtr vm; +}; + +virQEMUCapsInitQMPCommandPtr virQEMUCapsInitQMPCommandNew(char *binary, + const char *libDir, + uid_t runUid, + gid_t runGid, + char **qmperr); + +void virQEMUCapsInitQMPCommandFree(virQEMUCapsInitQMPCommandPtr cmd); + +int virQEMUCapsInitQMPCommandRun(virQEMUCapsInitQMPCommandPtr cmd, bool forceTCG); + +void virQEMUCapsInitQMPCommandAbort(virQEMUCapsInitQMPCommandPtr cmd); + #endif /* __QEMU_PROCESS_H__ */ -- 2.17.1
On Sun, Nov 11, 2018 at 13:59:09 -0600, Chris Venteicher wrote:
Qemu process code in qemu_capabilities.c is moved to qemu_process.c in order to make the code usable outside the original capabilities usecases.
This process code activates and manages Qemu processes without establishing a guest domain.
This patch is a straight cut/paste move between files.
Following patches modify the process code making it more generic and consistent with qemu_process.
Signed-off-by: Chris Venteicher <cventeic@redhat.com> --- src/qemu/qemu_capabilities.c | 218 +---------------------------------- src/qemu/qemu_process.c | 201 ++++++++++++++++++++++++++++++++ src/qemu/qemu_process.h | 29 +++++ 3 files changed, 231 insertions(+), 217 deletions(-)
...
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 06a65b44e4..0b3922fa39 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8064,3 +8064,204 @@ 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, +}; + + + +
Two empty lines would be enough.
+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); +} ... diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h index 2037467c94..4ba3988e3d 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -214,4 +214,33 @@ 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);
Each parameter on its own line, please. Jirka
s/virQEMUCapsInitQMPCommand/qemuProcess/ No functionality change. Use appropriate prefix in moved code. Signed-off-by: Chris Venteicher <cventeic@redhat.com> --- src/qemu/qemu_capabilities.c | 14 +++++++------- src/qemu/qemu_process.c | 28 ++++++++++++++-------------- src/qemu/qemu_process.h | 22 +++++++++++----------- 3 files changed, 32 insertions(+), 32 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 0f70fdf46d..f6d97648ce 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -4219,15 +4219,15 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, gid_t runGid, char **qmperr) { - virQEMUCapsInitQMPCommandPtr cmd = NULL; + qemuProcessPtr cmd = NULL; int ret = -1; int rc; - if (!(cmd = virQEMUCapsInitQMPCommandNew(qemuCaps->binary, libDir, - runUid, runGid, qmperr))) + if (!(proc = qemuProcessNew(qemuCaps->binary, libDir, + runUid, runGid, qmperr))) goto cleanup; - if ((rc = virQEMUCapsInitQMPCommandRun(cmd, false)) != 0) { + if ((rc = qemuProcessRun(cmd, false)) != 0) { if (rc == 1) ret = 0; goto cleanup; @@ -4237,8 +4237,8 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, goto cleanup; if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM)) { - virQEMUCapsInitQMPCommandAbort(cmd); - if ((rc = virQEMUCapsInitQMPCommandRun(cmd, true)) != 0) { + qemuProcessAbort(cmd); + if ((rc = qemuProcessRun(cmd, true)) != 0) { if (rc == 1) ret = 0; goto cleanup; @@ -4251,7 +4251,7 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, ret = 0; cleanup: - virQEMUCapsInitQMPCommandFree(cmd); + qemuProcessFree(cmd); return ret; } diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 0b3922fa39..dff0482856 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8081,12 +8081,12 @@ static qemuMonitorCallbacks callbacks = { void -virQEMUCapsInitQMPCommandFree(virQEMUCapsInitQMPCommandPtr cmd) +qemuProcessFree(qemuProcessPtr cmd) { if (!cmd) return; - virQEMUCapsInitQMPCommandAbort(cmd); + qemuProcessAbort(cmd); VIR_FREE(cmd->binary); VIR_FREE(cmd->monpath); VIR_FREE(cmd->monarg); @@ -8095,14 +8095,14 @@ virQEMUCapsInitQMPCommandFree(virQEMUCapsInitQMPCommandPtr cmd) } -virQEMUCapsInitQMPCommandPtr -virQEMUCapsInitQMPCommandNew(char *binary, - const char *libDir, - uid_t runUid, - gid_t runGid, - char **qmperr) +qemuProcessPtr +qemuProcessNew(char *binary, + const char *libDir, + uid_t runUid, + gid_t runGid, + char **qmperr) { - virQEMUCapsInitQMPCommandPtr cmd = NULL; + qemuProcessPtr cmd = NULL; if (VIR_ALLOC(cmd) < 0) goto error; @@ -8141,7 +8141,7 @@ virQEMUCapsInitQMPCommandNew(char *binary, return cmd; error: - virQEMUCapsInitQMPCommandFree(cmd); + qemuProcessFree(cmd); return NULL; } @@ -8151,8 +8151,8 @@ virQEMUCapsInitQMPCommandNew(char *binary, * 1 when probing QEMU failed */ int -virQEMUCapsInitQMPCommandRun(virQEMUCapsInitQMPCommandPtr cmd, - bool forceTCG) +qemuProcessRun(qemuProcessPtr cmd, + bool forceTCG) { virDomainXMLOptionPtr xmlopt = NULL; const char *machine; @@ -8222,7 +8222,7 @@ virQEMUCapsInitQMPCommandRun(virQEMUCapsInitQMPCommandPtr cmd, cleanup: if (!cmd->mon) - virQEMUCapsInitQMPCommandAbort(cmd); + qemuProcessAbort(cmd); virObjectUnref(xmlopt); return ret; @@ -8234,7 +8234,7 @@ virQEMUCapsInitQMPCommandRun(virQEMUCapsInitQMPCommandPtr cmd, void -virQEMUCapsInitQMPCommandAbort(virQEMUCapsInitQMPCommandPtr cmd) +qemuProcessAbort(qemuProcessPtr cmd) { if (cmd->mon) virObjectUnlock(cmd->mon); diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h index 4ba3988e3d..5417cb416f 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 _qemuProcess qemuProcess; +typedef qemuProcess *qemuProcessPtr; +struct _qemuProcess { char *binary; uid_t runUid; gid_t runGid; @@ -231,16 +231,16 @@ struct _virQEMUCapsInitQMPCommand { virDomainObjPtr vm; }; -virQEMUCapsInitQMPCommandPtr virQEMUCapsInitQMPCommandNew(char *binary, - const char *libDir, - uid_t runUid, - gid_t runGid, - char **qmperr); +qemuProcessPtr qemuProcessNew(char *binary, + const char *libDir, + uid_t runUid, + gid_t runGid, + char **qmperr); -void virQEMUCapsInitQMPCommandFree(virQEMUCapsInitQMPCommandPtr cmd); +void qemuProcessFree(qemuProcessPtr cmd); -int virQEMUCapsInitQMPCommandRun(virQEMUCapsInitQMPCommandPtr cmd, bool forceTCG); +int qemuProcessRun(qemuProcessPtr cmd, bool forceTCG); -void virQEMUCapsInitQMPCommandAbort(virQEMUCapsInitQMPCommandPtr cmd); +void qemuProcessAbort(qemuProcessPtr cmd); #endif /* __QEMU_PROCESS_H__ */ -- 2.17.1
On 11/11/2018 08:59 PM, Chris Venteicher wrote:
s/virQEMUCapsInitQMPCommand/qemuProcess/
No functionality change.
Use appropriate prefix in moved code.
Signed-off-by: Chris Venteicher <cventeic@redhat.com> --- src/qemu/qemu_capabilities.c | 14 +++++++------- src/qemu/qemu_process.c | 28 ++++++++++++++-------------- src/qemu/qemu_process.h | 22 +++++++++++----------- 3 files changed, 32 insertions(+), 32 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 0f70fdf46d..f6d97648ce 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -4219,15 +4219,15 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, gid_t runGid, char **qmperr) { - virQEMUCapsInitQMPCommandPtr cmd = NULL; + qemuProcessPtr cmd = NULL; int ret = -1; int rc;
- if (!(cmd = virQEMUCapsInitQMPCommandNew(qemuCaps->binary, libDir, - runUid, runGid, qmperr))) + if (!(proc = qemuProcessNew(qemuCaps->binary, libDir,
Ooops, this needs to stay @cmd. The idea is that after every single commit one has to be able to 'make all syntax-check check'.
+ runUid, runGid, qmperr))) goto cleanup;
Michal
On Sun, Nov 11, 2018 at 13:59:10 -0600, Chris Venteicher wrote:
s/virQEMUCapsInitQMPCommand/qemuProcess/
I think the new prefix should be qemuProcessQMP rather than qemuProcess. It's only ever used in the QMP code and while we may want to merge the code with the "normal" process code when possible and useful, we're not there yet. Thus the generic naming could be quite confusing. Otherwise this looks OK. Jirka
Prevent compile errors due to trying to use a const string as a non-const input to qemuProcessNew. No functionality change. Signed-off-by: Chris Venteicher <cventeic@redhat.com> --- src/qemu/qemu_process.c | 2 +- src/qemu/qemu_process.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index dff0482856..2f9726d463 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8096,7 +8096,7 @@ qemuProcessFree(qemuProcessPtr cmd) qemuProcessPtr -qemuProcessNew(char *binary, +qemuProcessNew(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 5417cb416f..39a2368ce5 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -231,7 +231,7 @@ struct _qemuProcess { virDomainObjPtr vm; }; -qemuProcessPtr qemuProcessNew(char *binary, +qemuProcessPtr qemuProcessNew(const char *binary, const char *libDir, uid_t runUid, gid_t runGid, -- 2.17.1
On Sun, Nov 11, 2018 at 13:59:11 -0600, Chris Venteicher wrote:
Prevent compile errors due to trying to use a const string as a non-const input to qemuProcessNew.
No functionality change.
OK, but it will obviously be affected by the change in the previous patch. Jirka
s/cmd/proc/ in process code imported from qemu_capabilities. No functionality is changed. Just variable renaming. Process code imported from qemu_capabilities was oriented around starting a process to issue a single QMP command. Future usecases (ex. baseline, compare) expect to use a single process to issue multiple different QMP commands. This patch changes the variable naming from cmd to proc to put focus on the process being maintained to issue commands. Signed-off-by: Chris Venteicher <cventeic@redhat.com> --- src/qemu/qemu_capabilities.c | 14 ++-- src/qemu/qemu_process.c | 140 +++++++++++++++++------------------ src/qemu/qemu_process.h | 6 +- 3 files changed, 80 insertions(+), 80 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index f6d97648ce..1ea63000e2 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -4219,7 +4219,7 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, gid_t runGid, char **qmperr) { - qemuProcessPtr cmd = NULL; + qemuProcessPtr proc = NULL; int ret = -1; int rc; @@ -4227,31 +4227,31 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, runUid, runGid, qmperr))) goto cleanup; - if ((rc = qemuProcessRun(cmd, false)) != 0) { + if ((rc = qemuProcessRun(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)) { - qemuProcessAbort(cmd); - if ((rc = qemuProcessRun(cmd, true)) != 0) { + qemuProcessAbort(proc); + if ((rc = qemuProcessRun(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: - qemuProcessFree(cmd); + qemuProcessFree(proc); return ret; } diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 2f9726d463..e949547124 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8081,17 +8081,17 @@ static qemuMonitorCallbacks callbacks = { void -qemuProcessFree(qemuProcessPtr cmd) +qemuProcessFree(qemuProcessPtr proc) { - if (!cmd) + if (!proc) return; - qemuProcessAbort(cmd); - VIR_FREE(cmd->binary); - VIR_FREE(cmd->monpath); - VIR_FREE(cmd->monarg); - VIR_FREE(cmd->pidfile); - VIR_FREE(cmd); + qemuProcessAbort(proc); + VIR_FREE(proc->binary); + VIR_FREE(proc->monpath); + VIR_FREE(proc->monarg); + VIR_FREE(proc->pidfile); + VIR_FREE(proc); } @@ -8102,25 +8102,25 @@ qemuProcessNew(const char *binary, gid_t runGid, char **qmperr) { - qemuProcessPtr cmd = NULL; + qemuProcessPtr 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 @@ -8129,19 +8129,19 @@ qemuProcessNew(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: - qemuProcessFree(cmd); + qemuProcessFree(proc); return NULL; } @@ -8151,7 +8151,7 @@ qemuProcessNew(const char *binary, * 1 when probing QEMU failed */ int -qemuProcessRun(qemuProcessPtr cmd, +qemuProcessRun(qemuProcessPtr proc, bool forceTCG) { virDomainXMLOptionPtr xmlopt = NULL; @@ -8165,7 +8165,7 @@ qemuProcessRun(qemuProcessPtr 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 @@ -8174,55 +8174,55 @@ qemuProcessRun(qemuProcessPtr cmd, * daemonize guarantees control won't return to libvirt * until the socket is present. */ - cmd->cmd = virCommandNewArgList(cmd->binary, - "-S", - "-no-user-config", - "-nodefaults", - "-nographic", - "-machine", machine, - "-qmp", cmd->monarg, - "-pidfile", cmd->pidfile, - "-daemonize", - NULL); - virCommandAddEnvPassCommon(cmd->cmd); - virCommandClearCaps(cmd->cmd); - virCommandSetGID(cmd->cmd, cmd->runGid); - virCommandSetUID(cmd->cmd, cmd->runUid); - - virCommandSetErrorBuffer(cmd->cmd, cmd->qmperr); + proc->cmd = virCommandNewArgList(proc->binary, + "-S", + "-no-user-config", + "-nodefaults", + "-nographic", + "-machine", machine, + "-qmp", proc->monarg, + "-pidfile", proc->pidfile, + "-daemonize", + NULL); + virCommandAddEnvPassCommon(proc->cmd); + virCommandClearCaps(proc->cmd); + virCommandSetGID(proc->cmd, proc->runGid); + virCommandSetUID(proc->cmd, proc->runUid); + + virCommandSetErrorBuffer(proc->cmd, proc->qmperr); /* Log, but otherwise ignore, non-zero status. */ - if (virCommandRun(cmd->cmd, &status) < 0) + if (virCommandRun(proc->cmd, &status) < 0) goto cleanup; if (status != 0) { VIR_DEBUG("QEMU %s exited with status %d: %s", - cmd->binary, status, *cmd->qmperr); + proc->binary, status, *proc->qmperr); goto ignore; } - if (virPidFileReadPath(cmd->pidfile, &cmd->pid) < 0) { - VIR_DEBUG("Failed to read pidfile %s", cmd->pidfile); + if (virPidFileReadPath(proc->pidfile, &proc->pid) < 0) { + VIR_DEBUG("Failed to read pidfile %s", proc->pidfile); goto ignore; } if (!(xmlopt = virDomainXMLOptionNew(NULL, NULL, NULL, NULL, NULL)) || - !(cmd->vm = virDomainObjNew(xmlopt))) + !(proc->vm = virDomainObjNew(xmlopt))) goto cleanup; - cmd->vm->pid = cmd->pid; + proc->vm->pid = proc->pid; - if (!(cmd->mon = qemuMonitorOpen(cmd->vm, &cmd->config, true, true, + if (!(proc->mon = qemuMonitorOpen(proc->vm, &proc->config, true, true, 0, &callbacks, NULL))) goto ignore; - virObjectLock(cmd->mon); + virObjectLock(proc->mon); ret = 0; cleanup: - if (!cmd->mon) - qemuProcessAbort(cmd); + if (!proc->mon) + qemuProcessAbort(proc); virObjectUnref(xmlopt); return ret; @@ -8234,34 +8234,34 @@ qemuProcessRun(qemuProcessPtr cmd, void -qemuProcessAbort(qemuProcessPtr cmd) +qemuProcessAbort(qemuProcessPtr 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 39a2368ce5..161311d007 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -237,10 +237,10 @@ qemuProcessPtr qemuProcessNew(const char *binary, gid_t runGid, char **qmperr); -void qemuProcessFree(qemuProcessPtr cmd); +void qemuProcessFree(qemuProcessPtr proc); -int qemuProcessRun(qemuProcessPtr cmd, bool forceTCG); +int qemuProcessRun(qemuProcessPtr proc, bool forceTCG); -void qemuProcessAbort(qemuProcessPtr cmd); +void qemuProcessAbort(qemuProcessPtr proc); #endif /* __QEMU_PROCESS_H__ */ -- 2.17.1
On 11/11/2018 08:59 PM, Chris Venteicher wrote:
s/cmd/proc/ in process code imported from qemu_capabilities.
No functionality is changed. Just variable renaming.
Process code imported from qemu_capabilities was oriented around starting a process to issue a single QMP command.
Future usecases (ex. baseline, compare) expect to use a single process to issue multiple different QMP commands.
This patch changes the variable naming from cmd to proc to put focus on the process being maintained to issue commands.
Signed-off-by: Chris Venteicher <cventeic@redhat.com> --- src/qemu/qemu_capabilities.c | 14 ++-- src/qemu/qemu_process.c | 140 +++++++++++++++++------------------ src/qemu/qemu_process.h | 6 +- 3 files changed, 80 insertions(+), 80 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index f6d97648ce..1ea63000e2 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -4219,7 +4219,7 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, gid_t runGid, char **qmperr) { - qemuProcessPtr cmd = NULL; + qemuProcessPtr proc = NULL; int ret = -1; int rc;
This is actually the place where the problem I've raised in 02/22 should be addressed. s/cmd/proc/ should happen here. Michal
On Sun, Nov 11, 2018 at 13:59:12 -0600, Chris Venteicher wrote:
s/cmd/proc/ in process code imported from qemu_capabilities.
No functionality is changed. Just variable renaming.
Process code imported from qemu_capabilities was oriented around starting a process to issue a single QMP command.
Future usecases (ex. baseline, compare) expect to use a single process to issue multiple different QMP commands.
This patch changes the variable naming from cmd to proc to put focus on the process being maintained to issue commands.
Signed-off-by: Chris Venteicher <cventeic@redhat.com> --- src/qemu/qemu_capabilities.c | 14 ++-- src/qemu/qemu_process.c | 140 +++++++++++++++++------------------ src/qemu/qemu_process.h | 6 +- 3 files changed, 80 insertions(+), 80 deletions(-)
OK Jirka
s/qemuProcessAbort/qemuProcessStopQmp/ applied to change function name used to stop QEMU processes in process code moved from qemu_capabilities. No functionality change. The new name, qemuProcessStopQmp, is consistent with the existing function qemuProcessStop used to stop Domain processes. Qmp is added to the end of qemuProcessStop to differentiate between the Domain and new non-domain version of the functions. qemuProcessStartQmp will be used in a future patch to mirror the qemuProcessStart function with a non-domain equivalent. Signed-off-by: Chris Venteicher <cventeic@redhat.com> --- src/qemu/qemu_capabilities.c | 2 +- src/qemu/qemu_process.c | 6 +++--- src/qemu/qemu_process.h | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 1ea63000e2..73ec8e5c6e 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -4237,7 +4237,7 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, goto cleanup; if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM)) { - qemuProcessAbort(proc); + qemuProcessStopQmp(proc); if ((rc = qemuProcessRun(proc, true)) != 0) { if (rc == 1) ret = 0; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index e949547124..2571024e8e 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8086,7 +8086,7 @@ qemuProcessFree(qemuProcessPtr proc) if (!proc) return; - qemuProcessAbort(proc); + qemuProcessStopQmp(proc); VIR_FREE(proc->binary); VIR_FREE(proc->monpath); VIR_FREE(proc->monarg); @@ -8222,7 +8222,7 @@ qemuProcessRun(qemuProcessPtr proc, cleanup: if (!proc->mon) - qemuProcessAbort(proc); + qemuProcessStopQmp(proc); virObjectUnref(xmlopt); return ret; @@ -8234,7 +8234,7 @@ qemuProcessRun(qemuProcessPtr proc, void -qemuProcessAbort(qemuProcessPtr proc) +qemuProcessStopQmp(qemuProcessPtr proc) { if (proc->mon) virObjectUnlock(proc->mon); diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h index 161311d007..25343c4592 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -241,6 +241,6 @@ void qemuProcessFree(qemuProcessPtr proc); int qemuProcessRun(qemuProcessPtr proc, bool forceTCG); -void qemuProcessAbort(qemuProcessPtr proc); +void qemuProcessStopQmp(qemuProcessPtr proc); #endif /* __QEMU_PROCESS_H__ */ -- 2.17.1
On Sun, Nov 11, 2018 at 13:59:13 -0600, Chris Venteicher wrote:
s/qemuProcessAbort/qemuProcessStopQmp/ applied to change function name used to stop QEMU processes in process code moved from qemu_capabilities.
Since the prefix will be qemuProcessQMP rather then qemuProcess, the qemuProcessAbort will actually be renamed as qemuProcessQMPStop. And qemuProcessRun (qemuProcessQMPRun with the new prefix) should be called qemuProcessQMPStart for consistency (see also my reply to patch 10/22 where a similarly named function would be introduced). Jirka
Follow the convention established in qemu_process of 1) alloc process structure 2) start process 3) use process 4) stop process 5) free process data structure The process data structure persists after the process activation fails or the process dies or is killed so stderr strings can be retrieved until the process data structure is freed. Signed-off-by: Chris Venteicher <cventeic@redhat.com> --- src/qemu/qemu_capabilities.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 73ec8e5c6e..082874082b 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -4251,6 +4251,7 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, ret = 0; cleanup: + qemuProcessStopQmp(proc); qemuProcessFree(proc); return ret; } -- 2.17.1
On Sun, Nov 11, 2018 at 13:59:14 -0600, Chris Venteicher wrote:
Follow the convention established in qemu_process of 1) alloc process structure 2) start process 3) use process 4) stop process 5) free process data structure
The process data structure persists after the process activation fails or the process dies or is killed so stderr strings can be retrieved until the process data structure is freed.
Signed-off-by: Chris Venteicher <cventeic@redhat.com> --- src/qemu/qemu_capabilities.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 73ec8e5c6e..082874082b 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -4251,6 +4251,7 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, ret = 0;
cleanup: + qemuProcessStopQmp(proc);
Doing this here would just crash the daemon if proc == NULL. Also qemuProcessFree will call the same function again, which will cause a lot of issues. For example, proc->vm will be unlocked and unreferenced twice. That said, you need to squash some code from the following patches to this one to make sure it doesn't cause any functional change.
qemuProcessFree(proc); return ret;
Jirka
In new process code, move from model where qemuProcess struct can be used to activate a series of Qemu processes to model where one qemuProcess struct is used for one and only one Qemu process. The forceTCG parameter (use / don't use KVM) will be passed when the qemuProcess struct is initialized since the qemuProcess struct won't be reused. Signed-off-by: Chris Venteicher <cventeic@redhat.com> --- src/qemu/qemu_capabilities.c | 16 ++++++++++++---- src/qemu/qemu_process.c | 11 +++++++---- src/qemu/qemu_process.h | 6 ++++-- 3 files changed, 23 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 082874082b..a957c3bdbd 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -4220,14 +4220,16 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, char **qmperr) { qemuProcessPtr proc = NULL; + qemuProcessPtr proc_kvm = NULL; int ret = -1; int rc; + bool forceTCG = false; if (!(proc = qemuProcessNew(qemuCaps->binary, libDir, - runUid, runGid, qmperr))) + runUid, runGid, qmperr, forceTCG))) goto cleanup; - if ((rc = qemuProcessRun(proc, false)) != 0) { + if ((rc = qemuProcessRun(proc)) != 0) { if (rc == 1) ret = 0; goto cleanup; @@ -4238,13 +4240,17 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM)) { qemuProcessStopQmp(proc); - if ((rc = qemuProcessRun(proc, true)) != 0) { + + forceTCG = true; + proc_kvm = qemuProcessNew(qemuCaps->binary, libDir, runUid, runGid, NULL, forceTCG); + + if ((rc = qemuProcessRun(proc_kvm)) != 0) { if (rc == 1) ret = 0; goto cleanup; } - if (virQEMUCapsInitQMPMonitorTCG(qemuCaps, proc->mon) < 0) + if (virQEMUCapsInitQMPMonitorTCG(qemuCaps, proc_kvm->mon) < 0) goto cleanup; } @@ -4252,7 +4258,9 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, cleanup: qemuProcessStopQmp(proc); + qemuProcessStopQmp(proc_kvm); qemuProcessFree(proc); + qemuProcessFree(proc_kvm); return ret; } diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 2571024e8e..dda74d5b7a 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8100,7 +8100,8 @@ qemuProcessNew(const char *binary, const char *libDir, uid_t runUid, gid_t runGid, - char **qmperr) + char **qmperr, + bool forceTCG) { qemuProcessPtr proc = NULL; @@ -8110,10 +8111,13 @@ qemuProcessNew(const char *binary, if (VIR_STRDUP(proc->binary, binary) < 0) goto error; + proc->forceTCG = forceTCG; + 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" */ @@ -8151,15 +8155,14 @@ qemuProcessNew(const char *binary, * 1 when probing QEMU failed */ int -qemuProcessRun(qemuProcessPtr proc, - bool forceTCG) +qemuProcessRun(qemuProcessPtr 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 25343c4592..ab2640ce7c 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -229,17 +229,19 @@ struct _qemuProcess { virDomainChrSourceDef config; pid_t pid; virDomainObjPtr vm; + bool forceTCG; }; qemuProcessPtr qemuProcessNew(const char *binary, const char *libDir, uid_t runUid, gid_t runGid, - char **qmperr); + char **qmperr, + bool forceTCG); void qemuProcessFree(qemuProcessPtr proc); -int qemuProcessRun(qemuProcessPtr proc, bool forceTCG); +int qemuProcessRun(qemuProcessPtr proc); void qemuProcessStopQmp(qemuProcessPtr proc); -- 2.17.1
On 11/11/2018 08:59 PM, Chris Venteicher wrote:
In new process code, move from model where qemuProcess struct can be used to activate a series of Qemu processes to model where one qemuProcess struct is used for one and only one Qemu process.
The forceTCG parameter (use / don't use KVM) will be passed when the qemuProcess struct is initialized since the qemuProcess struct won't be reused.
Signed-off-by: Chris Venteicher <cventeic@redhat.com> --- src/qemu/qemu_capabilities.c | 16 ++++++++++++---- src/qemu/qemu_process.c | 11 +++++++---- src/qemu/qemu_process.h | 6 ++++-- 3 files changed, 23 insertions(+), 10 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 082874082b..a957c3bdbd 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -4220,14 +4220,16 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, char **qmperr) { qemuProcessPtr proc = NULL; + qemuProcessPtr proc_kvm = NULL; int ret = -1; int rc; + bool forceTCG = false;
if (!(proc = qemuProcessNew(qemuCaps->binary, libDir, - runUid, runGid, qmperr))) + runUid, runGid, qmperr, forceTCG))) goto cleanup;
- if ((rc = qemuProcessRun(proc, false)) != 0) { + if ((rc = qemuProcessRun(proc)) != 0) { if (rc == 1) ret = 0; goto cleanup; @@ -4238,13 +4240,17 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps,
if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM)) { qemuProcessStopQmp(proc); - if ((rc = qemuProcessRun(proc, true)) != 0) { + + forceTCG = true; + proc_kvm = qemuProcessNew(qemuCaps->binary, libDir, runUid, runGid, NULL, forceTCG);
This is a very long line. There's this limit of 80 characters per line (although it's a soft limit).
+ + if ((rc = qemuProcessRun(proc_kvm)) != 0) { if (rc == 1) ret = 0; goto cleanup; }
- if (virQEMUCapsInitQMPMonitorTCG(qemuCaps, proc->mon) < 0) + if (virQEMUCapsInitQMPMonitorTCG(qemuCaps, proc_kvm->mon) < 0) goto cleanup; }
Michal
On Sun, Nov 11, 2018 at 13:59:15 -0600, Chris Venteicher wrote:
In new process code, move from model where qemuProcess struct can be used to activate a series of Qemu processes to model where one qemuProcess struct is used for one and only one Qemu process.
The forceTCG parameter (use / don't use KVM) will be passed when the qemuProcess struct is initialized since the qemuProcess struct won't be reused.
Signed-off-by: Chris Venteicher <cventeic@redhat.com> --- src/qemu/qemu_capabilities.c | 16 ++++++++++++---- src/qemu/qemu_process.c | 11 +++++++---- src/qemu/qemu_process.h | 6 ++++-- 3 files changed, 23 insertions(+), 10 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 082874082b..a957c3bdbd 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -4220,14 +4220,16 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, char **qmperr) { qemuProcessPtr proc = NULL; + qemuProcessPtr proc_kvm = NULL;
s/proc_kvm/procTCG/ The second QEMU process probes for TCG capabilities in case the first reported KVM as enabled (otherwise the first one already reported TCG capabilities).
int ret = -1; int rc; + bool forceTCG = false;
This variable does not make a lot of sense. You can just use false/true directly when calling qemuProcessNew.
if (!(proc = qemuProcessNew(qemuCaps->binary, libDir, - runUid, runGid, qmperr))) + runUid, runGid, qmperr, forceTCG))) goto cleanup;
- if ((rc = qemuProcessRun(proc, false)) != 0) { + if ((rc = qemuProcessRun(proc)) != 0) { if (rc == 1) ret = 0; goto cleanup; @@ -4238,13 +4240,17 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps,
if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM)) { qemuProcessStopQmp(proc); - if ((rc = qemuProcessRun(proc, true)) != 0) { + + forceTCG = true; + proc_kvm = qemuProcessNew(qemuCaps->binary, libDir, runUid, runGid, NULL, forceTCG); + + if ((rc = qemuProcessRun(proc_kvm)) != 0) { if (rc == 1) ret = 0; goto cleanup; }
- if (virQEMUCapsInitQMPMonitorTCG(qemuCaps, proc->mon) < 0) + if (virQEMUCapsInitQMPMonitorTCG(qemuCaps, proc_kvm->mon) < 0) goto cleanup; }
@@ -4252,7 +4258,9 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps,
cleanup: qemuProcessStopQmp(proc); + qemuProcessStopQmp(proc_kvm); qemuProcessFree(proc); + qemuProcessFree(proc_kvm); return ret; }
After this patch virQEMUCapsInitQMP contains two almost identical parts. It should be further refactored (in a follow up patch) to something like (I was too lazy to come up with a good name for the function) virQEMUCapsInitQMP() { if (virQEMUCapsInitQMP...(..., false, err) < 0) return -1; if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM) && virQEMUCapsInitQMP...(..., true, NULL) < 0) return -1; return 0; }
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 2571024e8e..dda74d5b7a 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8100,7 +8100,8 @@ qemuProcessNew(const char *binary, const char *libDir, uid_t runUid, gid_t runGid, - char **qmperr) + char **qmperr, + bool forceTCG) { qemuProcessPtr proc = NULL;
@@ -8110,10 +8111,13 @@ qemuProcessNew(const char *binary, if (VIR_STRDUP(proc->binary, binary) < 0) goto error;
+ proc->forceTCG = forceTCG;
I'd put this just after the "proc->qmperr = qmperr;" line with no empty line separator to keep the order consistent with the order of function parameters. But it's not a big deal.
+ proc->runUid = runUid; proc->runGid = runGid; proc->qmperr = qmperr;
+
Please, delete the extra empty line.
/* the ".sock" sufix is important to avoid a possible clash with a qemu * domain called "capabilities" */
Jirka
A qemuProcess struct tracks the entire lifespan of a single QEMU Process including storing error output when the process terminates or activation fails. Error output remains available until qemuProcessFree is called. The qmperr buffer no longer needs to be maintained outside of the qemuProcess structure because the structure is used for a single QEMU process and the structures can be maintained as long as required to retrieve the process error info. Capabilities init code is refactored but continues to report QEMU process error data only when the initial (non KVM) QEMU process activation fails to result in a usable monitor connection for retrieving capabilities. The VIR_ERR_INTERNAL_ERROR "Failed to probe QEMU binary" reporting is moved into virQEMUCapsInitQMP function and the stderr string is extracted from the qemuProcess struct using a macro to retrieve the string. The same error and log message should be generated, in the same conditions, after this patch as before. Signed-off-by: Chris Venteicher <cventeic@redhat.com> --- src/qemu/qemu_capabilities.c | 27 ++++++++++++--------------- src/qemu/qemu_process.c | 12 ++++-------- src/qemu/qemu_process.h | 6 ++++-- 3 files changed, 20 insertions(+), 25 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index a957c3bdbd..f5e327097e 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -4216,8 +4216,7 @@ static int virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, const char *libDir, uid_t runUid, - gid_t runGid, - char **qmperr) + gid_t runGid) { qemuProcessPtr proc = NULL; qemuProcessPtr proc_kvm = NULL; @@ -4226,7 +4225,7 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, bool forceTCG = false; if (!(proc = qemuProcessNew(qemuCaps->binary, libDir, - runUid, runGid, qmperr, forceTCG))) + runUid, runGid, forceTCG))) goto cleanup; if ((rc = qemuProcessRun(proc)) != 0) { @@ -4242,7 +4241,7 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, qemuProcessStopQmp(proc); forceTCG = true; - proc_kvm = qemuProcessNew(qemuCaps->binary, libDir, runUid, runGid, NULL, forceTCG); + proc_kvm = qemuProcessNew(qemuCaps->binary, libDir, runUid, runGid, forceTCG); if ((rc = qemuProcessRun(proc_kvm)) != 0) { if (rc == 1) @@ -4257,6 +4256,13 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, ret = 0; cleanup: + if (proc && !proc->mon) { + char *err = QEMU_PROCESS_ERROR(proc) ? QEMU_PROCESS_ERROR(proc) : _("unknown error"); + + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to probe QEMU binary with QMP: %s"), err); + } + qemuProcessStopQmp(proc); qemuProcessStopQmp(proc_kvm); qemuProcessFree(proc); @@ -4296,7 +4302,6 @@ virQEMUCapsNewForBinaryInternal(virArch hostArch, { virQEMUCapsPtr qemuCaps; struct stat sb; - char *qmperr = NULL; if (!(qemuCaps = virQEMUCapsNew())) goto error; @@ -4323,15 +4328,8 @@ virQEMUCapsNewForBinaryInternal(virArch hostArch, goto error; } - if (virQEMUCapsInitQMP(qemuCaps, libDir, runUid, runGid, &qmperr) < 0) { - virQEMUCapsLogProbeFailure(binary); - goto error; - } - - if (!qemuCaps->usedQMP) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to probe QEMU binary with QMP: %s"), - qmperr ? qmperr : _("unknown error")); + if (virQEMUCapsInitQMP(qemuCaps, libDir, runUid, runGid) < 0 || + !qemuCaps->usedQMP) { virQEMUCapsLogProbeFailure(binary); goto error; } @@ -4350,7 +4348,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 dda74d5b7a..a741d1cf91 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8091,6 +8091,7 @@ qemuProcessFree(qemuProcessPtr proc) VIR_FREE(proc->monpath); VIR_FREE(proc->monarg); VIR_FREE(proc->pidfile); + VIR_FREE(proc->qmperr); VIR_FREE(proc); } @@ -8100,7 +8101,6 @@ qemuProcessNew(const char *binary, const char *libDir, uid_t runUid, gid_t runGid, - char **qmperr, bool forceTCG) { qemuProcessPtr proc = NULL; @@ -8115,8 +8115,6 @@ qemuProcessNew(const char *binary, 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" @@ -8155,8 +8153,7 @@ qemuProcessNew(const char *binary, * 1 when probing QEMU failed */ int -qemuProcessRun(qemuProcessPtr proc) -{ +qemuProcessRun(qemuProcessPtr proc){ virDomainXMLOptionPtr xmlopt = NULL; const char *machine; int status = 0; @@ -8192,7 +8189,7 @@ qemuProcessRun(qemuProcessPtr proc) virCommandSetGID(proc->cmd, proc->runGid); virCommandSetUID(proc->cmd, proc->runUid); - virCommandSetErrorBuffer(proc->cmd, proc->qmperr); + virCommandSetErrorBuffer(proc->cmd, &(proc->qmperr)); /* Log, but otherwise ignore, non-zero status. */ if (virCommandRun(proc->cmd, &status) < 0) @@ -8200,7 +8197,7 @@ qemuProcessRun(qemuProcessPtr proc) if (status != 0) { VIR_DEBUG("QEMU %s exited with status %d: %s", - proc->binary, status, *proc->qmperr); + proc->binary, status, proc->qmperr); goto ignore; } @@ -8262,7 +8259,6 @@ qemuProcessStopQmp(qemuProcessPtr proc) (long long)proc->pid, virStrerror(errno, ebuf, sizeof(ebuf))); - VIR_FREE(*proc->qmperr); } if (proc->pidfile) unlink(proc->pidfile); diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h index ab2640ce7c..abd80baf5c 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -220,7 +220,7 @@ struct _qemuProcess { char *binary; uid_t runUid; gid_t runGid; - char **qmperr; + char *qmperr; /* qemu process stderr */ char *monarg; char *monpath; char *pidfile; @@ -232,11 +232,13 @@ struct _qemuProcess { bool forceTCG; }; +#define QEMU_PROCESS_ERROR(proc) \ + ((char *) (proc ? proc->qmperr: NULL)) + qemuProcessPtr qemuProcessNew(const char *binary, const char *libDir, uid_t runUid, gid_t runGid, - char **qmperr, bool forceTCG); void qemuProcessFree(qemuProcessPtr proc); -- 2.17.1
On 11/11/2018 08:59 PM, Chris Venteicher wrote:
A qemuProcess struct tracks the entire lifespan of a single QEMU Process including storing error output when the process terminates or activation fails.
Error output remains available until qemuProcessFree is called.
The qmperr buffer no longer needs to be maintained outside of the qemuProcess structure because the structure is used for a single QEMU process and the structures can be maintained as long as required to retrieve the process error info.
Capabilities init code is refactored but continues to report QEMU process error data only when the initial (non KVM) QEMU process activation fails to result in a usable monitor connection for retrieving capabilities.
The VIR_ERR_INTERNAL_ERROR "Failed to probe QEMU binary" reporting is moved into virQEMUCapsInitQMP function and the stderr string is extracted from the qemuProcess struct using a macro to retrieve the string.
The same error and log message should be generated, in the same conditions, after this patch as before.
Signed-off-by: Chris Venteicher <cventeic@redhat.com> --- src/qemu/qemu_capabilities.c | 27 ++++++++++++--------------- src/qemu/qemu_process.c | 12 ++++-------- src/qemu/qemu_process.h | 6 ++++-- 3 files changed, 20 insertions(+), 25 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index a957c3bdbd..f5e327097e 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -4216,8 +4216,7 @@ static int virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, const char *libDir, uid_t runUid, - gid_t runGid, - char **qmperr) + gid_t runGid) { qemuProcessPtr proc = NULL; qemuProcessPtr proc_kvm = NULL; @@ -4226,7 +4225,7 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, bool forceTCG = false;
if (!(proc = qemuProcessNew(qemuCaps->binary, libDir, - runUid, runGid, qmperr, forceTCG))) + runUid, runGid, forceTCG))) goto cleanup;
if ((rc = qemuProcessRun(proc)) != 0) { @@ -4242,7 +4241,7 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, qemuProcessStopQmp(proc);
forceTCG = true; - proc_kvm = qemuProcessNew(qemuCaps->binary, libDir, runUid, runGid, NULL, forceTCG); + proc_kvm = qemuProcessNew(qemuCaps->binary, libDir, runUid, runGid, forceTCG);
if ((rc = qemuProcessRun(proc_kvm)) != 0) { if (rc == 1) @@ -4257,6 +4256,13 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, ret = 0;
cleanup: + if (proc && !proc->mon) { + char *err = QEMU_PROCESS_ERROR(proc) ? QEMU_PROCESS_ERROR(proc) : _("unknown error");
or just: char *err = proc->qmperr; if (!err) err = _("uknown error");
+ + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to probe QEMU binary with QMP: %s"), err); + } + qemuProcessStopQmp(proc); qemuProcessStopQmp(proc_kvm); qemuProcessFree(proc); @@ -4296,7 +4302,6 @@ virQEMUCapsNewForBinaryInternal(virArch hostArch, { virQEMUCapsPtr qemuCaps; struct stat sb; - char *qmperr = NULL;
if (!(qemuCaps = virQEMUCapsNew())) goto error; @@ -4323,15 +4328,8 @@ virQEMUCapsNewForBinaryInternal(virArch hostArch, goto error; }
- if (virQEMUCapsInitQMP(qemuCaps, libDir, runUid, runGid, &qmperr) < 0) { - virQEMUCapsLogProbeFailure(binary); - goto error; - } - - if (!qemuCaps->usedQMP) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to probe QEMU binary with QMP: %s"), - qmperr ? qmperr : _("unknown error")); + if (virQEMUCapsInitQMP(qemuCaps, libDir, runUid, runGid) < 0 || + !qemuCaps->usedQMP) { virQEMUCapsLogProbeFailure(binary);
Oh, this won't fly. So virReportError() sets the error object and virQEMUCapsLogProbeFailure() uses it (by calling virGetLastErrorMessage()). But since you're removing the virReportError() call then there's no error object to get the error message from. IOW this will probably log: "Failed to probe capabilities for /usr/bin/qemu: no error" even though later the actual qemu error message is logged. Up until now, patches 01-07 look reasonable.
goto error; } @@ -4350,7 +4348,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 dda74d5b7a..a741d1cf91 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8091,6 +8091,7 @@ qemuProcessFree(qemuProcessPtr proc) VIR_FREE(proc->monpath); VIR_FREE(proc->monarg); VIR_FREE(proc->pidfile); + VIR_FREE(proc->qmperr); VIR_FREE(proc); }
@@ -8100,7 +8101,6 @@ qemuProcessNew(const char *binary, const char *libDir, uid_t runUid, gid_t runGid, - char **qmperr, bool forceTCG) { qemuProcessPtr proc = NULL; @@ -8115,8 +8115,6 @@ qemuProcessNew(const char *binary,
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" @@ -8155,8 +8153,7 @@ qemuProcessNew(const char *binary, * 1 when probing QEMU failed */ int -qemuProcessRun(qemuProcessPtr proc) -{ +qemuProcessRun(qemuProcessPtr proc){
Ooops, this is not right ;-)
virDomainXMLOptionPtr xmlopt = NULL; const char *machine; int status = 0; @@ -8192,7 +8189,7 @@ qemuProcessRun(qemuProcessPtr proc) virCommandSetGID(proc->cmd, proc->runGid); virCommandSetUID(proc->cmd, proc->runUid);
- virCommandSetErrorBuffer(proc->cmd, proc->qmperr); + virCommandSetErrorBuffer(proc->cmd, &(proc->qmperr));
/* Log, but otherwise ignore, non-zero status. */ if (virCommandRun(proc->cmd, &status) < 0) @@ -8200,7 +8197,7 @@ qemuProcessRun(qemuProcessPtr proc)
if (status != 0) { VIR_DEBUG("QEMU %s exited with status %d: %s", - proc->binary, status, *proc->qmperr); + proc->binary, status, proc->qmperr); goto ignore; }
@@ -8262,7 +8259,6 @@ qemuProcessStopQmp(qemuProcessPtr proc) (long long)proc->pid, virStrerror(errno, ebuf, sizeof(ebuf)));
- VIR_FREE(*proc->qmperr); } if (proc->pidfile) unlink(proc->pidfile); diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h index ab2640ce7c..abd80baf5c 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -220,7 +220,7 @@ struct _qemuProcess { char *binary; uid_t runUid; gid_t runGid; - char **qmperr; + char *qmperr; /* qemu process stderr */ char *monarg; char *monpath; char *pidfile; @@ -232,11 +232,13 @@ struct _qemuProcess { bool forceTCG; };
+#define QEMU_PROCESS_ERROR(proc) \ + ((char *) (proc ? proc->qmperr: NULL)) +
I don't think we need this macro. BTW, if you install 'cppi' you'll notice that there needs to be a space afer # and before define.
qemuProcessPtr qemuProcessNew(const char *binary, const char *libDir, uid_t runUid, gid_t runGid, - char **qmperr, bool forceTCG);
void qemuProcessFree(qemuProcessPtr proc);
Michal P.S. this is where I stop my review for today. Have some errands to run, will resume review tomorrow.
On Mon, Nov 12, 2018 at 16:53:34 +0100, Michal Privoznik wrote:
On 11/11/2018 08:59 PM, Chris Venteicher wrote:
A qemuProcess struct tracks the entire lifespan of a single QEMU Process including storing error output when the process terminates or activation fails.
Error output remains available until qemuProcessFree is called.
The qmperr buffer no longer needs to be maintained outside of the qemuProcess structure because the structure is used for a single QEMU process and the structures can be maintained as long as required to retrieve the process error info.
Capabilities init code is refactored but continues to report QEMU process error data only when the initial (non KVM) QEMU process activation fails to result in a usable monitor connection for retrieving capabilities.
The VIR_ERR_INTERNAL_ERROR "Failed to probe QEMU binary" reporting is moved into virQEMUCapsInitQMP function and the stderr string is extracted from the qemuProcess struct using a macro to retrieve the string.
The same error and log message should be generated, in the same conditions, after this patch as before.
Signed-off-by: Chris Venteicher <cventeic@redhat.com> --- src/qemu/qemu_capabilities.c | 27 ++++++++++++--------------- src/qemu/qemu_process.c | 12 ++++-------- src/qemu/qemu_process.h | 6 ++++-- 3 files changed, 20 insertions(+), 25 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index a957c3bdbd..f5e327097e 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c ... @@ -4323,15 +4328,8 @@ virQEMUCapsNewForBinaryInternal(virArch hostArch, goto error; }
- if (virQEMUCapsInitQMP(qemuCaps, libDir, runUid, runGid, &qmperr) < 0) { - virQEMUCapsLogProbeFailure(binary); - goto error; - } - - if (!qemuCaps->usedQMP) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to probe QEMU binary with QMP: %s"), - qmperr ? qmperr : _("unknown error")); + if (virQEMUCapsInitQMP(qemuCaps, libDir, runUid, runGid) < 0 || + !qemuCaps->usedQMP) { virQEMUCapsLogProbeFailure(binary);
Oh, this won't fly. So virReportError() sets the error object and virQEMUCapsLogProbeFailure() uses it (by calling virGetLastErrorMessage()). But since you're removing the virReportError() call then there's no error object to get the error message from. IOW this will probably log: "Failed to probe capabilities for /usr/bin/qemu: no error" even though later the actual qemu error message is logged.
Not really. The virReportError is still there, just moved inside virQEMUCapsInitQMP. No issue to see here I believe. Jirka
On Sun, Nov 11, 2018 at 13:59:16 -0600, Chris Venteicher wrote:
A qemuProcess struct tracks the entire lifespan of a single QEMU Process including storing error output when the process terminates or activation fails.
Error output remains available until qemuProcessFree is called.
The qmperr buffer no longer needs to be maintained outside of the qemuProcess structure because the structure is used for a single QEMU process and the structures can be maintained as long as required to retrieve the process error info.
Capabilities init code is refactored but continues to report QEMU process error data only when the initial (non KVM) QEMU process activation
As explained for the previous patch the initial QEMU process actually checks KVM if available.
fails to result in a usable monitor connection for retrieving capabilities.
The VIR_ERR_INTERNAL_ERROR "Failed to probe QEMU binary" reporting is moved into virQEMUCapsInitQMP function and the stderr string is extracted from the qemuProcess struct using a macro to retrieve the string.
The same error and log message should be generated, in the same conditions, after this patch as before.
Signed-off-by: Chris Venteicher <cventeic@redhat.com> --- src/qemu/qemu_capabilities.c | 27 ++++++++++++--------------- src/qemu/qemu_process.c | 12 ++++-------- src/qemu/qemu_process.h | 6 ++++-- 3 files changed, 20 insertions(+), 25 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index a957c3bdbd..f5e327097e 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -4216,8 +4216,7 @@ static int virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, const char *libDir, uid_t runUid, - gid_t runGid, - char **qmperr) + gid_t runGid)
Yes, hiding qmperror inside virQEMUCapsInitQMP (actually inside the new function which will be introduced once virQEMUCapsInitQMP is refactored as I suggested in my review for the previous patch) is definitely a good idea.
{ qemuProcessPtr proc = NULL; qemuProcessPtr proc_kvm = NULL; @@ -4226,7 +4225,7 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, bool forceTCG = false;
if (!(proc = qemuProcessNew(qemuCaps->binary, libDir, - runUid, runGid, qmperr, forceTCG))) + runUid, runGid, forceTCG))) goto cleanup;
The new function can just return directly from here...
if ((rc = qemuProcessRun(proc)) != 0) { @@ -4242,7 +4241,7 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, qemuProcessStopQmp(proc);
forceTCG = true; - proc_kvm = qemuProcessNew(qemuCaps->binary, libDir, runUid, runGid, NULL, forceTCG); + proc_kvm = qemuProcessNew(qemuCaps->binary, libDir, runUid, runGid, forceTCG);
if ((rc = qemuProcessRun(proc_kvm)) != 0) { if (rc == 1) @@ -4257,6 +4256,13 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, ret = 0;
cleanup: + if (proc && !proc->mon) {
... which means you wouldn't need to check for proc at this point.
+ char *err = QEMU_PROCESS_ERROR(proc) ? QEMU_PROCESS_ERROR(proc) : _("unknown error");
But even now you don't need to check for proc inside the macro. Which makes the main (though still very tiny) reason for introducing the macro. In other words, just drop the macro and use proc->qmperr directly.
+ + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to probe QEMU binary with QMP: %s"), err); + }
And this would report an error even if ret == 0.
+ qemuProcessStopQmp(proc); qemuProcessStopQmp(proc_kvm); qemuProcessFree(proc); @@ -4296,7 +4302,6 @@ virQEMUCapsNewForBinaryInternal(virArch hostArch, { virQEMUCapsPtr qemuCaps; struct stat sb; - char *qmperr = NULL;
if (!(qemuCaps = virQEMUCapsNew())) goto error; @@ -4323,15 +4328,8 @@ virQEMUCapsNewForBinaryInternal(virArch hostArch, goto error; }
- if (virQEMUCapsInitQMP(qemuCaps, libDir, runUid, runGid, &qmperr) < 0) { - virQEMUCapsLogProbeFailure(binary); - goto error; - } - - if (!qemuCaps->usedQMP) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to probe QEMU binary with QMP: %s"), - qmperr ? qmperr : _("unknown error")); + if (virQEMUCapsInitQMP(qemuCaps, libDir, runUid, runGid) < 0 || + !qemuCaps->usedQMP) {
There should be no need to keep the !qemuCaps->usedQMP check after this refactoring. See my notes to the next patch for more details.
virQEMUCapsLogProbeFailure(binary); goto error; } @@ -4350,7 +4348,6 @@ virQEMUCapsNewForBinaryInternal(virArch hostArch, }
cleanup: - VIR_FREE(qmperr); return qemuCaps;
error:
...
diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h index ab2640ce7c..abd80baf5c 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -220,7 +220,7 @@ struct _qemuProcess { char *binary; uid_t runUid; gid_t runGid; - char **qmperr; + char *qmperr; /* qemu process stderr */
You could even make the vairable name self-explanatory: char *stderr;
char *monarg; char *monpath; char *pidfile; @@ -232,11 +232,13 @@ struct _qemuProcess { bool forceTCG; };
+#define QEMU_PROCESS_ERROR(proc) \ + ((char *) (proc ? proc->qmperr: NULL)) +
Just drop this.
qemuProcessPtr qemuProcessNew(const char *binary, const char *libDir, uid_t runUid, gid_t runGid, - char **qmperr, bool forceTCG);
void qemuProcessFree(qemuProcessPtr proc);
Jirka
Failure to connect to QEMU to probe capabilities is not considered a error case and does not result in a negative return value. Check for a NULL monitor connection pointer before trying to send capabilities commands to QEMU rather than depending on a special positive return value. This simplifies logic and is more consistent with the operation of existing qemu_process functions. A macro is introduced to easily obtain the monitor pointer from the qemuProcess structure. Signed-off-by: Chris Venteicher <cventeic@redhat.com> --- src/qemu/qemu_capabilities.c | 28 ++++++++++++++++++---------- src/qemu/qemu_process.c | 9 +-------- src/qemu/qemu_process.h | 3 +++ 3 files changed, 22 insertions(+), 18 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index f5e327097e..fbb4336201 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -4220,43 +4220,51 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, { qemuProcessPtr proc = NULL; qemuProcessPtr proc_kvm = NULL; + qemuMonitorPtr mon = NULL; + qemuMonitorPtr mon_kvm = NULL; int ret = -1; - int rc; bool forceTCG = false; if (!(proc = qemuProcessNew(qemuCaps->binary, libDir, runUid, runGid, forceTCG))) goto cleanup; - if ((rc = qemuProcessRun(proc)) != 0) { - if (rc == 1) - ret = 0; + + if (qemuProcessRun(proc) < 0) + goto cleanup; + + if (!(mon = QEMU_PROCESS_MONITOR(proc))) { + ret = 0; /* Failure probing QEMU not considered error case */ goto cleanup; } - if (virQEMUCapsInitQMPMonitor(qemuCaps, proc->mon) < 0) + /* Pull capabilities from QEMU */ + if (virQEMUCapsInitQMPMonitor(qemuCaps, mon) < 0) goto cleanup; + /* Pull capabilities again if KVM supported */ if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM)) { qemuProcessStopQmp(proc); forceTCG = true; proc_kvm = qemuProcessNew(qemuCaps->binary, libDir, runUid, runGid, forceTCG); - if ((rc = qemuProcessRun(proc_kvm)) != 0) { - if (rc == 1) - ret = 0; + if (qemuProcessRun(proc_kvm) < 0) + goto cleanup; + + if (!(mon_kvm = QEMU_PROCESS_MONITOR(proc_kvm))) { + ret = 0; /* Failure probing QEMU not considered error case */ goto cleanup; } - if (virQEMUCapsInitQMPMonitorTCG(qemuCaps, proc_kvm->mon) < 0) + if (virQEMUCapsInitQMPMonitorTCG(qemuCaps, mon_kvm) < 0) goto cleanup; } ret = 0; cleanup: - if (proc && !proc->mon) { + if (!mon) { char *err = QEMU_PROCESS_ERROR(proc) ? QEMU_PROCESS_ERROR(proc) : _("unknown error"); virReportError(VIR_ERR_INTERNAL_ERROR, diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index a741d1cf91..2640ec2b32 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8148,10 +8148,6 @@ qemuProcessNew(const char *binary, } -/* Returns -1 on fatal error, - * 0 on success, - * 1 when probing QEMU failed - */ int qemuProcessRun(qemuProcessPtr proc){ virDomainXMLOptionPtr xmlopt = NULL; @@ -8218,6 +8214,7 @@ qemuProcessRun(qemuProcessPtr proc){ virObjectLock(proc->mon); + ignore: ret = 0; cleanup: @@ -8226,10 +8223,6 @@ qemuProcessRun(qemuProcessPtr proc){ virObjectUnref(xmlopt); return ret; - - ignore: - ret = 1; - goto cleanup; } diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h index abd80baf5c..37194e2501 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -235,6 +235,9 @@ struct _qemuProcess { #define QEMU_PROCESS_ERROR(proc) \ ((char *) (proc ? proc->qmperr: NULL)) +#define QEMU_PROCESS_MONITOR(proc) \ + ((qemuMonitorPtr) (proc ? proc->mon : NULL)) + qemuProcessPtr qemuProcessNew(const char *binary, const char *libDir, uid_t runUid, -- 2.17.1
On 11/11/2018 08:59 PM, Chris Venteicher wrote:
Failure to connect to QEMU to probe capabilities is not considered a error case and does not result in a negative return value.
Check for a NULL monitor connection pointer before trying to send capabilities commands to QEMU rather than depending on a special positive return value.
This simplifies logic and is more consistent with the operation of existing qemu_process functions.
A macro is introduced to easily obtain the monitor pointer from the qemuProcess structure.
Signed-off-by: Chris Venteicher <cventeic@redhat.com> --- src/qemu/qemu_capabilities.c | 28 ++++++++++++++++++---------- src/qemu/qemu_process.c | 9 +-------- src/qemu/qemu_process.h | 3 +++ 3 files changed, 22 insertions(+), 18 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index f5e327097e..fbb4336201 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -4220,43 +4220,51 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, { qemuProcessPtr proc = NULL; qemuProcessPtr proc_kvm = NULL; + qemuMonitorPtr mon = NULL; + qemuMonitorPtr mon_kvm = NULL; int ret = -1; - int rc; bool forceTCG = false;
if (!(proc = qemuProcessNew(qemuCaps->binary, libDir, runUid, runGid, forceTCG))) goto cleanup;
- if ((rc = qemuProcessRun(proc)) != 0) { - if (rc == 1) - ret = 0; + + if (qemuProcessRun(proc) < 0) + goto cleanup; + + if (!(mon = QEMU_PROCESS_MONITOR(proc))) { + ret = 0; /* Failure probing QEMU not considered error case */ goto cleanup; }
- if (virQEMUCapsInitQMPMonitor(qemuCaps, proc->mon) < 0) + /* Pull capabilities from QEMU */ + if (virQEMUCapsInitQMPMonitor(qemuCaps, mon) < 0) goto cleanup;
+ /* Pull capabilities again if KVM supported */ if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM)) { qemuProcessStopQmp(proc);
forceTCG = true; proc_kvm = qemuProcessNew(qemuCaps->binary, libDir, runUid, runGid, forceTCG);
- if ((rc = qemuProcessRun(proc_kvm)) != 0) { - if (rc == 1) - ret = 0; + if (qemuProcessRun(proc_kvm) < 0) + goto cleanup; + + if (!(mon_kvm = QEMU_PROCESS_MONITOR(proc_kvm))) { + ret = 0; /* Failure probing QEMU not considered error case */ goto cleanup; }
- if (virQEMUCapsInitQMPMonitorTCG(qemuCaps, proc_kvm->mon) < 0) + if (virQEMUCapsInitQMPMonitorTCG(qemuCaps, mon_kvm) < 0) goto cleanup; }
ret = 0;
cleanup: - if (proc && !proc->mon) { + if (!mon) { char *err = QEMU_PROCESS_ERROR(proc) ? QEMU_PROCESS_ERROR(proc) : _("unknown error");
virReportError(VIR_ERR_INTERNAL_ERROR, diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index a741d1cf91..2640ec2b32 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8148,10 +8148,6 @@ qemuProcessNew(const char *binary, }
-/* Returns -1 on fatal error, - * 0 on success, - * 1 when probing QEMU failed - */ int qemuProcessRun(qemuProcessPtr proc){ virDomainXMLOptionPtr xmlopt = NULL; @@ -8218,6 +8214,7 @@ qemuProcessRun(qemuProcessPtr proc){
virObjectLock(proc->mon);
+ ignore:
How about removing this label completely? I mean, s/goto ignore;/ret = 0; goto cleanup;/
ret = 0;
cleanup: @@ -8226,10 +8223,6 @@ qemuProcessRun(qemuProcessPtr proc){ virObjectUnref(xmlopt);
return ret; - - ignore: - ret = 1; - goto cleanup; }
diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h index abd80baf5c..37194e2501 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -235,6 +235,9 @@ struct _qemuProcess { #define QEMU_PROCESS_ERROR(proc) \ ((char *) (proc ? proc->qmperr: NULL))
+#define QEMU_PROCESS_MONITOR(proc) \ + ((qemuMonitorPtr) (proc ? proc->mon : NULL))
This looks like an overkill to me. At the only two points where this is used @proc/@proc_kvm can't be NULL so proc->mon/proc_kvm->mon can be accessed directly.
+ qemuProcessPtr qemuProcessNew(const char *binary, const char *libDir, uid_t runUid,
Michal
On Sun, Nov 11, 2018 at 13:59:17 -0600, Chris Venteicher wrote:
Failure to connect to QEMU to probe capabilities is not considered a error case and does not result in a negative return value.
That's not really true anymore. It used to be true when there was a fallback to parsing qemu -help output, but that's gone for some time now. So even though virQEMUCapsInitQMP reports 0 when probe fails, the caller (virQEMUCapsNewForBinaryInternal) uses !qemuCaps->usedQMP check to see whether we actually probed for anything. If not, an error is reported anyway. Thus this patch can be dropped completely and virQEMUCapsInitQMP should be fixed to just properly report an error when probing fails. And it should be done earlier, just before patch 8 I think. Jirka
Move a step closer to the function structure used elsewhere in qemu_process where qemuProcessStart and qemuProcessStop are the exposed functions. qemuProcessStartQmp mirrors qemuProcessStart in calling sub functions to intialize, launch the process and connect the monitor to the QEMU process. static functions qemuProcessInitQmp, qemuProcessLaunchQmp and qemuConnectMonitorQmp are also introduced. qemuProcessLaunchQmp is just renamed from qemuProcessRun and encapsulates all of the original code. qemuProcessInitQmp and qemuProcessMonitorQmp are introduced as empty wrappers into which subsequent patches will partition code from qemuProcessLaunchQmp. Signed-off-by: Chris Venteicher <cventeic@redhat.com> --- src/qemu/qemu_capabilities.c | 4 +- src/qemu/qemu_process.c | 96 +++++++++++++++++++++++++++++++++++- src/qemu/qemu_process.h | 2 +- 3 files changed, 97 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index fbb4336201..7168c470f6 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -4230,7 +4230,7 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, goto cleanup; - if (qemuProcessRun(proc) < 0) + if (qemuProcessStartQmp(proc) < 0) goto cleanup; if (!(mon = QEMU_PROCESS_MONITOR(proc))) { @@ -4249,7 +4249,7 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, forceTCG = true; proc_kvm = qemuProcessNew(qemuCaps->binary, libDir, runUid, runGid, forceTCG); - if (qemuProcessRun(proc_kvm) < 0) + if (qemuProcessStartQmp(proc_kvm) < 0) goto cleanup; if (!(mon_kvm = QEMU_PROCESS_MONITOR(proc_kvm))) { diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 2640ec2b32..b6aa3a9af3 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8148,8 +8148,29 @@ qemuProcessNew(const char *binary, } -int -qemuProcessRun(qemuProcessPtr proc){ +/* Initialize configuration and paths prior to starting QEMU + */ +static int +qemuProcessInitQmp(qemuProcessPtr proc) +{ + int ret = -1; + + VIR_DEBUG("Beginning VM startup process" + "emulator=%s", + proc->binary); + + ret = 0; + + VIR_DEBUG("ret=%i", ret); + return ret; +} + + +/* Launch QEMU Process + */ +static int +qemuProcessLaunchQmp(qemuProcessPtr proc) +{ virDomainXMLOptionPtr xmlopt = NULL; const char *machine; int status = 0; @@ -8226,6 +8247,77 @@ qemuProcessRun(qemuProcessPtr proc){ } +/* Connect Monitor to QEMU Process + */ +static int +qemuConnectMonitorQmp(qemuProcessPtr proc) +{ + int ret = -1; + + VIR_DEBUG("proc=%p, emulator=%s, proc->pid=%lld", + proc, NULLSTR(proc->binary), (long long) proc->pid); + + ret = 0; + + VIR_DEBUG("ret=%i", ret); + return ret; +} + + +/** + * qemuProcessStartQmp: + * @proc: Stores Process and Connection State + * + * Start and connect to QEMU binary so QMP queries can be made. + * + * Usage: + * proc = qemuProcessNew(binary, forceTCG, libDir, runUid, runGid); + * qemuProcessStartQmp(proc); + * mon = QEMU_PROCESS_MONITOR(proc); + * ** Send QMP Queries to QEMU using monitor ** + * qemuProcessStopQmp(proc); + * qemuProcessFree(proc); + * + * Check monitor is not NULL before using. + * + * QEMU probing failure results in monitor being NULL but is not considered + * an error case and does not result in a negative return value. + * + * Both qemuProcessStopQmp and qemuProcessFree must be called to cleanup and + * free resources, even in activation failure cases. + * + * Process error output (proc->qmperr) remains available in qemuProcess struct + * until qemuProcessFree is called. + */ +int +qemuProcessStartQmp(qemuProcessPtr proc) +{ + int ret = -1; + + VIR_DEBUG("emulator=%s", + proc->binary); + + if (qemuProcessInitQmp(proc) < 0) + goto cleanup; + + if (qemuProcessLaunchQmp(proc) < 0) + goto stop; + + if (qemuConnectMonitorQmp(proc) < 0) + goto stop; + + ret = 0; + + cleanup: + VIR_DEBUG("ret=%i", ret); + return ret; + + stop: + qemuProcessStopQmp(proc); + goto cleanup; +} + + void qemuProcessStopQmp(qemuProcessPtr proc) { diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h index 37194e2501..c34ca52ef5 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -246,7 +246,7 @@ qemuProcessPtr qemuProcessNew(const char *binary, void qemuProcessFree(qemuProcessPtr proc); -int qemuProcessRun(qemuProcessPtr proc); +int qemuProcessStartQmp(qemuProcessPtr proc); void qemuProcessStopQmp(qemuProcessPtr proc); -- 2.17.1
On 11/11/2018 08:59 PM, Chris Venteicher wrote:
Move a step closer to the function structure used elsewhere in qemu_process where qemuProcessStart and qemuProcessStop are the exposed functions.
qemuProcessStartQmp mirrors qemuProcessStart in calling sub functions to intialize, launch the process and connect the monitor to the QEMU process.
static functions qemuProcessInitQmp, qemuProcessLaunchQmp and qemuConnectMonitorQmp are also introduced.
qemuProcessLaunchQmp is just renamed from qemuProcessRun and encapsulates all of the original code.
qemuProcessInitQmp and qemuProcessMonitorQmp are introduced as empty wrappers into which subsequent patches will partition code from qemuProcessLaunchQmp.
Signed-off-by: Chris Venteicher <cventeic@redhat.com> --- src/qemu/qemu_capabilities.c | 4 +- src/qemu/qemu_process.c | 96 +++++++++++++++++++++++++++++++++++- src/qemu/qemu_process.h | 2 +- 3 files changed, 97 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index fbb4336201..7168c470f6 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -4230,7 +4230,7 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, goto cleanup;
- if (qemuProcessRun(proc) < 0) + if (qemuProcessStartQmp(proc) < 0) goto cleanup;
if (!(mon = QEMU_PROCESS_MONITOR(proc))) { @@ -4249,7 +4249,7 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, forceTCG = true; proc_kvm = qemuProcessNew(qemuCaps->binary, libDir, runUid, runGid, forceTCG);
- if (qemuProcessRun(proc_kvm) < 0) + if (qemuProcessStartQmp(proc_kvm) < 0) goto cleanup;
if (!(mon_kvm = QEMU_PROCESS_MONITOR(proc_kvm))) { diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 2640ec2b32..b6aa3a9af3 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8148,8 +8148,29 @@ qemuProcessNew(const char *binary, }
-int -qemuProcessRun(qemuProcessPtr proc){ +/* Initialize configuration and paths prior to starting QEMU + */ +static int +qemuProcessInitQmp(qemuProcessPtr proc) +{ + int ret = -1; + + VIR_DEBUG("Beginning VM startup process" + "emulator=%s", + proc->binary); + + ret = 0; + + VIR_DEBUG("ret=%i", ret); + return ret; +} +
I am sorry, but I'm failing to see the purpose of this function.
+ +/* Launch QEMU Process + */ +static int +qemuProcessLaunchQmp(qemuProcessPtr proc) +{ virDomainXMLOptionPtr xmlopt = NULL; const char *machine; int status = 0; @@ -8226,6 +8247,77 @@ qemuProcessRun(qemuProcessPtr proc){ }
+/* Connect Monitor to QEMU Process + */ +static int +qemuConnectMonitorQmp(qemuProcessPtr proc) +{ + int ret = -1; + + VIR_DEBUG("proc=%p, emulator=%s, proc->pid=%lld", + proc, NULLSTR(proc->binary), (long long) proc->pid); + + ret = 0; + + VIR_DEBUG("ret=%i", ret); + return ret; +}
Or this function. Looking into next patches I can see that you're extending them. Well, I still think it's not worth introducing empty functions, just do the rename as you're doing in next patches.
+ + +/** + * qemuProcessStartQmp: + * @proc: Stores Process and Connection State + * + * Start and connect to QEMU binary so QMP queries can be made. + * + * Usage: + * proc = qemuProcessNew(binary, forceTCG, libDir, runUid, runGid); + * qemuProcessStartQmp(proc); + * mon = QEMU_PROCESS_MONITOR(proc); + * ** Send QMP Queries to QEMU using monitor ** + * qemuProcessStopQmp(proc); + * qemuProcessFree(proc); + * + * Check monitor is not NULL before using. + * + * QEMU probing failure results in monitor being NULL but is not considered + * an error case and does not result in a negative return value. + * + * Both qemuProcessStopQmp and qemuProcessFree must be called to cleanup and + * free resources, even in activation failure cases. + * + * Process error output (proc->qmperr) remains available in qemuProcess struct + * until qemuProcessFree is called. + */ +int +qemuProcessStartQmp(qemuProcessPtr proc) +{ + int ret = -1; + + VIR_DEBUG("emulator=%s", + proc->binary); + + if (qemuProcessInitQmp(proc) < 0) + goto cleanup; + + if (qemuProcessLaunchQmp(proc) < 0) + goto stop; + + if (qemuConnectMonitorQmp(proc) < 0) + goto stop; + + ret = 0; + + cleanup: + VIR_DEBUG("ret=%i", ret); + return ret; + + stop: + qemuProcessStopQmp(proc); + goto cleanup; +} + + void qemuProcessStopQmp(qemuProcessPtr proc) { diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h index 37194e2501..c34ca52ef5 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -246,7 +246,7 @@ qemuProcessPtr qemuProcessNew(const char *binary,
void qemuProcessFree(qemuProcessPtr proc);
-int qemuProcessRun(qemuProcessPtr proc); +int qemuProcessStartQmp(qemuProcessPtr proc);
void qemuProcessStopQmp(qemuProcessPtr proc);
Michal
Quoting Michal Privoznik (2018-11-14 09:45:07)
On 11/11/2018 08:59 PM, Chris Venteicher wrote:
Move a step closer to the function structure used elsewhere in qemu_process where qemuProcessStart and qemuProcessStop are the exposed functions.
qemuProcessStartQmp mirrors qemuProcessStart in calling sub functions to intialize, launch the process and connect the monitor to the QEMU process.
static functions qemuProcessInitQmp, qemuProcessLaunchQmp and qemuConnectMonitorQmp are also introduced.
qemuProcessLaunchQmp is just renamed from qemuProcessRun and encapsulates all of the original code.
qemuProcessInitQmp and qemuProcessMonitorQmp are introduced as empty wrappers into which subsequent patches will partition code from qemuProcessLaunchQmp.
Signed-off-by: Chris Venteicher <cventeic@redhat.com> --- src/qemu/qemu_capabilities.c | 4 +- src/qemu/qemu_process.c | 96 +++++++++++++++++++++++++++++++++++- src/qemu/qemu_process.h | 2 +- 3 files changed, 97 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index fbb4336201..7168c470f6 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -4230,7 +4230,7 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, goto cleanup;
- if (qemuProcessRun(proc) < 0) + if (qemuProcessStartQmp(proc) < 0) goto cleanup;
if (!(mon = QEMU_PROCESS_MONITOR(proc))) { @@ -4249,7 +4249,7 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, forceTCG = true; proc_kvm = qemuProcessNew(qemuCaps->binary, libDir, runUid, runGid, forceTCG);
- if (qemuProcessRun(proc_kvm) < 0) + if (qemuProcessStartQmp(proc_kvm) < 0) goto cleanup;
if (!(mon_kvm = QEMU_PROCESS_MONITOR(proc_kvm))) { diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 2640ec2b32..b6aa3a9af3 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8148,8 +8148,29 @@ qemuProcessNew(const char *binary, }
-int -qemuProcessRun(qemuProcessPtr proc){ +/* Initialize configuration and paths prior to starting QEMU + */ +static int +qemuProcessInitQmp(qemuProcessPtr proc) +{ + int ret = -1; + + VIR_DEBUG("Beginning VM startup process" + "emulator=%s", + proc->binary); + + ret = 0; + + VIR_DEBUG("ret=%i", ret); + return ret; +} +
I am sorry, but I'm failing to see the purpose of this function.
+ +/* Launch QEMU Process + */ +static int +qemuProcessLaunchQmp(qemuProcessPtr proc) +{ virDomainXMLOptionPtr xmlopt = NULL; const char *machine; int status = 0; @@ -8226,6 +8247,77 @@ qemuProcessRun(qemuProcessPtr proc){ }
+/* Connect Monitor to QEMU Process + */ +static int +qemuConnectMonitorQmp(qemuProcessPtr proc) +{ + int ret = -1; + + VIR_DEBUG("proc=%p, emulator=%s, proc->pid=%lld", + proc, NULLSTR(proc->binary), (long long) proc->pid); + + ret = 0; + + VIR_DEBUG("ret=%i", ret); + return ret; +}
Or this function. Looking into next patches I can see that you're extending them. Well, I still think it's not worth introducing empty functions, just do the rename as you're doing in next patches.
Yep, I was trying to make it easier to review but didn't explain well enough from the start. Sorry I wasn't clear. Patch 10 (this patch) and 12 are about making it possible to do simple cut/paste moves on semi-complicated blocks of original code moved within patches 11 and 13. The goal was to make patches 11 and 13 easy to review because I don't actually change the code. It's just moved. If this seems good with the better explanation I can just try to make that clear in the commit message for patch 10 and 12. If it's more confusing this way I can start out with qemuProcesStartQmp only calling qemuProcessLaunchQmp and the add qemuProcessInitQmp and qemuConnectMonitorQmp as individual commits with full contents and just explain that the guts are cut/paste moves with no changes in the commit message. Please let me know which approach you think is best.
+ + +/** + * qemuProcessStartQmp: + * @proc: Stores Process and Connection State + * + * Start and connect to QEMU binary so QMP queries can be made. + * + * Usage: + * proc = qemuProcessNew(binary, forceTCG, libDir, runUid, runGid); + * qemuProcessStartQmp(proc); + * mon = QEMU_PROCESS_MONITOR(proc); + * ** Send QMP Queries to QEMU using monitor ** + * qemuProcessStopQmp(proc); + * qemuProcessFree(proc); + * + * Check monitor is not NULL before using. + * + * QEMU probing failure results in monitor being NULL but is not considered + * an error case and does not result in a negative return value. + * + * Both qemuProcessStopQmp and qemuProcessFree must be called to cleanup and + * free resources, even in activation failure cases. + * + * Process error output (proc->qmperr) remains available in qemuProcess struct + * until qemuProcessFree is called. + */ +int +qemuProcessStartQmp(qemuProcessPtr proc) +{ + int ret = -1; + + VIR_DEBUG("emulator=%s", + proc->binary); + + if (qemuProcessInitQmp(proc) < 0) + goto cleanup; + + if (qemuProcessLaunchQmp(proc) < 0) + goto stop; + + if (qemuConnectMonitorQmp(proc) < 0) + goto stop; + + ret = 0; + + cleanup: + VIR_DEBUG("ret=%i", ret); + return ret; + + stop: + qemuProcessStopQmp(proc); + goto cleanup; +} + + void qemuProcessStopQmp(qemuProcessPtr proc) { diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h index 37194e2501..c34ca52ef5 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -246,7 +246,7 @@ qemuProcessPtr qemuProcessNew(const char *binary,
void qemuProcessFree(qemuProcessPtr proc);
-int qemuProcessRun(qemuProcessPtr proc); +int qemuProcessStartQmp(qemuProcessPtr proc);
void qemuProcessStopQmp(qemuProcessPtr proc);
Michal
On Wed, Nov 14, 2018 at 10:47:14 -0600, Chris Venteicher wrote:
Quoting Michal Privoznik (2018-11-14 09:45:07)
On 11/11/2018 08:59 PM, Chris Venteicher wrote:
Move a step closer to the function structure used elsewhere in qemu_process where qemuProcessStart and qemuProcessStop are the exposed functions.
qemuProcessStartQmp mirrors qemuProcessStart in calling sub functions to intialize, launch the process and connect the monitor to the QEMU process.
static functions qemuProcessInitQmp, qemuProcessLaunchQmp and qemuConnectMonitorQmp are also introduced.
qemuProcessLaunchQmp is just renamed from qemuProcessRun and encapsulates all of the original code.
qemuProcessInitQmp and qemuProcessMonitorQmp are introduced as empty wrappers into which subsequent patches will partition code from qemuProcessLaunchQmp.
...
+/* Initialize configuration and paths prior to starting QEMU + */ +static int +qemuProcessInitQmp(qemuProcessPtr proc) +{ + int ret = -1; + + VIR_DEBUG("Beginning VM startup process" + "emulator=%s", + proc->binary); + + ret = 0; + + VIR_DEBUG("ret=%i", ret); + return ret; +} +
I am sorry, but I'm failing to see the purpose of this function.
+ +/* Launch QEMU Process + */ +static int +qemuProcessLaunchQmp(qemuProcessPtr proc) +{ virDomainXMLOptionPtr xmlopt = NULL; const char *machine; int status = 0; @@ -8226,6 +8247,77 @@ qemuProcessRun(qemuProcessPtr proc){ }
+/* Connect Monitor to QEMU Process + */ +static int +qemuConnectMonitorQmp(qemuProcessPtr proc) +{ + int ret = -1; + + VIR_DEBUG("proc=%p, emulator=%s, proc->pid=%lld", + proc, NULLSTR(proc->binary), (long long) proc->pid); + + ret = 0; + + VIR_DEBUG("ret=%i", ret); + return ret; +}
Or this function. Looking into next patches I can see that you're extending them. Well, I still think it's not worth introducing empty functions, just do the rename as you're doing in next patches.
Yep, I was trying to make it easier to review but didn't explain well enough from the start. Sorry I wasn't clear.
Patch 10 (this patch) and 12 are about making it possible to do simple cut/paste moves on semi-complicated blocks of original code moved within patches 11 and 13.
The goal was to make patches 11 and 13 easy to review because I don't actually change the code. It's just moved.
If this seems good with the better explanation I can just try to make that clear in the commit message for patch 10 and 12.
If it's more confusing this way I can start out with qemuProcesStartQmp only calling qemuProcessLaunchQmp and the add qemuProcessInitQmp and qemuConnectMonitorQmp as individual commits with full contents and just explain that the guts are cut/paste moves with no changes in the commit message.
The approach would be fine, but I believe Michal just didn't see any reason to split the function in three parts in the first place. qemuProcessStart which you're trying to mirror was split into several phase because there is a code which needs to call the individual phases separately so that additional logic can be put between the phases. But it doesn't look like you need to do anything like this in your series. So unless you want to somehow merge parts of the code for qemuProcessStart and qemuProcessStartQmp, there's no real reason for the split. So either you need to explain why we need to split the function or you can drop this and the follow up patches doing the split. And since this and the changes I suggested for the patches 1-9 may result non-trivial changes to the other patches in this series, I'm going to stop my series now. The main reason is that it is not entirely obvious which of the following patches are just advancing the function split or fixing issues introduced by the split and which of them would be needed anyway. So please, digest the review and send an updated version for further review. And as explained in my reply to the cover latter, please send both this refactoring and the new CPU related code in one series. I hope it won't take me so long to review. I apologize for the delay this time. Jirka
qemuMonitor code lives in qemuConnectMonitorQmp rather than in qemuProcessNew and qemuProcessLaunchQmp. This is consistent with existing structure in qemu_process.c where qemuConnectMonitor function contains monitor code for domain process activation. Simple code moves in this patch. Improvements in later patch. Only intended functional change in this patch is we don't move (include) code to initiate process stop on failure to create monitor. As comments in qemuProcessStartQmp say... Client must always call qemuProcessStop and qemuProcessFree, even in error cases. Signed-off-by: Chris Venteicher <cventeic@redhat.com> --- src/qemu/qemu_process.c | 41 ++++++++++++++++++++--------------------- 1 file changed, 20 insertions(+), 21 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index b6aa3a9af3..fe4361ed5d 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8136,10 +8136,6 @@ qemuProcessNew(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: @@ -8171,7 +8167,6 @@ qemuProcessInitQmp(qemuProcessPtr proc) static int qemuProcessLaunchQmp(qemuProcessPtr proc) { - virDomainXMLOptionPtr xmlopt = NULL; const char *machine; int status = 0; int ret = -1; @@ -8223,26 +8218,10 @@ qemuProcessLaunchQmp(qemuProcessPtr proc) goto ignore; } - if (!(xmlopt = virDomainXMLOptionNew(NULL, NULL, NULL, NULL, NULL)) || - !(proc->vm = virDomainObjNew(xmlopt))) - goto cleanup; - - proc->vm->pid = proc->pid; - - if (!(proc->mon = qemuMonitorOpen(proc->vm, &proc->config, true, true, - 0, &callbacks, NULL))) - goto ignore; - - virObjectLock(proc->mon); - ignore: ret = 0; cleanup: - if (!proc->mon) - qemuProcessStopQmp(proc); - virObjectUnref(xmlopt); - return ret; } @@ -8253,13 +8232,33 @@ static int qemuConnectMonitorQmp(qemuProcessPtr proc) { int ret = -1; + virDomainXMLOptionPtr xmlopt = NULL; VIR_DEBUG("proc=%p, emulator=%s, proc->pid=%lld", proc, NULLSTR(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; + + proc->vm->pid = proc->pid; + + if (!(proc->mon = qemuMonitorOpen(proc->vm, &proc->config, true, true, + 0, &callbacks, NULL))) + goto ignore; + + virObjectLock(proc->mon); + + ignore: ret = 0; + cleanup: VIR_DEBUG("ret=%i", ret); + virObjectUnref(xmlopt); return ret; } -- 2.17.1
Store libDir path in the qemuProcess struct in anticipation of moving path construction code into qemuProcessInitQmp function. Signed-off-by: Chris Venteicher <cventeic@redhat.com> --- 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 fe4361ed5d..0b96debf25 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8088,6 +8088,7 @@ qemuProcessFree(qemuProcessPtr proc) qemuProcessStopQmp(proc); VIR_FREE(proc->binary); + VIR_FREE(proc->libDir); VIR_FREE(proc->monpath); VIR_FREE(proc->monarg); VIR_FREE(proc->pidfile); @@ -8108,7 +8109,8 @@ qemuProcessNew(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->forceTCG = forceTCG; @@ -8119,7 +8121,7 @@ qemuProcessNew(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) @@ -8131,7 +8133,7 @@ qemuProcessNew(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 c34ca52ef5..1b8f743861 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -218,6 +218,7 @@ typedef struct _qemuProcess qemuProcess; typedef qemuProcess *qemuProcessPtr; struct _qemuProcess { char *binary; + char *libDir; uid_t runUid; gid_t runGid; char *qmperr; /* qemu process stderr */ -- 2.17.1
Move code for setting paths and prepping file system from qemuProcessNew to qemuProcessInitQmp. This keeps qemuProcessNew limited to structure initialization and most closely mirrors pattern established in qemuProcessInit within qemuProcessInitQmp. 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> --- 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 0b96debf25..8e98b97443 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8118,26 +8118,6 @@ qemuProcessNew(const char *binary, proc->runUid = runUid; proc->runGid = runGid; - /* 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: @@ -8157,8 +8137,30 @@ qemuProcessInitQmp(qemuProcessPtr proc) "emulator=%s", 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: VIR_DEBUG("ret=%i", ret); return ret; } -- 2.17.1
The monitor config data is removed from the qemuProcess 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> --- 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 8e98b97443..d4ed2d6353 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8237,13 +8237,14 @@ qemuConnectMonitorQmp(qemuProcessPtr proc) { int ret = -1; virDomainXMLOptionPtr xmlopt = NULL; + virDomainChrSourceDef monConfig; VIR_DEBUG("proc=%p, emulator=%s, proc->pid=%lld", proc, NULLSTR(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))) @@ -8251,7 +8252,7 @@ qemuConnectMonitorQmp(qemuProcessPtr 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 ignore; diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h index 1b8f743861..d1541d5407 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -227,7 +227,6 @@ struct _qemuProcess { char *pidfile; virCommandPtr cmd; qemuMonitorPtr mon; - virDomainChrSourceDef config; pid_t pid; virDomainObjPtr vm; bool forceTCG; -- 2.17.1
Gracefully handle case when proc activation failed prior to calling. Consistent with the existing code for qemuConnectMonitor (for domains) in qemu_process.c... - Handle qemMonitorOpen failure with INFO message and NULL ptr - Identify parameters passed to qemuMonitorOpen Monitor callbacks will be removed in future patch so we prep for passing NULL for the callback pointer. Set proc->mon to NULL then use VIR_STEAL_PTR if successful to be consistent with other functions. Signed-off-by: Chris Venteicher <cventeic@redhat.com> --- src/qemu/qemu_process.c | 27 +++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index d4ed2d6353..55a092ecbb 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8236,25 +8236,48 @@ static int qemuConnectMonitorQmp(qemuProcessPtr proc) { int ret = -1; + qemuMonitorPtr mon = NULL; + unsigned long long timeout = 0; + bool retry = true; + bool enableJson = true; + virQEMUDriverPtr driver = NULL; + qemuMonitorCallbacksPtr monCallbacks = &callbacks; virDomainXMLOptionPtr xmlopt = NULL; virDomainChrSourceDef monConfig; VIR_DEBUG("proc=%p, emulator=%s, proc->pid=%lld", proc, NULLSTR(proc->binary), (long long) proc->pid); + if (!proc || !proc->pid) + goto ignore; + + proc->mon = NULL; + monConfig.type = VIR_DOMAIN_CHR_TYPE_UNIX; monConfig.data.nix.path = proc->monpath; monConfig.data.nix.listen = false; + /* Create a NULL Domain object for qemuMonitor */ if (!(xmlopt = virDomainXMLOptionNew(NULL, NULL, NULL, NULL, NULL)) || !(proc->vm = virDomainObjNew(xmlopt))) goto cleanup; proc->vm->pid = proc->pid; - if (!(proc->mon = qemuMonitorOpen(proc->vm, &monConfig, true, true, - 0, &callbacks, NULL))) + mon = qemuMonitorOpen(proc->vm, + &monConfig, + enableJson, + retry, + timeout, + monCallbacks, + driver); + + if (!mon) { + VIR_INFO("Failed to connect monitor to emulator %s", proc->binary); goto ignore; + } + + VIR_STEAL_PTR(proc->mon, mon); virObjectLock(proc->mon); -- 2.17.1
qemuProcessNew is one of the 4 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. No functional changes are made. Signed-off-by: Chris Venteicher <cventeic@redhat.com> --- src/qemu/qemu_process.c | 26 +++++++++++++++++++++----- 1 file changed, 21 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 55a092ecbb..5ff7d6878c 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8097,6 +8097,18 @@ qemuProcessFree(qemuProcessPtr proc) } +/** + * qemuProcessNew: + * @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 to QEMU + * for completing QMP Queries. + */ qemuProcessPtr qemuProcessNew(const char *binary, const char *libDir, @@ -8104,25 +8116,29 @@ qemuProcessNew(const char *binary, gid_t runGid, bool forceTCG) { + qemuProcessPtr ret = NULL; qemuProcessPtr proc = NULL; + VIR_DEBUG("exec=%s, libDir=%s, runUid=%u, runGid=%u, forceTCG=%d", + NULLSTR(binary), NULLSTR(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->forceTCG = forceTCG; proc->runUid = runUid; proc->runGid = runGid; - return proc; + VIR_STEAL_PTR(ret, proc); - error: + cleanup: qemuProcessFree(proc); - return NULL; + return ret; } -- 2.17.1
qemuProcessStopQmp is one of the 4 public functions used to create and manage a Qemu process for QMP command exchanges. Add comment header and debug message. Other minor code formatting cleanup. No change in functionality is intended. Signed-off-by: Chris Venteicher <cventeic@redhat.com> --- src/qemu/qemu_process.c | 29 ++++++++++++++++++++++------- 1 file changed, 22 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 5ff7d6878c..27a959cf9d 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8360,14 +8360,27 @@ qemuProcessStartQmp(qemuProcessPtr proc) goto cleanup; } - -void -qemuProcessStopQmp(qemuProcessPtr proc) +/** + * qemuProcessStop: + * @proc: Stores Process and Connection State + * + * Stop Monitor Connection and QEMU Process + */ +void qemuProcessStopQmp(qemuProcessPtr proc) { - if (proc->mon) - virObjectUnlock(proc->mon); - qemuMonitorClose(proc->mon); - proc->mon = NULL; + qemuMonitorPtr mon = NULL; + + if (!proc) + return; + + VIR_DEBUG("Shutting down proc=%p emulator=%s mon=%p pid=%lld", + proc, proc->binary, proc->mon, (long long)proc->pid); + + if ((mon = QEMU_PROCESS_MONITOR(proc))) { + virObjectUnlock(mon); + qemuMonitorClose(mon); + proc->mon = NULL; + } virCommandAbort(proc->cmd); virCommandFree(proc->cmd); @@ -8388,7 +8401,9 @@ qemuProcessStopQmp(qemuProcessPtr proc) virStrerror(errno, ebuf, sizeof(ebuf))); } + if (proc->pidfile) unlink(proc->pidfile); + proc->pid = 0; } -- 2.17.1
Catch execution paths where qemuProcessFree is called before qemuProcessStopQmp then report error and force stop before proceeding. Also added public function header and debug message. Signed-off-by: Chris Venteicher <cventeic@redhat.com> --- src/qemu/qemu_process.c | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 27a959cf9d..c44e46fc88 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8078,15 +8078,28 @@ static qemuMonitorCallbacks callbacks = { }; - - +/** + * qemuProcessFree: + * @proc: Stores Process and Connection State + * + * Free process data structure. + */ void qemuProcessFree(qemuProcessPtr proc) { + VIR_DEBUG("proc=%p, proc->mon=%p", proc, (proc ? proc->mon : NULL)); + if (!proc) return; - qemuProcessStopQmp(proc); + /* This should never be non-NULL if we get here, but just in case... */ + if (proc->mon || proc->pid) { + VIR_ERROR(_("Unexpected QEMU still active during process free" + " emulator: %s, pid: %lld, mon: %p"), + NULLSTR(proc->binary), (long long) proc->pid, proc->mon); + qemuProcessStopQmp(proc); + } + VIR_FREE(proc->binary); VIR_FREE(proc->libDir); VIR_FREE(proc->monpath); -- 2.17.1
Qemu process code for capababilities doesn't use monitor callbacks and defines empty callback functions. Allow NULL to be passed to qemuMonitorOpen for callbacks and remove the empty functions from the QMP process code. Signed-off-by: Chris Venteicher <cventeic@redhat.com> --- src/qemu/qemu_monitor.c | 4 ++-- src/qemu/qemu_process.c | 14 +------------- 2 files changed, 3 insertions(+), 15 deletions(-) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 7f7013e115..77f4fe7cf7 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -813,12 +813,12 @@ qemuMonitorOpenInternal(virDomainObjPtr vm, { qemuMonitorPtr mon; - if (!cb->eofNotify) { + if (cb && !cb->eofNotify) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("EOF notify callback must be supplied")); return NULL; } - if (!cb->errorNotify) { + if (cb && !cb->errorNotify) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Error notify callback must be supplied")); return NULL; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index c44e46fc88..d20f832053 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8066,18 +8066,6 @@ qemuProcessReconnectAll(virQEMUDriverPtr driver) } -static void virQEMUCapsMonitorNotify(qemuMonitorPtr mon ATTRIBUTE_UNUSED, - virDomainObjPtr vm ATTRIBUTE_UNUSED, - void *opaque ATTRIBUTE_UNUSED) -{ -} - -static qemuMonitorCallbacks callbacks = { - .eofNotify = virQEMUCapsMonitorNotify, - .errorNotify = virQEMUCapsMonitorNotify, -}; - - /** * qemuProcessFree: * @proc: Stores Process and Connection State @@ -8270,7 +8258,7 @@ qemuConnectMonitorQmp(qemuProcessPtr proc) bool retry = true; bool enableJson = true; virQEMUDriverPtr driver = NULL; - qemuMonitorCallbacksPtr monCallbacks = &callbacks; + qemuMonitorCallbacksPtr monCallbacks = NULL; virDomainXMLOptionPtr xmlopt = NULL; virDomainChrSourceDef monConfig; -- 2.17.1
qemuProcessStartQmp 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 qemuProcess code so multiple functions can issue QMP commands in arbitrary orders. This also simplifies the functions using the connection provided by qemuProcessStartQmp 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 qemuProcess command that internally calls qemuMonitorSetCapabilities. Signed-off-by: Chris Venteicher <cventeic@redhat.com> --- src/qemu/qemu_capabilities.c | 14 -------------- src/qemu/qemu_process.c | 8 ++++++++ tests/qemucapabilitiestest.c | 7 +++++++ 3 files changed, 15 insertions(+), 14 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 7168c470f6..f06d0a4428 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -4013,13 +4013,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()); - ret = 0; - goto cleanup; - } - if (qemuMonitorGetVersion(mon, &major, &minor, µ, &package) < 0) { @@ -4193,13 +4186,6 @@ virQEMUCapsInitQMPMonitorTCG(virQEMUCapsPtr qemuCaps ATTRIBUTE_UNUSED, { int ret = -1; - if (qemuMonitorSetCapabilities(mon) < 0) { - VIR_DEBUG("Failed to set monitor capabilities %s", - virGetLastErrorMessage()); - ret = 0; - 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 d20f832053..eba2cd6765 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8298,6 +8298,14 @@ qemuConnectMonitorQmp(qemuProcessPtr proc) virObjectLock(proc->mon); + /* Exit capabilities negotiation mode and enter QEMU command mode + * by issuing qmp_capabilities command to QEMU */ + if (qemuMonitorSetCapabilities(proc->mon) < 0) { + VIR_DEBUG("Failed to set monitor capabilities %s", + virGetLastErrorMessage()); + goto ignore; + } + ignore: ret = 0; diff --git a/tests/qemucapabilitiestest.c b/tests/qemucapabilitiestest.c index 8fe5a55e1d..ea4671739b 100644 --- a/tests/qemucapabilitiestest.c +++ b/tests/qemucapabilitiestest.c @@ -58,6 +58,9 @@ testQemuCaps(const void *opaque) if (!(mon = qemuMonitorTestNewFromFileFull(repliesFile, &data->driver, NULL))) goto cleanup; + if (qemuMonitorSetCapabilities(qemuMonitorTestGetMonitor(mon)) < 0) + goto cleanup; + if (!(capsActual = virQEMUCapsNew()) || virQEMUCapsInitQMPMonitor(capsActual, qemuMonitorTestGetMonitor(mon)) < 0) @@ -65,6 +68,10 @@ testQemuCaps(const void *opaque) if (virQEMUCapsGet(capsActual, QEMU_CAPS_KVM)) { qemuMonitorResetCommandID(qemuMonitorTestGetMonitor(mon)); + + if (qemuMonitorSetCapabilities(qemuMonitorTestGetMonitor(mon)) < 0) + goto cleanup; + if (virQEMUCapsInitQMPMonitorTCG(capsActual, qemuMonitorTestGetMonitor(mon)) < 0) goto cleanup; -- 2.17.1
Multiple QEMU processes for QMP commands can operate concurrently. Use a unique directory under libDir for each QEMU processes to avoid pidfile and unix socket collision between processes. The pid file name is changed from "capabilities.pidfile" to "qmp.pid" because we no longer need to avoid a possible clash with a qemu domain called "capabilities" now that the processes artifacts are stored in their own unique temporary directories. "Capabilities" was changed to "qmp" in the pid file name because these processes are no longer specific to the capabilities usecase and are more generic in terms of being used for any general purpose QMP message exchanges with a QEMU process that is not associated with a domain. Signed-off-by: Chris Venteicher <cventeic@redhat.com> --- src/qemu/qemu_process.c | 26 ++++++++++++++++---------- src/qemu/qemu_process.h | 1 + 2 files changed, 17 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index eba2cd6765..4dbc7038fd 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8090,6 +8090,7 @@ qemuProcessFree(qemuProcessPtr 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); @@ -8148,33 +8149,33 @@ qemuProcessNew(const char *binary, static int qemuProcessInitQmp(qemuProcessPtr proc) { + char *template = NULL; int ret = -1; VIR_DEBUG("Beginning VM startup process" "emulator=%s", proc->binary); - /* the ".sock" sufix is important to avoid a possible clash with a qemu - * domain called "capabilities" - */ - if (virAsprintf(&proc->monpath, "%s/%s", proc->libDir, - "capabilities.monitor.sock") < 0) + if (virAsprintf(&template, "%s/qemu.XXXXXX", proc->libDir) < 0) + goto cleanup; + + proc->uniqDir = mkdtemp(template); + + if (virAsprintf(&proc->monpath, "%s/%s", proc->uniqDir, + "qmp.monitor") < 0) goto cleanup; if (virAsprintf(&proc->monarg, "unix:%s,server,nowait", proc->monpath) < 0) goto cleanup; - /* ".pidfile" suffix is used rather than ".pid" to avoid a possible clash - * with a qemu domain called "capabilities" + /* * Normally we'd use runDir for pid files, but because we're using * -daemonize we need QEMU to be allowed to create them, rather * than libvirtd. So we're using libDir which QEMU can write to */ - if (virAsprintf(&proc->pidfile, "%s/%s", proc->libDir, "capabilities.pidfile") < 0) + if (virAsprintf(&proc->pidfile, "%s/%s", proc->uniqDir, "qmp.pid") < 0) goto cleanup; - virPidFileForceCleanupPath(proc->pidfile); - ret = 0; cleanup: @@ -8415,4 +8416,9 @@ void qemuProcessStopQmp(qemuProcessPtr proc) unlink(proc->pidfile); proc->pid = 0; + + + if (proc->uniqDir) + rmdir(proc->uniqDir); + } diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h index d1541d5407..f66fc0c82c 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -225,6 +225,7 @@ struct _qemuProcess { char *monarg; char *monpath; char *pidfile; + char *uniqDir; virCommandPtr cmd; qemuMonitorPtr mon; pid_t pid; -- 2.17.1
Locking the monitor object immediately after call to qemuMonitorOpen doesn't make sense now that we have expanded the QEMU process code to cover more than the original capabilities usecase. Removing the monitor lock makes the qemuConnectMonitorQmp code consistent with the qemuConnectMonitor code used to establish the monitor when QEMU process is started for domains. Signed-off-by: Chris Venteicher <cventeic@redhat.com> --- src/qemu/qemu_process.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 4dbc7038fd..2f9c1701a3 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8297,8 +8297,6 @@ qemuConnectMonitorQmp(qemuProcessPtr proc) VIR_STEAL_PTR(proc->mon, mon); - virObjectLock(proc->mon); - /* Exit capabilities negotiation mode and enter QEMU command mode * by issuing qmp_capabilities command to QEMU */ if (qemuMonitorSetCapabilities(proc->mon) < 0) { -- 2.17.1
On 11/11/2018 08:59 PM, Chris Venteicher wrote:
Make process code usable outside qemu_capabilities by moving code from qemu_capabilities to qemu_process and exposing public functions.
The process code is used to activate a non domain QEMU process for QMP message exchanges.
This patch set modifies capabilities to use the new public functions.
--
The process code is being decoupled from qemu_capabilities now to support hypervisor baseline and comparison using QMP commands.
This patch set was originally submitted as part of the baseline patch set: [libvirt] [PATCH v4 00/37] BaselineHypervisorCPU using QEMU QMP exchanges https://www.redhat.com/archives/libvir-list/2018-November/msg00091.html
Okay, so you want to implement cpu-baseline for s390. But that doesn't really explain the code movement. Also, somehow the code movement makes the code bigger? I guess what I am saying is that I don't see much justification for these patches.
The baseline and comparison requirements are described here: https://bugzilla.redhat.com/show_bug.cgi?id=1511999 https://bugzilla.redhat.com/show_bug.cgi?id=1511996
I am extracting and resubmitting just the process changes as a stand alone series to try to make review easier.
The patch set shows capabilities using the public functions. To see baseline using the public functions... Look at the "qemu_driver:" patches at the end of https://www.redhat.com/archives/libvir-list/2018-November/msg00091.html
Also, The "qemu_driver: Support feature expansion via QEMU when baselining cpu" patch might be of particular interest because the same QEMU process is used for both baseline and expansion using QMP commands.
--
Many patches were used to isolate code moves and name changes from other actual implementation changes.
The patches reuse the pattern of public qemuProcess{Start,Stop} functions and internal static qemuProcess{Init,Launch,ConnectMonitor} functions but adds a "Qmp" suffix to make them unique.
A number of patches are about re-partitioning the code into static functions for initialization, process launch and connection monitor stuff. This matches the established pattern in qemu_process and seemed to make sense to do.
For concurrency... A thread safe library function creates a unique directory under libDir for each QEMU process (for QMP messaging) to decouple processes in terms of sockets and file system footprint.
Every patch should compile independently if applied in sequence.
Oh, but it doesn't. I'm running 'make -j10 all syntax-check check' and I am hitting compilation/syntax error occasionally.
Chris Venteicher (22): qemu_process: Move process code from qemu_capabilities to qemu_process qemu_process: Use qemuProcess prefix qemu_process: Limit qemuProcessNew 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 qemuProcess struct for a single process qemu_process: Persist stderr in qemuProcess struct qemu_capabilities: Detect caps probe failure by checking monitor ptr qemu_process: Introduce qemuProcessStartQmp qemu_process: Collect monitor code in single function qemu_process: Store libDir in qemuProcess struct qemu_process: Setup paths within qemuProcessInitQmp qemu_process: Stop retaining Qemu Monitor config in qemuProcess qemu_process: Don't open monitor if process failed qemu_process: Cleanup qemuProcess alloc function qemu_process: Cleanup qemuProcessStopQmp function qemu_process: Catch process free before process stop qemu_monitor: Make monitor callbacks optional qemu_process: Enter QMP command mode when starting QEMU Process qemu_process: Use unique directories for QMP processes qemu_process: Stop locking QMP process monitor immediately
src/qemu/qemu_capabilities.c | 300 +++++------------------------ src/qemu/qemu_monitor.c | 4 +- src/qemu/qemu_process.c | 356 +++++++++++++++++++++++++++++++++++ src/qemu/qemu_process.h | 37 ++++ tests/qemucapabilitiestest.c | 7 + 5 files changed, 444 insertions(+), 260 deletions(-)
Michal
Quoting Michal Privoznik (2018-11-14 09:45:06)
On 11/11/2018 08:59 PM, Chris Venteicher wrote:
Make process code usable outside qemu_capabilities by moving code from qemu_capabilities to qemu_process and exposing public functions.
The process code is used to activate a non domain QEMU process for QMP message exchanges.
This patch set modifies capabilities to use the new public functions.
--
The process code is being decoupled from qemu_capabilities now to support hypervisor baseline and comparison using QMP commands.
This patch set was originally submitted as part of the baseline patch set: [libvirt] [PATCH v4 00/37] BaselineHypervisorCPU using QEMU QMP exchanges https://www.redhat.com/archives/libvir-list/2018-November/msg00091.html
Okay, so you want to implement cpu-baseline for s390. But that doesn't really explain the code movement. Also, somehow the code movement makes the code bigger? I guess what I am saying is that I don't see much justification for these patches.
Here is the feedback from an earlier hypervisor baseline review that resulted in this patch set. https://www.redhat.com/archives/libvir-list/2018-July/msg00881.html I think Jiri correctly identified capabilities, and now baseline and comparison, are unrelated services that all independently need to start a non-domain QEMU process for QMP messaging. I am not sure, but it seems likely there could be other (S390...) commands in the future that use QMP messages outside of a domain context to get info or do work at the QEMU level. All the baseline code I had in qemu_capabilities didn't make sense there anymore once I moved the process code from qemu_capabilities to qemu_process. Here is the latest baseline patch set: https://www.redhat.com/archives/libvir-list/2018-November/msg00091.html In the latest baseline patch set, all the baseline code is in qemu_driver and uses the process functions exposed now from qemu_process. So as best I can tell there main choice is... 1) Leave process code in qemu_capabilities and make the 4 core process functions (new, start, stop, free) and data strut public so they can also be used by baseline and comparison from qemu_driver. 2) Move the process code from qemu_capabilities to qemu_process. (this patch set) and expose the functions / data struct from qemu_process. In case 1 functions have the virQemuCaps prefix. In case 2 functions have the qemuProcess prefix. In either approach there are some changes needed to the process code to decouple it from the capabilities code to support both capabilities and baseline. I did spend a few patches in this patch set breaking out the init, process launch and monitor connection code into different static functions in the style used elsewhere in qemu_process. That could be reversed if it doesn't add enough value if the decision is to move the process code to qemu_process.
The baseline and comparison requirements are described here: https://bugzilla.redhat.com/show_bug.cgi?id=1511999 https://bugzilla.redhat.com/show_bug.cgi?id=1511996
I am extracting and resubmitting just the process changes as a stand alone series to try to make review easier.
The patch set shows capabilities using the public functions. To see baseline using the public functions... Look at the "qemu_driver:" patches at the end of https://www.redhat.com/archives/libvir-list/2018-November/msg00091.html
Also, The "qemu_driver: Support feature expansion via QEMU when baselining cpu" patch might be of particular interest because the same QEMU process is used for both baseline and expansion using QMP commands.
--
Many patches were used to isolate code moves and name changes from other actual implementation changes.
The patches reuse the pattern of public qemuProcess{Start,Stop} functions and internal static qemuProcess{Init,Launch,ConnectMonitor} functions but adds a "Qmp" suffix to make them unique.
A number of patches are about re-partitioning the code into static functions for initialization, process launch and connection monitor stuff. This matches the established pattern in qemu_process and seemed to make sense to do.
For concurrency... A thread safe library function creates a unique directory under libDir for each QEMU process (for QMP messaging) to decouple processes in terms of sockets and file system footprint.
Every patch should compile independently if applied in sequence.
Oh, but it doesn't. I'm running 'make -j10 all syntax-check check' and I am hitting compilation/syntax error occasionally.
Yep. My bad. I thought I was careful about making and checking every patch... but stuff got through. At least one of the errors looks like a slip when I did a merge as part of a rebase where I changed the patch order to make it easier to review. It's clear now I need to manualy or by script 'make -j10 all syntax-check check' on each patch before I submit.
Chris Venteicher (22): qemu_process: Move process code from qemu_capabilities to qemu_process qemu_process: Use qemuProcess prefix qemu_process: Limit qemuProcessNew 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 qemuProcess struct for a single process qemu_process: Persist stderr in qemuProcess struct qemu_capabilities: Detect caps probe failure by checking monitor ptr qemu_process: Introduce qemuProcessStartQmp qemu_process: Collect monitor code in single function qemu_process: Store libDir in qemuProcess struct qemu_process: Setup paths within qemuProcessInitQmp qemu_process: Stop retaining Qemu Monitor config in qemuProcess qemu_process: Don't open monitor if process failed qemu_process: Cleanup qemuProcess alloc function qemu_process: Cleanup qemuProcessStopQmp function qemu_process: Catch process free before process stop qemu_monitor: Make monitor callbacks optional qemu_process: Enter QMP command mode when starting QEMU Process qemu_process: Use unique directories for QMP processes qemu_process: Stop locking QMP process monitor immediately
src/qemu/qemu_capabilities.c | 300 +++++------------------------ src/qemu/qemu_monitor.c | 4 +- src/qemu/qemu_process.c | 356 +++++++++++++++++++++++++++++++++++ src/qemu/qemu_process.h | 37 ++++ tests/qemucapabilitiestest.c | 7 + 5 files changed, 444 insertions(+), 260 deletions(-)
Michal
On 11/14/2018 07:17 PM, Chris Venteicher wrote:
Quoting Michal Privoznik (2018-11-14 09:45:06)
On 11/11/2018 08:59 PM, Chris Venteicher wrote:
Make process code usable outside qemu_capabilities by moving code from qemu_capabilities to qemu_process and exposing public functions.
The process code is used to activate a non domain QEMU process for QMP message exchanges.
This patch set modifies capabilities to use the new public functions.
--
The process code is being decoupled from qemu_capabilities now to support hypervisor baseline and comparison using QMP commands.
This patch set was originally submitted as part of the baseline patch set: [libvirt] [PATCH v4 00/37] BaselineHypervisorCPU using QEMU QMP exchanges https://www.redhat.com/archives/libvir-list/2018-November/msg00091.html
Okay, so you want to implement cpu-baseline for s390. But that doesn't really explain the code movement. Also, somehow the code movement makes the code bigger? I guess what I am saying is that I don't see much justification for these patches.
Here is the feedback from an earlier hypervisor baseline review that resulted in this patch set. https://www.redhat.com/archives/libvir-list/2018-July/msg00881.html
I think Jiri correctly identified capabilities, and now baseline and comparison, are unrelated services that all independently need to start a non-domain QEMU process for QMP messaging.
I am not sure, but it seems likely there could be other (S390...) commands in the future that use QMP messages outside of a domain context to get info or do work at the QEMU level.
All the baseline code I had in qemu_capabilities didn't make sense there anymore once I moved the process code from qemu_capabilities to qemu_process.
Here is the latest baseline patch set: https://www.redhat.com/archives/libvir-list/2018-November/msg00091.html
In the latest baseline patch set, all the baseline code is in qemu_driver and uses the process functions exposed now from qemu_process.
So as best I can tell there main choice is...
1) Leave process code in qemu_capabilities and make the 4 core process functions (new, start, stop, free) and data strut public so they can also be used by baseline and comparison from qemu_driver.
2) Move the process code from qemu_capabilities to qemu_process. (this patch set) and expose the functions / data struct from qemu_process.
Oh, my bad. I just skimmed through referenced v3 and did not read it carefully. If I did I would learn that this feature you're adding is not just like any other feature. Therefore code movement and unification makes actually sense. So after all this is the right way to go. Sorry for the noise. All in all, the patches look okayish. But we will have to see v2 of them, I'm afraid.
In case 1 functions have the virQemuCaps prefix. In case 2 functions have the qemuProcess prefix.
In either approach there are some changes needed to the process code to decouple it from the capabilities code to support both capabilities and baseline.
I did spend a few patches in this patch set breaking out the init, process launch and monitor connection code into different static functions in the style used elsewhere in qemu_process. That could be reversed if it doesn't add enough value if the decision is to move the process code to qemu_process.
The baseline and comparison requirements are described here: https://bugzilla.redhat.com/show_bug.cgi?id=1511999 https://bugzilla.redhat.com/show_bug.cgi?id=1511996
I am extracting and resubmitting just the process changes as a stand alone series to try to make review easier.
The patch set shows capabilities using the public functions. To see baseline using the public functions... Look at the "qemu_driver:" patches at the end of https://www.redhat.com/archives/libvir-list/2018-November/msg00091.html
Also, The "qemu_driver: Support feature expansion via QEMU when baselining cpu" patch might be of particular interest because the same QEMU process is used for both baseline and expansion using QMP commands.
--
Many patches were used to isolate code moves and name changes from other actual implementation changes.
The patches reuse the pattern of public qemuProcess{Start,Stop} functions and internal static qemuProcess{Init,Launch,ConnectMonitor} functions but adds a "Qmp" suffix to make them unique.
A number of patches are about re-partitioning the code into static functions for initialization, process launch and connection monitor stuff. This matches the established pattern in qemu_process and seemed to make sense to do.
For concurrency... A thread safe library function creates a unique directory under libDir for each QEMU process (for QMP messaging) to decouple processes in terms of sockets and file system footprint.
Every patch should compile independently if applied in sequence.
Oh, but it doesn't. I'm running 'make -j10 all syntax-check check' and I am hitting compilation/syntax error occasionally.
Yep. My bad.
I thought I was careful about making and checking every patch... but stuff got through.
At least one of the errors looks like a slip when I did a merge as part of a rebase where I changed the patch order to make it easier to review.
It's clear now I need to manualy or by script 'make -j10 all syntax-check check' on each patch before I submit.
This can be easily done in git now (assuming you're on the branch with these patches on): libvirt.git $ git rebase --exec "make -j10 all syntax-check check" master Michal
On Sun, Nov 11, 2018 at 13:59:08 -0600, Chris Venteicher wrote:
Make process code usable outside qemu_capabilities by moving code from qemu_capabilities to qemu_process and exposing public functions.
The process code is used to activate a non domain QEMU process for QMP message exchanges.
This patch set modifies capabilities to use the new public functions.
--
The process code is being decoupled from qemu_capabilities now to support hypervisor baseline and comparison using QMP commands.
This patch set was originally submitted as part of the baseline patch set: [libvirt] [PATCH v4 00/37] BaselineHypervisorCPU using QEMU QMP exchanges https://www.redhat.com/archives/libvir-list/2018-November/msg00091.html
The baseline and comparison requirements are described here: https://bugzilla.redhat.com/show_bug.cgi?id=1511999 https://bugzilla.redhat.com/show_bug.cgi?id=1511996
I am extracting and resubmitting just the process changes as a stand alone series to try to make review easier.
I know this was suggested by Colin, but it actually makes review harder, because there's no code which would make use of the new functions and thus there seems to be no reason for refactoring. Not to mention that seeing real usage of a new function influences its design. Although it surely makes sense do have the refactoring patches in the beginning of the series rather than mixing them with the rest of the new code. I applied both this series and your former v4 series, rebased the v4 one on top of this RFC and now I'm reviewing the result. So please, keep sending just one series with all the patches. During the review, I'm not going to repeat anything already mentioned by Michal, unless I want to say something more about it. Jirka
participants (3)
-
Chris Venteicher -
Jiri Denemark -
Michal Privoznik