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

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

[Johannes Berg]

From: Johannes Berg <johannes.berg@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.

Fixes: 8bb7ddb78a1c ("libvhost-user: add glib source helper")
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 contrib/libvhost-user/libvhost-user-glib.c | 3 +--
 contrib/libvhost-user/libvhost-user-glib.h | 1 -
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/contrib/libvhost-user/libvhost-user-glib.c b/contrib/libvhost-user/libvhost-user-glib.c
index 99edd2f3de45..a092a55c1d57 100644
--- a/contrib/libvhost-user/libvhost-user-glib.c
+++ b/contrib/libvhost-user/libvhost-user-glib.c
@@ -146,7 +146,7 @@ vug_init(VugDev *dev, uint16_t max_queues, int socket,
     dev->fdmap = g_hash_table_new_full(NULL, NULL, NULL,
                                        (GDestroyNotify) g_source_destroy);
 
-    dev->src = vug_source_new(dev, socket, G_IO_IN, vug_watch, NULL);
+    set_watch(&dev->parent, socket, VU_WATCH_IN, vug_watch, NULL);
 
     return true;
 }
@@ -157,5 +157,4 @@ vug_deinit(VugDev *dev)
     g_assert(dev);
 
     g_hash_table_unref(dev->fdmap);
-    g_source_unref(dev->src);
 }
diff --git a/contrib/libvhost-user/libvhost-user-glib.h b/contrib/libvhost-user/libvhost-user-glib.h
index 64d539d93aba..32a6ec6df063 100644
--- a/contrib/libvhost-user/libvhost-user-glib.h
+++ b/contrib/libvhost-user/libvhost-user-glib.h
@@ -22,7 +22,6 @@ typedef struct VugDev {
     VuDev parent;
 
     GHashTable *fdmap; /* fd -> gsource */
-    GSource *src;
 } VugDev;
 
 bool vug_init(VugDev *dev, uint16_t max_queues, int socket,
-- 
2.23.0

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

[Marc-André Lureau]

Hi

On Tue, Aug 27, 2019 at 12:32 PM Johannes Berg
<johannes@sipsolutions.net> wrote:
>
> From: Johannes Berg <johannes.berg@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)

>
> Fixes: 8bb7ddb78a1c ("libvhost-user: add glib source helper")
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> ---
>  contrib/libvhost-user/libvhost-user-glib.c | 3 +--
>  contrib/libvhost-user/libvhost-user-glib.h | 1 -
>  2 files changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/contrib/libvhost-user/libvhost-user-glib.c b/contrib/libvhost-user/libvhost-user-glib.c
> index 99edd2f3de45..a092a55c1d57 100644
> --- a/contrib/libvhost-user/libvhost-user-glib.c
> +++ b/contrib/libvhost-user/libvhost-user-glib.c
> @@ -146,7 +146,7 @@ vug_init(VugDev *dev, uint16_t max_queues, int socket,
>      dev->fdmap = g_hash_table_new_full(NULL, NULL, NULL,
>                                         (GDestroyNotify) g_source_destroy);
>
> -    dev->src = vug_source_new(dev, socket, G_IO_IN, vug_watch, NULL);
> +    set_watch(&dev->parent, socket, VU_WATCH_IN, vug_watch, NULL);
>
>      return true;
>  }
> @@ -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.

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

Imho, we should change the behaviour of the function to return a ref
source. This needs fixing the hashtable destroy callback. It will be
more consistent with other usages of the functions that also need
fixing in contrib/vhost-user-input/main.c for example.


