Re: [libvirt] [PATCH v5 15/36] qemu_process: Don't open monitor if process failed
On Sun, Dec 02, 2018 at 23:10:09 -0600, Chris Venteicher wrote:
Gracefully handle case when proc activation failed prior to calling.
Consistent with the existing code for qemuConnectMonitor (for domains)
in qemu_process.c...
- Handle qemMonitorOpen failure with INFO message and NULL ptr
- Identify parameters passed to qemuMonitorOpen
Monitor callbacks will be removed in future patch so we prep for passing
NULL for the callback pointer.
Set proc->mon to NULL then use VIR_STEAL_PTR if successful to be
consistent with other functions.
Signed-off-by: Chris Venteicher <cventeic(a)redhat.com>
---
src/qemu/qemu_process.c | 29 +++++++++++++++++++++++++++--
1 file changed, 27 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 8906a22e3c..31d41688fe 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -8262,25 +8262,50 @@ static int
qemuProcessQmpConnectMonitor(qemuProcessQmpPtr 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) {
+ ret = 0;
+ goto cleanup;
+ }
This is a dead code. We'd just see a segfault much earlier than at this
point if proc == NULL. For example, two lines above in the debug
message. But anyway, this function will just never be called if proc ==
NULL. Similarly proc->pid == 0 only when starting the process failed in
which case this function will not (and should not) be called.
+
+ 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);
This just hides the values we pass to qemuMonitorOpen. I agree that the
previous state is not ideal, but checking the prototype of
qemuMonitorOpen is easy. On the other hand one would have to scroll up
and down several times to see what values we pass here.
+
+ if (!mon) {
+ VIR_INFO("Failed to connect monitor to emulator %s", proc->binary);
qemuMonitorOpen already reported an error if it failed for any reason so
logging any further message with info priority makes no sense.
goto cleanup;
+ }
+
+ VIR_STEAL_PTR(proc->mon, mon);
This is useful when we want to share success and error paths and just
free, cleanup, or do something similar with mon only in case of error.
But there's no such thing here and even none of the later patches
introduces anything like that.
virObjectLock(proc->mon);
That said I don't see any reason for keeping this patch. Please, drop
it.
Jirka