[phodav,03/13] spice: handle SIGINT properly

Submitted by Jakub Janku on May 23, 2019, 8:37 a.m.

Details

Message ID 20190523083725.1554-4-jjanku@redhat.com
State New
Headers show
Series "Miscellaneous series" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Jakub Janku May 23, 2019, 8:37 a.m.
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ů <jjanku@redhat.com>
---
 spice/spice-webdavd.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Patch hide | download patch | download mbox

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;

Comments

Hi

On Thu, May 23, 2019 at 10:37 AM Jakub Janků <jjanku@redhat.com> 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ů <jjanku@redhat.com>
> ---
>  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
Hi,

On Thu, May 23, 2019 at 3:31 PM Marc-André Lureau
<marcandre.lureau@gmail.com> wrote:
>
> Hi
>
> On Thu, May 23, 2019 at 10:37 AM Jakub Janků <jjanku@redhat.com> 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ů <jjanku@redhat.com>
> > ---
> >  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
> 
> Hi,
> 
> On Thu, May 23, 2019 at 3:31 PM Marc-André Lureau
> <marcandre.lureau@gmail.com> wrote:
> >
> > Hi
> >
> > On Thu, May 23, 2019 at 10:37 AM Jakub Janků <jjanku@redhat.com> 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ů <jjanku@redhat.com>
> > > ---
> > >  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
Hi,

On Mon, May 27, 2019 at 5:58 PM Frediano Ziglio <fziglio@redhat.com> wrote:
>
> >
> > Hi,
> >
> > On Thu, May 23, 2019 at 3:31 PM Marc-André Lureau
> > <marcandre.lureau@gmail.com> wrote:
> > >
> > > Hi
> > >
> > > On Thu, May 23, 2019 at 10:37 AM Jakub Janků <jjanku@redhat.com> 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ů <jjanku@redhat.com>
> > > > ---
> > > >  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