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

[Marc-André Lureau]

Hi

On Tue, Aug 27, 2019 at 3:37 PM Johannes Berg <address@hidden> 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
> > <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?

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