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

[Frediano Ziglio]

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

This is a point for Windows... they manage to have unloading working.
Also you can unload Linux kernel modules.
I honestly find these reasoning a lazy excuse to bad programming and design.

Frediano