[libvirt] 'make check' hangs

[Daniel P. Berrange]

On Wed, Nov 30, 2011 at 11:48:16AM +0100, Jiri Denemark wrote:
> 
> Hmm, I suspect this may be caused by
> 
> commit fd7e85ac6af833845aa0eb2526158c319800a0ae
> Author: Jiri Denemark <jdenemar at redhat.com>
> Date:   Tue Oct 11 15:05:52 2011 +0200
> 
>     virsh: Always run event loop
> 
>     Since virsh already implements event loop, it has to also run it. So far
>     the event loop was only running during virsh console command.
> 
> However, the commit added a special hack to prevent this from happening:
> 
>     @@ -17080,6 +17101,16 @@ vshDeinit(vshControl *ctl)
>          }
>          virResetLastError();
> 
>     +    if (ctl->eventLoopStarted) {
>     +        /* HACK: Add a dummy timeout to break event loop */
>     +        int timer = virEventAddTimeout(-1, NULL, NULL, NULL);
>     +        if (timer != -1)
>     +            virEventRemoveTimeout(timer);
>     +
>     +        virThreadJoin(&ctl->eventLoop);
>     +        ctl->eventLoopStarted = false;
>     +    }
>     +
>          return true;

I think the problem may lie a little bit further up

static bool
vshDeinit(vshControl *ctl)
{
    ctl->quit = true;
    vshReadlineDeinit(ctl);
    vshCloseLogFile(ctl);
    VIR_FREE(ctl->name);
    if (ctl->conn) {
        int ret;
        if ((ret = virConnectClose(ctl->conn)) != 0) {
            vshError(ctl, _("Failed to disconnect from the hypervisor, %d leaked reference(s)"), ret);
        }
    }
    virResetLastError();

    if (ctl->eventLoopStarted) {
        /* HACK: Add a dummy timeout to break event loop */
        int timer = virEventAddTimeout(-1, NULL, NULL, NULL);


Now look at the event loop thread:

static void
vshEventLoop(void *opaque)
{
    vshControl *ctl = opaque;

    while (!ctl->quit) {
        if (virEventRunDefaultImpl() < 0) {
            virshReportError(ctl);
        }
    }
}

Neither the read of ctl->quit, nor the write of ctl->quit are protected
by any locking. While you have assigned to ctl->quit before adding the
dummy function, I'm not convinced that is sufficient, because in the
absence of any thread synchronization point, the compiler is free to
reorder your assignment to occur later. In addition, the data is not
neccessarily coherant across threads. IMHO, the read/write of ctl->quit
needs to be protected by a mutex.

Regrads,
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 :|