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

[Daniel P. Berrange]

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 at 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;

        /* 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
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|