Re: [libvirt] [PATCH v2 01/31] qemu: Make QMP probing process reusable
On Thu, Nov 24, 2016 at 14:54:48 +0100, Pavel Hrdina wrote:
On Mon, Nov 21, 2016 at 12:20:57AM +0100, Jiri Denemark wrote:
> The code that runs a new QEMU process to be used for probing
> capabilities is separated into four reusable functions so that any code
> that wants to probe a QEMU process may just follow a few simple steps:
>
> cmd = virQEMUCapsInitQMPCommandNew(...);
>
> mon = virQEMUCapsInitQMPCommandRun(cmd, ...);
>
> /* talk to the running QEMU process using its QMP monitor */
>
> if (reprobeIsRequired) {
> virQEMUCapsInitQMPCommandAbort(cmd, ...);
> mon = virQEMUCapsInitQMPCommandRun(cmd, ...);
>
> /* talk to the running QEMU process again */
> }
>
> virQEMUCapsInitQMPCommandFree(cmd);
>
> Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com>
> ---
> src/qemu/qemu_capabilities.c | 259 +++++++++++++++++++++++++++++--------------
> 1 file changed, 174 insertions(+), 85 deletions(-)
>
> +static qemuMonitorPtr
> +virQEMUCapsInitQMPCommandRun(virQEMUCapsInitQMPCommandPtr cmd,
> + bool *qemuFailed)
That *bool* is not necessary, function virQEMUCapsInitQMPCommandRun can return
simple *int* because qemuMonitorPtr is stored in *cmd*. So this function can
return -1 in case of fatal error. In case return is 0, following code can
easily depend on whether cmd->mon is set or not.
Indeed, although I think the function should rather return -1, 0, 1
instead of forcing the code to do magic based on cmd->mon.
Otherwise this patch looks good, but I think that it would be better
to send a
new version with the suggested change.
Sure, since the patch is quite large and not exactly easy to read, the
following is the diff I will squash into v2.
Jirka
diff --git i/src/qemu/qemu_capabilities.c w/src/qemu/qemu_capabilities.c
index 1257bb0ff..d6182ccb7 100644
--- i/src/qemu/qemu_capabilities.c
+++ w/src/qemu/qemu_capabilities.c
@@ -4143,12 +4143,16 @@ virQEMUCapsInitQMPCommandNew(char *binary,
}
-static qemuMonitorPtr
-virQEMUCapsInitQMPCommandRun(virQEMUCapsInitQMPCommandPtr cmd,
- bool *qemuFailed)
+/* Returns -1 on fatal error,
+ * 0 on success,
+ * 1 when probing QEMU failed
+ */
+static int
+virQEMUCapsInitQMPCommandRun(virQEMUCapsInitQMPCommandPtr cmd)
{
virDomainXMLOptionPtr xmlopt = NULL;
int status = 0;
+ int ret = -1;
VIR_DEBUG("Try to probe capabilities of '%s' via QMP",
cmd->binary);
@@ -4203,15 +4207,17 @@ virQEMUCapsInitQMPCommandRun(virQEMUCapsInitQMPCommandPtr cmd,
virObjectLock(cmd->mon);
+ ret = 0;
+
cleanup:
if (!cmd->mon)
virQEMUCapsInitQMPCommandAbort(cmd);
virObjectUnref(xmlopt);
- return cmd->mon;
+ return ret;
ignore:
- *qemuFailed = true;
+ ret = 1;
goto cleanup;
}
@@ -4224,21 +4230,20 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps,
char **qmperr)
{
virQEMUCapsInitQMPCommandPtr cmd = NULL;
- qemuMonitorPtr mon = NULL;
- bool qemuFailed = false;
int ret = -1;
+ int rc;
if (!(cmd = virQEMUCapsInitQMPCommandNew(qemuCaps->binary, libDir,
runUid, runGid, qmperr)))
goto cleanup;
- if (!(mon = virQEMUCapsInitQMPCommandRun(cmd, &qemuFailed))) {
- if (qemuFailed)
+ if ((rc = virQEMUCapsInitQMPCommandRun(cmd)) != 0) {
+ if (rc == 1)
ret = 0;
goto cleanup;
}
- if (virQEMUCapsInitQMPMonitor(qemuCaps, mon) < 0)
+ if (virQEMUCapsInitQMPMonitor(qemuCaps, cmd->mon) < 0)
goto cleanup;
ret = 0;