[Spice-devel] [PATCH phodav 03/13] spice: handle SIGINT properly

[Frediano Ziglio]

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