>  }
> diff --git a/contrib/libvhost-user/libvhost-user-glib.h b/contrib/libvhost-user/libvhost-user-glib.h
> index 64d539d93aba..32a6ec6df063 100644
> --- a/contrib/libvhost-user/libvhost-user-glib.h
> +++ b/contrib/libvhost-user/libvhost-user-glib.h
> @@ -22,7 +22,6 @@ typedef struct VugDev {
>      VuDev parent;
>
>      GHashTable *fdmap; /* fd -> gsource */
> -    GSource *src;
>  } VugDev;
>
>  bool vug_init(VugDev *dev, uint16_t max_queues, int socket,
> --
> 2.23.0
>
>


-- 
Marc-André Lureau

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
> <johannes@sipsolutions.net> wrote:
> > From: Johannes Berg <johannes.berg@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?

> > @@ -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

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 <johannes@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
> > <johannes@sipsolutions.net> wrote:
> > > From: Johannes Berg <johannes.berg@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

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

[Johannes Berg]

Hi,

> What other problems? Sure we need the caller to unref.

Don't recall, and now I can't reproduce it, sorry.

> > > 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.
[...]
> Sure we can add the socket to the hashtable, but it's better to avoid
> since we can.

Ok, that works. Does this seem better to you?

diff --git a/contrib/libvhost-user/libvhost-user-glib.c b/contrib/libvhost-user/libvhost-user-glib.c
index 99edd2f3de45..3e902ccd2efb 100644
--- a/contrib/libvhost-user/libvhost-user-glib.c
+++ b/contrib/libvhost-user/libvhost-user-glib.c
@@ -91,7 +91,6 @@ vug_source_new(VugDev *gdev, int fd, GIOCondition cond,
     g_source_add_poll(gsrc, &src->gfd);
     id = g_source_attach(gsrc, NULL);
     g_assert(id);
-    g_source_unref(gsrc);
 
     return gsrc;
 }
@@ -131,6 +130,15 @@ static void vug_watch(VuDev *dev, int condition, void *data)
     }
 }
 
+void vug_source_destroy(GSource *src)
+{
+    if (!src)
+        return;
+
+    g_source_unref(src);
+    g_source_destroy(src);
+}
+
 bool
 vug_init(VugDev *dev, uint16_t max_queues, int socket,
          vu_panic_cb panic, const VuDevIface *iface)
@@ -144,7 +152,7 @@ vug_init(VugDev *dev, uint16_t max_queues, int socket,
     }
 
     dev->fdmap = g_hash_table_new_full(NULL, NULL, NULL,
-                                       (GDestroyNotify) g_source_destroy);
+                                       (GDestroyNotify) vug_source_destroy);
 
     dev->src = vug_source_new(dev, socket, G_IO_IN, vug_watch, NULL);
 
@@ -157,5 +165,5 @@ vug_deinit(VugDev *dev)
     g_assert(dev);
 
     g_hash_table_unref(dev->fdmap);
-    g_source_unref(dev->src);
+    vug_source_destroy(dev->src);
 }
diff --git a/contrib/libvhost-user/libvhost-user-glib.h b/contrib/libvhost-user/libvhost-user-glib.h
index 64d539d93aba..1a79a4916ef2 100644
--- a/contrib/libvhost-user/libvhost-user-glib.h
+++ b/contrib/libvhost-user/libvhost-user-glib.h
@@ -31,5 +31,6 @@ void vug_deinit(VugDev *dev);
 
 GSource *vug_source_new(VugDev *dev, int fd, GIOCondition cond,
                         vu_watch_cb vu_cb, gpointer data);
+void vug_source_destroy(GSource *src);
 
 #endif /* LIBVHOST_USER_GLIB_H */
diff --git a/contrib/vhost-user-input/main.c b/contrib/vhost-user-input/main.c
index 449fd2171a5a..7d6b0f9d80cc 100644
--- a/contrib/vhost-user-input/main.c
+++ b/contrib/vhost-user-input/main.c
@@ -187,7 +187,7 @@ vi_queue_set_started(VuDev *dev, int qidx, bool started)
     }
 
     if (!started && vi->evsrc) {
-        g_source_destroy(vi->evsrc);
+        vug_source_destroy(vi->evsrc);
         vi->evsrc = NULL;
     }
 }
@@ -401,9 +401,7 @@ main(int argc, char *argv[])
 
     vug_deinit(&vi.dev);
 
-    if (vi.evsrc) {
-        g_source_unref(vi.evsrc);
-    }
+    vugg_source_destroy(vi.evsrc);
     g_array_free(vi.config, TRUE);
     g_free(vi.queue);
     return 0;