At 03/29/2011 05:59 PM, Daniel P. Berrange Write:
> 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@xxxxxxxxxxxxxx>
>>
>> ---
>> 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;
Yes, setting mon->lastErrno here is better.
Pushed with this fixed. Thanks.
>
> /* 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list