Re: [Spice-devel] [PATCH spice-gtk] Add call of gst_deinit at program exit
> On 19 Oct 2017, at 15:10, Daniel P. Berrange wrote: > > On Thu, Oct 19, 2017 at 03:01:17PM +0200, Christophe de Dinechin wrote: >>> One reason is that you may use a library dynamically, and you may dlclose() >>> it, and then atexit() will likely crash. >> >> Is that a real or theoretical scenario? Who loads this library dynamically >> currently? > > Anyone using SPICE from a non-C language will have dlopen'd it and potentially > later dlclose it. I’m actually quite curious how this could possibly fly today, given what I learned recently about how we deal with gstreamer objects. Closing the library seems a bit difficult. There isn’t even a header for that library. That being said, there is this in spicy.c: #if HAVE_GSTAUDIO || HAVE_GSTVIDEO gst_init(&argc, &argv); #endif So I guess it makes sense to put the gst_deinit there. Ugly #ifdefs included. I’ll do a patch with this approach instead. In that case, do we really need calls to gst_init_check() in the “library"? > > 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 :| ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-gtk] Add call of gst_deinit at program exit
> On 19 Oct 2017, at 15:10, Daniel P. Berrange wrote: > > On Thu, Oct 19, 2017 at 03:01:17PM +0200, Christophe de Dinechin wrote: >>> One reason is that you may use a library dynamically, and you may dlclose() >>> it, and then atexit() will likely crash. >> >> Is that a real or theoretical scenario? Who loads this library dynamically >> currently? > > Anyone using SPICE from a non-C language will have dlopen'd it and potentially > later dlclose it. Note that here, the symbol passed to atexit is in gstreamer, not in spice. > > 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 :| ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-gtk] Add call of gst_deinit at program exit
On Thu, Oct 19, 2017 at 03:01:17PM +0200, Christophe de Dinechin wrote: > > One reason is that you may use a library dynamically, and you may dlclose() > > it, and then atexit() will likely crash. > > Is that a real or theoretical scenario? Who loads this library dynamically > currently? Anyone using SPICE from a non-C language will have dlopen'd it and potentially later dlclose it. 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 :| ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-gtk] Add call of gst_deinit at program exit
> > On 19 Oct 2017, at 13:49, Marc-André Lureau < marcandre.lur...@redhat.com >
> > wrote:
>
> > - Original Message -
>
> > > > On 19 Oct 2017, at 13:15, Marc-André Lureau <
> > > > marcandre.lur...@redhat.com
> > > > >
> > >
> >
>
> > > > wrote:
> > >
> >
>
> > > > Hi
> > >
> >
>
> > > > - Original Message -
> > >
> >
>
> > > > > > From: Christophe de Dinechin < dinec...@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 < dinec...@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 16
> > > > >
> > > >
> > >
> >
>
> > > > > > --- a/spice-common
> > > > >
> > > >
> > >
> >
>
> > > > > > +++ b/spice-common
> > > > >
> > > >
> > >
> >
>
> > > > > > @@ -1 +1 @@
> > > > >
> > > >
> > >
> >
>
> > > > > > -Subproject commit 429ad965371ceaaf60b81ccbed7da660ef9e0a94
> > > > >
> > > >
> > >
> >
>
> > > > > > +Subproject commit ba11de3f3fd58d1b1a99bb62dd9e409e9961a78e
> > > > >
> > > >
> > >
> >
>
> > probably a mistake
>
> Yes.
> > > > > > 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.
> > > >
> > >
> >
>
> > > > And calling gst_deinit() from a library is also wrong.
> > >
> >
>
> > > Why? What would be a better way?
> >
>
> > Not calling it:
>
> But as pointed in the commit log, this means that some gstreamer tools
> that rely on gst_deinit being called are broken for all spice clients.
> > /**
>
> > * gst_deinit:
>
> > *
>
> > * Clean up any resources created by GStreamer in gst_init().
>
> > *
>
> > * It is normally not needed to call this function in a normal application
>
> > * as the resources will automatically be freed when the program terminates.
>
> > * This function is therefore mostly used by testsuites and other memory
>
> > * profiling tools.
>
> > *
>
> > * After this call GStreamer (including this method) should not be used
> > anymore.
>
> > */
>
> > spice-gtk is a library, that happen to use gstreamer.
>
> If this library did init gstreamer, it is responsible to deinit it.
> Just like if it opens a file or allocates memory, it’s responsible to close
> it or deallocate it.
> > If something else in the client app is also using gstreamer this will cause
> > troubles.
>
> And if someone unloads our library presently, it leaks.
> > Well calling atexit() should be fine, even if called multiple times, Why
> > it's
> > not done in gstreamer in this case? Why would every application have to add
> > gst_deinit() themself?
>
> Well, the gstreamer init / deinit design reeks of big globals lurking
> somewhere anyway.
> So if you ask me, that interface should have been more like:
> gst_id id = gst_init();
> gst_deinit(id);
> That would have made it very clear that you could init multiple times and
> that each caller
> was responsible for calling deinit. But of course, that would have also meant
> passing the
> id around all the time. Much less convenient.
Or you could use a static counter incremented by gst_init and decreme
Re: [Spice-devel] [PATCH spice-gtk] Add call of gst_deinit at program exit
> On 19 Oct 2017, at 13:49, Marc-André Lureau
> wrote:
>
>
>
> - Original Message -
>>
>>> On 19 Oct 2017, at 13:15, Marc-André Lureau
>>> wrote:
>>>
>>> Hi
>>>
>>> - Original Message -
>
> From: Christophe de Dinechin
>
> 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
> ---
> 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 16
> --- a/spice-common
> +++ b/spice-common
> @@ -1 +1 @@
> -Subproject commit 429ad965371ceaaf60b81ccbed7da660ef9e0a94
> +Subproject commit ba11de3f3fd58d1b1a99bb62dd9e409e9961a78e
>
> probably a mistake
Yes.
>
> 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.
>>>
>>> And calling gst_deinit() from a library is also wrong.
>>
>> Why? What would be a better way?
>
> Not calling it:
But as pointed in the commit log, this means that some gstreamer tools
that rely on gst_deinit being called are broken for all spice clients.
> /**
> * gst_deinit:
> *
> * Clean up any resources created by GStreamer in gst_init().
> *
> * It is normally not needed to call this function in a normal application
> * as the resources will automatically be freed when the program terminates.
> * This function is therefore mostly used by testsuites and other memory
> * profiling tools.
> *
> * After this call GStreamer (including this method) should not be used
> anymore.
> */
>
> spice-gtk is a library, that happen to use gstreamer.
If this library did init gstreamer, it is responsible to deinit it.
Just like if it opens a file or allocates memory, it’s responsible to close it
or deallocate it.
>
> If something else in the client app is also using gstreamer this will cause
> troubles.
And if someone unloads our library presently, it leaks.
>
> Well calling atexit() should be fine, even if called multiple times, Why it's
> not done in gstreamer in this case? Why would every application have to add
> gst_deinit() themself?
Well, the gstreamer init / deinit design reeks of big globals lurking somewhere
anyway.
So if you ask me, that interface should have been more like:
gst_id id = gst_init();
gst_deinit(id);
That would have made it very clear that you could init multiple times and that
each caller
was responsible for calling deinit. But of course, that would have also meant
passing the
id around all the time. Much less convenient.
Since we don’t have that kind of interface, we can only second-guess wha the
intent of the
orignal developers was.
Globals are evil. Pthread globals are worse. That’s not a reason to not call
deinit.
>
> One reason is that you may use a library dynamically, and you may dlclose()
> it, and then atexit() will likely crash.
Is that a real or theoretical scenario? Who loads this library dynamically
currently?
>
>>
>> The scenario where we call gst_init is a bit complicated,
>> and there are multiple clients that may call it. I see
>> no case where, when we have called gst_init, we are not
>> responsible for also calling gst_deinit.
>>
>> In any case, it’s even more wrong to not call gst_deinit at all,
>> and I don’t see a simple way to do that from main with the
>> existing code without elaborate tests that have somewhat
>> deep knowledge of the library internals.
>>
>> How would you do it?
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-gtk] Add call of gst_deinit at program exit
On Thu, Oct 19, 2017 at 08:53:08AM -0400, Frediano Ziglio wrote: > > > > On Thu, Oct 19, 2017 at 08:03:00AM -0400, Frediano Ziglio wrote: > > > > > > > > > > > > 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. > > > > > > 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. > > > > Calling it laziness is completely missing the point. There is *nothing* > > in the pthreads API that lets you avoid the problem with thread local > > destructors described above, no matter how much we want to fix it. The > > only "solution" is to not use pthread locals at all, which is not practical. > > > > If not lazy as I said is bad design, not? Somebody designed pthread too. POSIX designed pthreads, but dlopen comes from Solaris, only after that adopted as part of POSIX. The situation is just a result of the way the standard has organically grown over time, not been designed top down to deal with all possible interactions 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 :| ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-gtk] Add call of gst_deinit at program exit
> > On Thu, Oct 19, 2017 at 08:03:00AM -0400, Frediano Ziglio wrote: > > > > > > > > > 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. > > > > 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. > > Calling it laziness is completely missing the point. There is *nothing* > in the pthreads API that lets you avoid the problem with thread local > destructors described above, no matter how much we want to fix it. The > only "solution" is to not use pthread locals at all, which is not practical. > > > Regards, > Daniel If not lazy as I said is bad design, not? Somebody designed pthread too. Frediano ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-gtk] Add call of gst_deinit at program exit
On Thu, Oct 19, 2017 at 08:03:00AM -0400, Frediano Ziglio wrote: > > > > > > 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. > > 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. Calling it laziness is completely missing the point. There is *nothing* in the pthreads API that lets you avoid the problem with thread local destructors described above, no matter how much we want to fix it. The only "solution" is to not use pthread locals at all, which is not practical. 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 :| ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-gtk] Add call of gst_deinit at program exit
>
> On Thu, Oct 19, 2017 at 07:47:24AM -0400, Frediano Ziglio wrote:
> > > > On 19 Oct 2017, at 12:32, Frediano Ziglio < fzig...@redhat.com > wrote:
> > >
> >
> > > > > From: Christophe de Dinechin < dinec...@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 < dinec...@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 16
> > > >
> > >
> > > > > --- 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
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-gtk] Add call of gst_deinit at program exit
On Thu, Oct 19, 2017 at 07:47:24AM -0400, Frediano Ziglio wrote:
> > > On 19 Oct 2017, at 12:32, Frediano Ziglio < fzig...@redhat.com > wrote:
> >
>
> > > > From: Christophe de Dinechin < dinec...@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 < dinec...@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 16
> > >
> >
> > > > --- 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 :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-gtk] Add call of gst_deinit at program exit
- Original Message -
>
> > On 19 Oct 2017, at 13:15, Marc-André Lureau
> > wrote:
> >
> > Hi
> >
> > - Original Message -
> >>>
> >>> From: Christophe de Dinechin
> >>>
> >>> 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
> >>> ---
> >>> 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 16
> >>> --- a/spice-common
> >>> +++ b/spice-common
> >>> @@ -1 +1 @@
> >>> -Subproject commit 429ad965371ceaaf60b81ccbed7da660ef9e0a94
> >>> +Subproject commit ba11de3f3fd58d1b1a99bb62dd9e409e9961a78e
probably a mistake
> >>> 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.
> >
> > And calling gst_deinit() from a library is also wrong.
>
> Why? What would be a better way?
Not calling it:
/**
* gst_deinit:
*
* Clean up any resources created by GStreamer in gst_init().
*
* It is normally not needed to call this function in a normal application
* as the resources will automatically be freed when the program terminates.
* This function is therefore mostly used by testsuites and other memory
* profiling tools.
*
* After this call GStreamer (including this method) should not be used
anymore.
*/
spice-gtk is a library, that happen to use gstreamer.
If something else in the client app is also using gstreamer this will cause
troubles.
Well calling atexit() should be fine, even if called multiple times, Why it's
not done in gstreamer in this case? Why would every application have to add
gst_deinit() themself?
One reason is that you may use a library dynamically, and you may dlclose() it,
and then atexit() will likely crash.
>
> The scenario where we call gst_init is a bit complicated,
> and there are multiple clients that may call it. I see
> no case where, when we have called gst_init, we are not
> responsible for also calling gst_deinit.
>
> In any case, it’s even more wrong to not call gst_deinit at all,
> and I don’t see a simple way to do that from main with the
> existing code without elaborate tests that have somewhat
> deep knowledge of the library internals.
>
> How would you do it?
>
> >
>
>
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-gtk] Add call of gst_deinit at program exit
> > On 19 Oct 2017, at 12:32, Frediano Ziglio < fzig...@redhat.com > wrote:
>
> > > From: Christophe de Dinechin < dinec...@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 < dinec...@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 16
> >
>
> > > --- 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).
Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-gtk] Add call of gst_deinit at program exit
> On 19 Oct 2017, at 13:19, Frediano Ziglio wrote:
>
>>
>> Hi
>>
>> - Original Message -
From: Christophe de Dinechin
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
---
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 16
--- 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.
>>
>> And calling gst_deinit() from a library is also wrong.
>>
>>
>
> If the main target is just spicy and knowing spicy is mainly a developer
> tool you can add a call to gst_deinit in spicy.
Why would only spicy be concerned? That’s the case where I saw
the issue, but I guess any user of the library has the same issue.
> Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-gtk] Add call of gst_deinit at program exit
> On 19 Oct 2017, at 13:15, Marc-André Lureau
> wrote:
>
> Hi
>
> - Original Message -
>>>
>>> From: Christophe de Dinechin
>>>
>>> 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
>>> ---
>>> 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 16
>>> --- 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.
>
> And calling gst_deinit() from a library is also wrong.
Why? What would be a better way?
The scenario where we call gst_init is a bit complicated,
and there are multiple clients that may call it. I see
no case where, when we have called gst_init, we are not
responsible for also calling gst_deinit.
In any case, it’s even more wrong to not call gst_deinit at all,
and I don’t see a simple way to do that from main with the
existing code without elaborate tests that have somewhat
deep knowledge of the library internals.
How would you do it?
>
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-gtk] Add call of gst_deinit at program exit
> On 19 Oct 2017, at 12:32, Frediano Ziglio wrote:
>
>>
>> From: Christophe de Dinechin
>>
>> 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
>> ---
>> 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 16
>> --- 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
>
> Frediano
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
>
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-gtk] Add call of gst_deinit at program exit
>
> Hi
>
> - Original Message -
> > >
> > > From: Christophe de Dinechin
> > >
> > > 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
> > > ---
> > > 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 16
> > > --- 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.
>
> And calling gst_deinit() from a library is also wrong.
>
>
If the main target is just spicy and knowing spicy is mainly a developer
tool you can add a call to gst_deinit in spicy.
Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-gtk] Add call of gst_deinit at program exit
Hi
- Original Message -
> >
> > From: Christophe de Dinechin
> >
> > 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
> > ---
> > 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 16
> > --- 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.
And calling gst_deinit() from a library is also wrong.
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-gtk] Add call of gst_deinit at program exit
>
> From: Christophe de Dinechin
>
> 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
> ---
> 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 16
> --- 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.
Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH spice-gtk] Add call of gst_deinit at program exit
From: Christophe de Dinechin
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
---
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 16
--- 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);
--
2.13.5 (Apple Git-94)
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
