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

[Jakub Janku]

Hi,

On Mon, May 27, 2019 at 5:58 PM Frediano Ziglio <fziglio at redhat.com> wrote:
>
> >
> > 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.

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