Re: [Spice-devel] [PATCH phodav 03/13] spice: handle SIGINT properly
Hi,
On Mon, May 27, 2019 at 5:58 PM Frediano Ziglio wrote:
>
> >
> > Hi,
> >
> > On Thu, May 23, 2019 at 3:31 PM Marc-André Lureau
> > wrote:
> > >
> > > Hi
> > >
> > > On Thu, May 23, 2019 at 10:37 AM Jakub Janků wrote:
> > > >
> > > > According to [0], g_debug should not be used in a signal handler.
> > > > So, to avoid reentrancy, do not print debug message when quit is
> > > > called with SIGINT.
> > > >
> > > > [0]
> > > > https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/signal?view=vs-2019
> > > >
>
>
>
> The quit function is called by signal handler or "manually".
> If called manually it's not a problem.
> The only signal registered for this function is SIGINT which in Windows
> is managed by another thread (as written in the link you sent, and by the
> way is handled by SetConsoleCtrlHandler) so it's not a problem to call
> g_debug. Note that this function is also called manually with SIGTERM but
> still not a problem on Windows as service_ctrl_handler is run in another
> thread.
Oh, I somehow missed the big purple box that says Ctrl+C creates a new
thread and I supposed it behaves like on unix, sorry, my bad.
>
> The problems I see is that quit_service should be defined volatile and
> g_main_loop_quit should not be called on Unix! If a lock used by
> g_main_loop_quit is retained while the signal is called you'll have
> a deadlock.
> Maybe I'm wrong but I didn't find a note if g_main_loop_quit is signal
> safe so better not to call it from a signal handler.
> g_unix_signal_add seems a good solution for Unix.
Hopefully better this time:
https://gitlab.gnome.org/GNOME/phodav/merge_requests/3/
Thanks,
Jakub
>
> > > > Signed-off-by: Jakub Janků
> > > > ---
> > > > spice/spice-webdavd.c | 3 ++-
> > > > 1 file changed, 2 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/spice/spice-webdavd.c b/spice/spice-webdavd.c
> > > > index e494692..cdfa73d 100644
> > > > --- a/spice/spice-webdavd.c
> > > > +++ b/spice/spice-webdavd.c
> > > > @@ -237,7 +237,8 @@ static void mdns_unregister_service (void);
> > > > static void
> > > > quit (int sig)
> > > > {
> > > > - g_debug ("quit %d", sig);
> > > > + if (sig != SIGINT)
> > > > +g_debug ("quit %d", sig);
> > > >
> > >
> > > I would simply remove the g_debug() call then.
> >
> > Ok then.
> >
> > On Unix, we could use g_unix_signal_add, I'll change it.
> > But sadly there doesn't seem to be a Windows equivalent.
> >
> > Cheers,
> > Jakub
> > >
> > > (maybe we should have a different function for the signal handler)
> > >
>
> It sounds a great idea.
>
> > > >if (sig == SIGINT || sig == SIGTERM)
> > > >quit_service = TRUE;
>
> Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH phodav 03/13] spice: handle SIGINT properly
>
> Hi,
>
> On Thu, May 23, 2019 at 3:31 PM Marc-André Lureau
> wrote:
> >
> > Hi
> >
> > On Thu, May 23, 2019 at 10:37 AM Jakub Janků wrote:
> > >
> > > According to [0], g_debug should not be used in a signal handler.
> > > So, to avoid reentrancy, do not print debug message when quit is
> > > called with SIGINT.
> > >
> > > [0]
> > > https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/signal?view=vs-2019
> > >
The quit function is called by signal handler or "manually".
If called manually it's not a problem.
The only signal registered for this function is SIGINT which in Windows
is managed by another thread (as written in the link you sent, and by the
way is handled by SetConsoleCtrlHandler) so it's not a problem to call
g_debug. Note that this function is also called manually with SIGTERM but
still not a problem on Windows as service_ctrl_handler is run in another
thread.
The problems I see is that quit_service should be defined volatile and
g_main_loop_quit should not be called on Unix! If a lock used by
g_main_loop_quit is retained while the signal is called you'll have
a deadlock.
Maybe I'm wrong but I didn't find a note if g_main_loop_quit is signal
safe so better not to call it from a signal handler.
g_unix_signal_add seems a good solution for Unix.
> > > Signed-off-by: Jakub Janků
> > > ---
> > > spice/spice-webdavd.c | 3 ++-
> > > 1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/spice/spice-webdavd.c b/spice/spice-webdavd.c
> > > index e494692..cdfa73d 100644
> > > --- a/spice/spice-webdavd.c
> > > +++ b/spice/spice-webdavd.c
> > > @@ -237,7 +237,8 @@ static void mdns_unregister_service (void);
> > > static void
> > > quit (int sig)
> > > {
> > > - g_debug ("quit %d", sig);
> > > + if (sig != SIGINT)
> > > +g_debug ("quit %d", sig);
> > >
> >
> > I would simply remove the g_debug() call then.
>
> Ok then.
>
> On Unix, we could use g_unix_signal_add, I'll change it.
> But sadly there doesn't seem to be a Windows equivalent.
>
> Cheers,
> Jakub
> >
> > (maybe we should have a different function for the signal handler)
> >
It sounds a great idea.
> > >if (sig == SIGINT || sig == SIGTERM)
> > >quit_service = TRUE;
Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH phodav 03/13] spice: handle SIGINT properly
Hi,
On Thu, May 23, 2019 at 3:31 PM Marc-André Lureau
wrote:
>
> Hi
>
> On Thu, May 23, 2019 at 10:37 AM Jakub Janků wrote:
> >
> > According to [0], g_debug should not be used in a signal handler.
> > So, to avoid reentrancy, do not print debug message when quit is
> > called with SIGINT.
> >
> > [0]
> > https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/signal?view=vs-2019
> >
> > Signed-off-by: Jakub Janků
> > ---
> > spice/spice-webdavd.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/spice/spice-webdavd.c b/spice/spice-webdavd.c
> > index e494692..cdfa73d 100644
> > --- a/spice/spice-webdavd.c
> > +++ b/spice/spice-webdavd.c
> > @@ -237,7 +237,8 @@ static void mdns_unregister_service (void);
> > static void
> > quit (int sig)
> > {
> > - g_debug ("quit %d", sig);
> > + if (sig != SIGINT)
> > +g_debug ("quit %d", sig);
> >
>
> I would simply remove the g_debug() call then.
Ok then.
On Unix, we could use g_unix_signal_add, I'll change it.
But sadly there doesn't seem to be a Windows equivalent.
Cheers,
Jakub
>
> (maybe we should have a different function for the signal handler)
>
> >if (sig == SIGINT || sig == SIGTERM)
> >quit_service = TRUE;
> > --
> > 2.21.0
> >
> > ___
> > Spice-devel mailing list
> > Spice-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
>
>
>
> --
> Marc-André Lureau
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH phodav 03/13] spice: handle SIGINT properly
Hi
On Thu, May 23, 2019 at 10:37 AM Jakub Janků wrote:
>
> According to [0], g_debug should not be used in a signal handler.
> So, to avoid reentrancy, do not print debug message when quit is
> called with SIGINT.
>
> [0]
> https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/signal?view=vs-2019
>
> Signed-off-by: Jakub Janků
> ---
> spice/spice-webdavd.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/spice/spice-webdavd.c b/spice/spice-webdavd.c
> index e494692..cdfa73d 100644
> --- a/spice/spice-webdavd.c
> +++ b/spice/spice-webdavd.c
> @@ -237,7 +237,8 @@ static void mdns_unregister_service (void);
> static void
> quit (int sig)
> {
> - g_debug ("quit %d", sig);
> + if (sig != SIGINT)
> +g_debug ("quit %d", sig);
>
I would simply remove the g_debug() call then.
(maybe we should have a different function for the signal handler)
>if (sig == SIGINT || sig == SIGTERM)
>quit_service = TRUE;
> --
> 2.21.0
>
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
--
Marc-André Lureau
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH phodav 03/13] spice: handle SIGINT properly
According to [0], g_debug should not be used in a signal handler.
So, to avoid reentrancy, do not print debug message when quit is
called with SIGINT.
[0]
https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/signal?view=vs-2019
Signed-off-by: Jakub Janků
---
spice/spice-webdavd.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/spice/spice-webdavd.c b/spice/spice-webdavd.c
index e494692..cdfa73d 100644
--- a/spice/spice-webdavd.c
+++ b/spice/spice-webdavd.c
@@ -237,7 +237,8 @@ static void mdns_unregister_service (void);
static void
quit (int sig)
{
- g_debug ("quit %d", sig);
+ if (sig != SIGINT)
+g_debug ("quit %d", sig);
if (sig == SIGINT || sig == SIGTERM)
quit_service = TRUE;
--
2.21.0
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
