[libvirt] [PATCH RFC 01/22] qemu_process: Move process code from qemu_capabilities to qemu_process

[Chris Venteicher]

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 <***@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__ */

[Chris Venteicher]

s/virQEMUCapsInitQMPCommand/qemuProcess/

No functionality change.

Use appropriate prefix in moved code.

Signed-off-by: Chris Venteicher <***@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__ */

[Michal Privoznik]

Post by Chris Venteicher
s/virQEMUCapsInitQMPCommand/qemuProcess/
No functionality change.
Use appropriate prefix in moved code.
---
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'.

Post by Chris Venteicher
+ runUid, runGid, qmperr)))
goto cleanup;

Michal

[Jiri Denemark]

Post by Chris Venteicher
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

[Chris Venteicher]

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 <***@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__ */

[Jiri Denemark]

Post by Chris Venteicher
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

[Chris Venteicher]

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 <***@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,

[Michal Privoznik]

Post by Chris Venteicher
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.
---
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;
- 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);

How about removing this label completely? I mean, s/goto ignore;/ret =
0; goto cleanup;/

Post by Chris Venteicher
ret = 0;
@@ -8226,10 +8223,6 @@ qemuProcessRun(qemuProcessPtr proc){
virObjectUnref(xmlopt);
return ret;
-
- 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.

Post by Chris Venteicher
+
qemuProcessPtr qemuProcessNew(const char *binary,
const char *libDir,
uid_t runUid,

Michal

[Jiri Denemark]

Post by Chris Venteicher
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

[Chris Venteicher]

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 <***@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);

[Michal Privoznik]

Post by Chris Venteicher
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.
---
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.

Post by Chris Venteicher
+
+/* 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.

Post by Chris Venteicher
+
+
+/**
+ *
+ * Start and connect to QEMU binary so QMP queries can be made.
+ *
+ * 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;
+
+ VIR_DEBUG("ret=%i", ret);
+ return ret;
+
+ 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

[Chris Venteicher]

Quoting Michal Privoznik (2018-11-14 09:45:07)

Post by Michal Privoznik

Post by Chris Venteicher
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.
---
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.

Post by Chris Venteicher
+
+/* 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.

Post by Michal Privoznik

Post by Chris Venteicher
+
+
+/**
+ *
+ * Start and connect to QEMU binary so QMP queries can be made.
+ *
+ * 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;
+
+ VIR_DEBUG("ret=%i", ret);
+ return ret;
+
+ 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

[Jiri Denemark]

Post by Chris Venteicher
Quoting Michal Privoznik (2018-11-14 09:45:07)

Post by Michal Privoznik

Post by Chris Venteicher
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.

...

Post by Chris Venteicher

Post by Michal Privoznik

Post by Chris Venteicher
+/* 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.

Post by Chris Venteicher
+
+/* 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

[Chris Venteicher]

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 <***@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;
}

[Chris Venteicher]

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 <***@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);

[Chris Venteicher]

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 <***@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;
}

[Jiri Denemark]

Post by Chris Venteicher
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.
---
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;
+ 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.

Post by Chris Venteicher
qemuProcessFree(proc);
return ret;

Jirka

[Chris Venteicher]

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 <***@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;
}

[Chris Venteicher]

Store libDir path in the qemuProcess struct in anticipation of moving
path construction code into qemuProcessInitQmp function.

Signed-off-by: Chris Venteicher <***@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 */

[Chris Venteicher]

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 <***@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,

[Jiri Denemark]

Post by Chris Venteicher
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

[Chris Venteicher]

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 <***@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);

[Chris Venteicher]

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 <***@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;
}

[Chris Venteicher]

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 <***@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__ */

[Michal Privoznik]

Post by Chris Venteicher
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.
---
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

[Jiri Denemark]

Post by Chris Venteicher
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.
---
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

[Chris Venteicher]

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 <***@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;

[Chris Venteicher]

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 <***@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;

[Chris Venteicher]

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 <***@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);

[Michal Privoznik]

Post by Chris Venteicher
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.
---
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).

Post by Chris Venteicher
+
+ 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

[Jiri Denemark]

Post by Chris Venteicher
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.
---
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).

Post by Chris Venteicher
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.

Post by Chris Venteicher
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,
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;
}

Post by Chris Venteicher
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.

Post by Chris Venteicher
+
proc->runUid = runUid;
proc->runGid = runGid;
proc->qmperr = qmperr;
+

Please, delete the extra empty line.

Post by Chris Venteicher
/* the ".sock" sufix is important to avoid a possible clash with a qemu
* domain called "capabilities"
*/

Jirka

[Chris Venteicher]

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 <***@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) {

[Chris Venteicher]

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 <***@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);

[Michal Privoznik]

Post by Chris Venteicher
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.
---
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;
+ 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");

Post by Chris Venteicher
+
+ 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.

Post by Chris Venteicher
goto error;
}
@@ -4350,7 +4348,6 @@ virQEMUCapsNewForBinaryInternal(virArch hostArch,
}
- VIR_FREE(qmperr);
return qemuCaps;
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 ;-)

Post by Chris Venteicher
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.

Post by Chris Venteicher
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.

[Jiri Denemark]

Post by Michal Privoznik

Post by Chris Venteicher
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.
---
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

...

Post by Michal Privoznik

Post by Chris Venteicher
@@ -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

[Jiri Denemark]

Post by Chris Venteicher
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.

Post by Chris Venteicher
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.
---
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.

Post by Chris Venteicher
{
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...

Post by Chris Venteicher
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;
+ if (proc && !proc->mon) {

... which means you wouldn't need to check for proc at this point.

Post by Chris Venteicher
+ 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.

Post by Chris Venteicher
+
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Failed to probe QEMU binary with QMP: %s"), err);
+ }

And this would report an error even if ret == 0.

Post by Chris Venteicher
+
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.

Post by Chris Venteicher
virQEMUCapsLogProbeFailure(binary);
goto error;
}
@@ -4350,7 +4348,6 @@ virQEMUCapsNewForBinaryInternal(virArch hostArch,
}
- VIR_FREE(qmperr);
return qemuCaps;

...

Post by Chris Venteicher
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;

Post by Chris Venteicher
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.

Post by Chris Venteicher
qemuProcessPtr qemuProcessNew(const char *binary,
const char *libDir,
uid_t runUid,
gid_t runGid,
- char **qmperr,
bool forceTCG);
void qemuProcessFree(qemuProcessPtr proc);

Jirka

[Chris Venteicher]

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 <***@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;

[Chris Venteicher]

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 <***@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;
}

[Chris Venteicher]

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 <***@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, &micro,
&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;

[Michal Privoznik]

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.
[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.

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.

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

[Chris Venteicher]

Quoting Michal Privoznik (2018-11-14 09:45:06)

Post by Michal Privoznik

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.
[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.

Post by Michal Privoznik

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.

Post by Michal Privoznik

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

[Michal Privoznik]

Post by Chris Venteicher
Quoting Michal Privoznik (2018-11-14 09:45:06)

Post by Michal Privoznik

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.
[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.
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.

Post by Chris Venteicher
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.

Post by Michal Privoznik

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

[Jiri Denemark]

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.
[libvirt] [PATCH v4 00/37] BaselineHypervisorCPU using QEMU QMP exchanges
https://www.redhat.com/archives/libvir-list/2018-November/msg00091.html
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

[Jiri Denemark]

Post by Chris Venteicher
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.
---
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(-)

...

Post by Chris Venteicher
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.

Post by Chris Venteicher
+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);
+}

...

Post by Chris Venteicher
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