On Tue, Mar 29, 2011 at 17:48:48 +0800, Wen Congyang wrote: > diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c > index eed83f4..647e2bb 100644 > --- a/src/qemu/qemu_monitor.c > +++ b/src/qemu/qemu_monitor.c > @@ -906,7 +906,14 @@ int qemuMonitorSetCapabilities(qemuMonitorPtr mon) > > if (mon->json) { > ret = qemuMonitorJSONSetCapabilities(mon); > - mon->json_hmp = qemuMonitorJSONCheckHMP(mon); > + if (ret == 0) { > + mon->json_hmp = qemuMonitorJSONCheckHMP(mon); > + if (mon->json_hmp < 0) { > + /* qemu may quited unexpectedly when we call > + * qemuMonitorJSONCheckHMP() */ > + ret = -1; > + } > + } This shouldn't really work since mon->json_hmp is declared as unsigned json_hmp: 1;

I think we need something like if (ret == 0) { int hmp = qemuMonitorJSONCheckHMP(mon); if (hmp < 0) { ... } else { mon->json_hmp = hmp > 0; } } Jirka

On Tue, Mar 29, 2011 at 03:22:40PM +0800, Wen Congyang wrote: > If the monitor met a error, and we will call qemuProcessHandleMonitorEOF(). > But we may try to send monitor command after qemuProcessHandleMonitorEOF() > returned. Then libvirtd will be blocked in qemuMonitorSend(). > > Steps to reproduce this bug: > 1. use gdb to attach libvirtd, and set a breakpoint in the function > qemuConnectMonitor() > 2. start a vm > 3. let the libvirtd to run until qemuMonitorOpen() returns. > 4. kill the qemu process > 5. continue running libvirtd > > Signed-off-by: Wen Congyang <wency(a)cn.fujitsu.com&gt; > > --- > src/qemu/qemu_monitor.c | 13 +++++++++++++ > 1 files changed, 13 insertions(+), 0 deletions(-) > > diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c > index 800f744..eed83f4 100644 > --- a/src/qemu/qemu_monitor.c > +++ b/src/qemu/qemu_monitor.c > @@ -572,6 +572,13 @@ qemuMonitorIO(int watch, int fd, int events, void *opaque) { > mon->msg->lastErrno = EIO; > virCondSignal(&mon->notify); > } > + /* If qemu quited unexpectedly, and we may try to send monitor > + * command later. But we have no chance to wake up it. So set > + * mon->lastErrno to EIO, and check it before sending monitor > + * command. > + */ > + if (!mon->lastErrno) > + mon->lastErrno = EIO; > quit = 1; > } else if (events) { > VIR_ERROR(_("unhandled fd event %d for monitor fd %d"), I'm wondering if we should just move this safety check a little further down in this method, because the 'else if (events)' branch likely wants this check too. eg I'd put it in the bit where we run the EOF callback something like this: /* We have to unlock to avoid deadlock against command thread, * but is this safe ? I think it is, because the callback * will try to acquire the virDomainObjPtr mutex next */ if (failed || quit) { void (*eofNotify)(qemuMonitorPtr, virDomainObjPtr, int) = mon->cb->eofNotify; virDomainObjPtr vm = mon->vm; + /* If qemu quited unexpectedly, and we may try to send monitor + * command later. But we have no chance to wake up it. So set + * mon->lastErrno to EIO, and check it before sending monitor + * command. + */ + if (!mon->lastErrno) + mon->lastErrno = EIO;

/* Make sure anyone waiting wakes up now */ virCondSignal(&mon->notify); if (qemuMonitorUnref(mon) > 0) qemuMonitorUnlock(mon); VIR_DEBUG("Triggering EOF callback error? %d", failed); (eofNotify)(mon, vm, failed); > @@ -725,6 +732,12 @@ int qemuMonitorSend(qemuMonitorPtr mon, > { > int ret = -1; > > + /* Check whether qemu quited unexpectedly */ > + if (mon->lastErrno) { > + msg->lastErrno = mon->lastErrno; > + return -1; > + } > + > mon->msg = msg; > qemuMonitorUpdateWatch(mon); Daniel