Hi On Tue, Aug 27, 2019 at 3:37 PM Johannes Berg <johan...@sipsolutions.net> wrote: > > 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 > > <johan...@sipsolutions.net> wrote: > > > From: Johannes Berg <johannes.b...@intel.com> > > > > > > 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?
That's helpful, thanks > > > > @@ -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. What other problems? Sure we need the caller to unref. > > > 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? The vug_source_new() function. > > 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). Using the hashtable shouldn't be necessary, as VugDev will handle the attach/detach of the socket FD. The hashtable is meant for VuDev callback. Sure we can add the socket to the hashtable, but it's better to avoid since we can. -- Marc-André Lureau
