[Spice-devel] [PATCH spice-gtk] Add call of gst_deinit at program exit

[Daniel P. Berrange]

On Thu, Oct 19, 2017 at 07:47:24AM -0400, Frediano Ziglio wrote:
> > > On 19 Oct 2017, at 12:32, Frediano Ziglio < fziglio at redhat.com > wrote:
> > 
> 
> > > > From: Christophe de Dinechin < dinechin at redhat.com >
> > > 
> > 
> 
> > > > This is useful for some instrumentation, e.g. the leaks tracer,
> > > 
> > 
> > > > that perform some of their operations within gst_deinit.
> > > 
> > 
> 
> > > > Without this patch, if you run spicy with
> > > 
> > 
> > > > GST_DEBUG="GST_TRACER:7" GST_TRACERS="leaks" spicy ...
> > > 
> > 
> > > > the leak tracer does not run at exit, because it runs in gst_deinit.
> > > 
> > 
> 
> > > > Signed-off-by: Christophe de Dinechin < dinechin at redhat.com >
> > > 
> > 
> > > > ---
> > > 
> > 
> > > > spice-common | 2 +-
> > > 
> > 
> > > > src/channel-display-gst.c | 1 +
> > > 
> > 
> > > > 2 files changed, 2 insertions(+), 1 deletion(-)
> > > 
> > 
> 
> > > > diff --git a/spice-common b/spice-common
> > > 
> > 
> > > > index 429ad96..ba11de3 160000
> > > 
> > 
> > > > --- a/spice-common
> > > 
> > 
> > > > +++ b/spice-common
> > > 
> > 
> > > > @@ -1 +1 @@
> > > 
> > 
> > > > -Subproject commit 429ad965371ceaaf60b81ccbed7da660ef9e0a94
> > > 
> > 
> > > > +Subproject commit ba11de3f3fd58d1b1a99bb62dd9e409e9961a78e
> > > 
> > 
> > > > diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> > > 
> > 
> > > > index f978602..c9ab9bf 100644
> > > 
> > 
> > > > --- a/src/channel-display-gst.c
> > > 
> > 
> > > > +++ b/src/channel-display-gst.c
> > > 
> > 
> > > > @@ -578,6 +578,7 @@ static gboolean gstvideo_init(void)
> > > 
> > 
> > > > GError *err = NULL;
> > > 
> > 
> > > > if (gst_init_check(NULL, NULL, &err)) {
> > > 
> > 
> > > > success = 1;
> > > 
> > 
> > > > + atexit(gst_deinit);
> > > 
> > 
> > > > } else {
> > > 
> > 
> > > > spice_warning("Disabling GStreamer video support: %s",
> > > 
> > 
> > > > err->message);
> > > 
> > 
> > > > g_clear_error(&err);
> > > 
> > 
> 
> > > Calling atexit from a library is a bad idea.
> > 
> 
> > Could you elaborate?
> 
> > I do not really agree with this statement. I’d actually go as far as saying
> > that libraries are the reason atexit exists to start with.
> > Apparently, I’m not alone, see first three responses in
> > https://stackoverflow.com/questions/25115612/whats-the-scenario-to-use-atexit-function
> > that all mention libraries.
> 
> > Christophe
> 
> Shared libraries in theory can be unloaded before the atexit function is called for instance. 
> 
> Calling gst_deinit from a library is a bad idea because other part of the program could use 
> gstreamer too and call other gstreamer functions. Even after your atexit function! 
> 
> The gcc way to catch shared library unload is to use the destructor attribute
> which in Unix usually chain the .deinit/deinit function. Also has the
> advantage of not using space (atexit function calls are usually limited as in
> many systems the buffer used to register  them is static).

In fact non-trivial shared libraries should generally never be unloaded, even
if they were originally dlopend.  If the library has used a pthread local with
a destructor function, then unloading the library will remove the code that
contains the impl of the destructor. When the thread later exits and its thread
locals are cleaned up, the application will crash & burn.

Many libraries, including libvirt, will link with '-z nodelete' to prevent
unloading of the library even if dlclose() is called, to avoid these kind
of crashes.

IOW getting perfect "cleanup" is just a fools errand and will likely create
obscure problems down the road that are worse than the problems the cleanup
is trying to solve. Just accept normal process resource cleanup when the
application exits.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|