Re: [Qemu-devel] [PATCH] libvhost-user-glib: fix VugDev main fd cleanup

[Johannes Berg]

On Tue, 2019-08-27 at 14:47 +0400, Marc-André Lureau wrote:
> Hi
> 
> On Tue, Aug 27, 2019 at 12:32 PM Johannes Berg
> <address@hidden> wrote:
> > From: Johannes Berg <address@hidden>
> > 
> > If you try to make a device implementation that can handle multiple
> > connections and allow disconnections (which requires overriding the
> > VHOST_USER_NONE handling), then glib will warn that we remove a src
> > while it's still on the mainloop, and will poll() an FD that doesn't
> > exist anymore.
> > 
> > Fix this by just using the internal add_watch() function that has
> > all necessary cleanups built in via the hashtable, rather than
> > treating the "main" fd of a device specially.
> 
> It would be easier to see a backtrace of the error (under gdb with
> G_DEBUG=fatal_criticals)

fatal-warnings, but sure:

Program received signal SIGTRAP, Trace/breakpoint trap.
0x00007ffff7cc6885 in _g_log_abort (breakpoint=1) at 
../../../glib/gmessages.c:554
554     ../../../glib/gmessages.c: No such file or directory.
(gdb) bt
#0  0x00007ffff7cc6885 in _g_log_abort (breakpoint=1) at 
../../../glib/gmessages.c:554
#1  0x00007ffff7cc7b8d in g_logv (log_domain=0x7ffff7d0d00e "GLib", 
log_level=G_LOG_LEVEL_WARNING, format=<optimized out>, 
    args=args@entry=0x7fffffffe010) at ../../../glib/gmessages.c:1371
#2  0x00007ffff7cc7d5f in g_log (log_domain=log_domain@entry=0x7ffff7d0d00e 
"GLib", 
    log_level=log_level@entry=G_LOG_LEVEL_WARNING, 
    format=format@entry=0x7ffff7d150f8 "../../../glib/gmain.c:2116: ref_count 
== 0, but source was still attached to a context!") at 
../../../glib/gmessages.c:1413
#3  0x00007ffff7cbda4a in g_source_unref_internal (source=0x55555557b870, 
context=0x555555579120, have_lock=1)
    at ../../../glib/gmain.c:2116
#4  0x00007ffff7cc09a8 in g_main_dispatch (context=0x555555579120) at 
../../../glib/gmain.c:3217
#5  g_main_context_dispatch (context=context@entry=0x555555579120) at 
../../../glib/gmain.c:3854
#6  0x00007ffff7cc0c88 in g_main_context_iterate (context=0x555555579120, 
block=block@entry=1, dispatch=dispatch@entry=1, 
    self=<optimized out>) at ../../../glib/gmain.c:3927
#7  0x00007ffff7cc0f82 in g_main_loop_run (loop=0x55555557a300) at 
../../../glib/gmain.c:4123
[...]

Doesn't really help (me) much as the cause of the error is much earlier?

> > @@ -157,5 +157,4 @@ vug_deinit(VugDev *dev)
> >      g_assert(dev);
> > 
> >      g_hash_table_unref(dev->fdmap);
> > -    g_source_unref(dev->src);
> 
> I think the main problem here is that src is not owned, since
> vug_source_new() does g_source_unref(). This is looks unfortunate.

I thought so too, but removing that g_source_unref() causes other
problems.

> However, the source should be destroyed (detached from the main
> context). I think this is ultimately what your change is about.

Yes, it just makes it use the same path as all the other FDs/sources.

> Imho, we should change the behaviour of the function to return a ref
> source. 

Which "the function" do you mean?

I'm not really sure I understand what you're actually saying about
my patch though. Are you saying I shouldn't do this? But then I don't
really understand why. Why should the "main" FD be different from any of
the VQ FDs, and not just all go into the hashtable? I basically arrived
at this patch by noting that the other FDs were OK (the warning only
occurs for this one), and the cleanup for the others is handled
correctly while destroying the hashtable. Having to clean this
particular one up manually seemed pointless (though I couldn't even make
it work correctly).

johannes