On 11/11/2018 08:59 PM, Chris Venteicher wrote:
Failure to connect to QEMU to probe capabilities is not considered a error case and does not result in a negative return value.
Check for a NULL monitor connection pointer before trying to send capabilities commands to QEMU rather than depending on a special positive return value.
This simplifies logic and is more consistent with the operation of existing qemu_process functions.
A macro is introduced to easily obtain the monitor pointer from the qemuProcess structure.
Signed-off-by: Chris Venteicher <cventeic@redhat.com> --- src/qemu/qemu_capabilities.c | 28 ++++++++++++++++++---------- src/qemu/qemu_process.c | 9 +-------- src/qemu/qemu_process.h | 3 +++ 3 files changed, 22 insertions(+), 18 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index f5e327097e..fbb4336201 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -4220,43 +4220,51 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, { qemuProcessPtr proc = NULL; qemuProcessPtr proc_kvm = NULL; + qemuMonitorPtr mon = NULL; + qemuMonitorPtr mon_kvm = NULL; int ret = -1; - int rc; bool forceTCG = false;
if (!(proc = qemuProcessNew(qemuCaps->binary, libDir, runUid, runGid, forceTCG))) goto cleanup;
- if ((rc = qemuProcessRun(proc)) != 0) { - if (rc == 1) - ret = 0; + + if (qemuProcessRun(proc) < 0) + goto cleanup; + + if (!(mon = QEMU_PROCESS_MONITOR(proc))) { + ret = 0; /* Failure probing QEMU not considered error case */ goto cleanup; }
- if (virQEMUCapsInitQMPMonitor(qemuCaps, proc->mon) < 0) + /* Pull capabilities from QEMU */ + if (virQEMUCapsInitQMPMonitor(qemuCaps, mon) < 0) goto cleanup;
+ /* Pull capabilities again if KVM supported */ if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM)) { qemuProcessStopQmp(proc);
forceTCG = true; proc_kvm = qemuProcessNew(qemuCaps->binary, libDir, runUid, runGid, forceTCG);
- if ((rc = qemuProcessRun(proc_kvm)) != 0) { - if (rc == 1) - ret = 0; + if (qemuProcessRun(proc_kvm) < 0) + goto cleanup; + + if (!(mon_kvm = QEMU_PROCESS_MONITOR(proc_kvm))) { + ret = 0; /* Failure probing QEMU not considered error case */ goto cleanup; }
- if (virQEMUCapsInitQMPMonitorTCG(qemuCaps, proc_kvm->mon) < 0) + if (virQEMUCapsInitQMPMonitorTCG(qemuCaps, mon_kvm) < 0) goto cleanup; }
ret = 0;
cleanup: - if (proc && !proc->mon) { + if (!mon) { char *err = QEMU_PROCESS_ERROR(proc) ? QEMU_PROCESS_ERROR(proc) : _("unknown error");
virReportError(VIR_ERR_INTERNAL_ERROR, diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index a741d1cf91..2640ec2b32 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8148,10 +8148,6 @@ qemuProcessNew(const char *binary, }
-/* Returns -1 on fatal error, - * 0 on success, - * 1 when probing QEMU failed - */ int qemuProcessRun(qemuProcessPtr proc){ virDomainXMLOptionPtr xmlopt = NULL; @@ -8218,6 +8214,7 @@ qemuProcessRun(qemuProcessPtr proc){
virObjectLock(proc->mon);
+ ignore:
How about removing this label completely? I mean, s/goto ignore;/ret = 0; goto cleanup;/
ret = 0;
cleanup: @@ -8226,10 +8223,6 @@ qemuProcessRun(qemuProcessPtr proc){ virObjectUnref(xmlopt);
return ret; - - ignore: - ret = 1; - goto cleanup; }
diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h index abd80baf5c..37194e2501 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -235,6 +235,9 @@ struct _qemuProcess { #define QEMU_PROCESS_ERROR(proc) \ ((char *) (proc ? proc->qmperr: NULL))
+#define QEMU_PROCESS_MONITOR(proc) \ + ((qemuMonitorPtr) (proc ? proc->mon : NULL))
This looks like an overkill to me. At the only two points where this is used @proc/@proc_kvm can't be NULL so proc->mon/proc_kvm->mon can be accessed directly.
+ qemuProcessPtr qemuProcessNew(const char *binary, const char *libDir, uid_t runUid,
Michal