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