[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] do not send monitor command after monitor met error



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 cn fujitsu com>
>>
>> ---
>>  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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]