Subject: New signal stuff - Batch #1

From: Chris Rankin <[email-masked]>
Date: Thu, 20 May 1999 00:21:12 +1000 (EST)

Hi,
This is the diff made against fvwm/fvwm/events.c,
fvwm/fvwm/module_interface.c and fvwm/fvwm/fvwm.c. All it does is
finish the work I started before 2.2; it's not too drastic. It works
OK on my system but I'd like to hear how well it goes down elsewhere
before I start on other modules.

autoconf should probably check for sigmask(), sigblock() and
sigsetmask() for the sake of completeness.

Note that there are also 2 new files called fvwmsignal.c and
fvwmsignal.h. These should ultimately go into libfvwm.a; I haven't
patched any Makefiles - I'm not entirely sure which file the new
module would have to be put into.

Cheers,
Chris.

-----------------------------------------------------------------------------

ChangeLog:
1999-05-19  Chris Rankin

    * Added wrapped version of select() called fvwmSelect() which will
      never block once the isTerminated flag becomes "true". This
      removes a race condition where fvwm could potentially block in a
      select even after it has received a signal telling it to
      terminate.
    * Modified signal-installation to make handlers mutually
      exclusive. This is needed now the handlers access static data
      (to make fvwmSelect work).

-----------------------------------------------------------------------------
#ifndef FVWMSIGNAL_H
#define FVWMSIGNAL_H

/***************************************************************************
 * This module provides wrappers around system functions that could
 * potentially block, e.g. select(). These wrappers will check that
 * the "terminate" flag is still clear and then call the system
 * function in one atomic operation. This ensures that FVWM will not
 * block in a system function once it has received the signal to quit.
 *
 * This module was written by Chris Rankin,  [email-masked]
 ***************************************************************************/

/***************************************************************************
 * This module is intended to use POSIX.1 signal semantics, since most
 * modern systems can reasonably be expected to support POSIX and since
 * the semantics of the old "signal()" function vary from system to system.
 * If POSIX.1 is not available then the module can provide BSD signal
 * semantics, which can be summarised as follows:
 *  - the signal handler will NOT uninstall itself once it has been called
 *  - a signal will be temporarily blocked from further delivery so long
 *    as its handler is running
 *  - certain system calls will be automatically restarted if interrupted
 *    by a signal
 */

#if !defined(HAVE_SIGACTION) \
    && defined(HAVE_SIGBLOCK) && defined(HAVE_SIGSETMASK)
#  define USE_BSD_SIGNALS
#endif

#include <signal.h>
#include <sys/time.h>

/***************************************************************************
 * Global variables
 */
extern volatile sig_atomic_t isTerminated;

/***************************************************************************
 * Module prototypes
 */
extern void fvwmSetTerminate(void);

#ifdef USE_BSD_SIGNALS
extern void fvwmSetSignalMask(int);
#endif

#ifdef 	HAVE_SELECT
extern int fvwmSelect(fd_set_size_t nfds,
                      fd_set *readfds, fd_set *writefds, fd_set *exceptfds,
                      struct timeval *timeout);
#endif

#endif /* FVWMSIGNAL_H */
-----------------------------------------------------------------------------
/***************************************************************************
 * fvwmsignal.c
 * Written by Chris Rankin,  [email-masked]
 *
 ***************************************************************************/

#include "config.h"

#include <setjmp.h>
#include <errno.h>
#include "fvwmsignal.h"

#define true   1
#define false  0

typedef enum { SIG_INIT=0, SIG_DONE } SIG_STATUS;

volatile sig_atomic_t isTerminated = false;

static volatile sig_atomic_t canJump = false;
static sigjmp_buf deadJump;

#ifdef USE_BSD_SIGNALS
static int term_sigs;

/************************************************************************
 * fvwmSetSignalMask - store the set of mutually exclusive signals
 * away for future reference. This prevents different signals from
 * trying to access the same static data at the same time.
 *
 * NOTE: We don't need this if we have POSIX.1 since we can install
 *       a signal mask automatically using sigaction()
 */
void
fvwmSetSignalMask(int sigmask)
{
  term_sigs = sigmask;
}
#endif

/************************************************************************
 * fvwmSetTerminate - set the "end-of-execution" flag.
 * This function should ONLY be called at the end of a
 * signal handler. It is an integral part of the mechanism
 * to stop system calls from blocking once the process is
 * finished. 
 *
 * NOTE: This is NOT a signal handler function in its own right!
 */
void
fvwmSetTerminate(void)
{
#ifdef USE_BSD_SIGNALS
  int old_mask = sigblock(term_sigs);
#endif
  isTerminated = true;

  if (canJump)
  {
    canJump = false;
    errno = EINTR;
    /*
     * This non-local jump is safe ONLY because we haven't called
     * any non-reentrant functions in the short period where the
     * "canJump" variable is true.
     *
     * NOTE: No need to restore the signal mask, since siglongjmp
     *       is designed to do that for us.
     */
    siglongjmp(deadJump, SIG_DONE);
  }
#ifdef USE_BSD_SIGNALS
  sigsetmask(old_mask);
#endif
}

#ifdef HAVE_SELECT
/************************************************************************
 * fvwmSelect - wrapper around the select() system call.
 * This system call may block indefinitely. We don't want
 * to block at all if the "terminate" flag is set - we
 * just want it to fail as quickly as possible.
 */
int
fvwmSelect(fd_set_size_t nfds,
           fd_set *readfds, fd_set *writefds, fd_set *exceptfds,
           struct timeval *timeout)
{
  volatile int iRet = -1;  /* This variable MUST NOT be in a register */

  /*
   * Now initialise the non-local jump. Between here and the end of
   * the routine (more-or-less) we must NOT call any non-reentrant
   * functions! This is because we might need to abandon them half
   * way through execution and return here!
   */
  if ( sigsetjmp(deadJump, 1) == SIG_INIT )
  {
    /*
     * Activate the non-local jump. Between now and when we turn the
     * jump off again, we must NOT call any non-reentrant functions
     * because we could be interrupted halfway through ...
     */
    canJump = true;

    /*
     * If we have already been told to terminate then we will not
     * execute the select() because the flag will be set. If a
     * "terminate" signal arrives between testing the flag and
     * calling select() then we will jump back to the non-local
     * jump point ...
     */
    if ( !isTerminated )
    {
      /*
       * The "die" signal will interrupt this system call:
       * that IS the whole point, after all :-)
       */
      iRet = select(nfds, readfds, writefds, exceptfds, timeout);
    }

    /*
     * The non-local jump is about to go out of scope,
     * so we must deactivate it. Note that the return-
     * value from select() will be safely stored in the
     * local variable before the jump is disabled.
     */
    canJump = false;
  }

  return iRet;
}

#endif  /* HAVE_SELECT */

-----------------------------------------------------------------------------

--- events.c.orig	Wed May 19 21:37:44 1999
+++ events.c	Wed May 19 22:26:19 1999
@@ -69,6 +69,7 @@
 #endif
 
 #include "fvwm.h"
+#include "fvwmsignal.h"
 #include "events.h"
 #include "icons.h"
 #include <X11/Xatom.h>
@@ -204,13 +205,6 @@
  ************************************************************************/
 void HandleEvents(void)
 {
-  /*
-   * TEMPORARY declaration: this variable should really be
-   * part of a separate source-file to handle signals
-   */
-  extern volatile sig_atomic_t isTerminated;
-  /**/
-
   DBUG("HandleEvents","Routine Entered");
   while ( !isTerminated )
     {
@@ -1744,11 +1738,11 @@
 
   DBUG("My_XNextEvent","waiting for module input/output");
   XFlush(dpy);
-  if (select( fd_width,
-	      SELECT_FD_SET_CAST &in_fdset,
-	      SELECT_FD_SET_CAST &out_fdset,
-	      SELECT_FD_SET_CAST 0,
-              timeoutP) > 0) {
+  if (fvwmSelect( fd_width,
+	          SELECT_FD_SET_CAST &in_fdset,
+	          SELECT_FD_SET_CAST &out_fdset,
+	          SELECT_FD_SET_CAST 0,
+                  timeoutP) > 0) {
 
     /* Check for module input. */
     for (i=0; i<npipes; i++) {
--- fvwm.c.orig	Wed May 19 21:37:44 1999
+++ fvwm.c	Wed May 19 23:43:44 1999
@@ -39,6 +39,7 @@
 #endif
 
 #include "fvwm.h"
+#include "fvwmsignal.h"
 #include "events.h"
 #include "functions.h"
 #include "menus.h"
@@ -89,7 +90,7 @@
 int FvwmErrorHandler(Display *, XErrorEvent *);
 int CatchFatal(Display *);
 int CatchRedirectError(Display *, XErrorEvent *);
-void newhandler(int sig);
+void InstallSignals(void);
 void CreateCursors(void);
 void ChildDied(int nonsense);
 void SaveDesktopState(void);
@@ -141,15 +142,7 @@
 
 typedef enum { FVWM_RUNNING=0, FVWM_DONE, FVWM_RESTART } FVWM_STATE;
 
-/*
- * Currently, the "isTerminated" variable is superfluous. However,
- * I am looking to the future where isTerminated controls all
- * event loops in FVWM: then we can collect such code into a common
- * object-file ...
- */
 static volatile sig_atomic_t fvwmRunState = FVWM_RUNNING;
-volatile sig_atomic_t isTerminated = False;
-/**/
 
 /***********************************************************************
  *
@@ -322,49 +315,7 @@
   }
 
   DBUG("main","Installing signal handlers");
-
-#ifdef HAVE_SIGACTION
-  {
-    struct sigaction sigact;
-
-    /*
-     * Use reliable signal semantics since they are predictable and portable.
-     * DeadPipe() looks like a no-op, so don't stop processing system calls
-     * to handle a SIGPIPE (Why don't we just -ignore- SIGPIPE?)
-     */
-#ifdef SA_RESTART
-    sigact.sa_flags = SA_RESTART;
-#else
-    sigact.sa_flags = 0;
-#endif
-    sigemptyset(&sigact.sa_mask);
-
-    sigact.sa_handler = DeadPipe;  /* This handler does nothing ??? */
-    sigaction(SIGPIPE, &sigact, NULL);
-
-    /*
-     * If we need to restart then we need to stop what we're doing as
-     * quickly as possible - hence interrupt any system call we're
-     * blocked in ...
-     */
-#ifdef SA_INTERRUPT
-    sigact.sa_flags = SA_INTERRUPT;
-#else
-    sigact.sa_flags = 0;
-#endif
-    sigact.sa_handler = Restart;
-    sigaction(SIGUSR1, &sigact, NULL);
-  }
-#else
-  /* We don't have sigaction(), so fall back to less robust methods.  */
-  signal(SIGPIPE, DeadPipe);
-  signal(SIGUSR1, Restart);
-#endif
-
-  newhandler(SIGINT);
-  newhandler(SIGHUP);
-  newhandler(SIGQUIT);
-  newhandler(SIGTERM);
+  InstallSignals();
 
   ReapChildren();
 
@@ -1041,38 +992,90 @@
   return;
 }
 
+
 /***********************************************************************
  *
  *  Procedure:
- *	newhandler: Installs new signal handler (reliable semantics)
+ *	InstallSignals: install the signal handlers, using whatever
+ *         means we have at our disposal. The more POSIXy, the better
  *
  ************************************************************************/
 void
-newhandler(int sig)
+InstallSignals(void)
 {
 #ifdef HAVE_SIGACTION
   struct sigaction  sigact;
 
-  sigaction(sig,NULL,&sigact);
-  if (sigact.sa_handler != SIG_IGN)
-  {
-    /*
-     * SigDone requires that we QUIT as soon as possible afterwards,
-     * so we need to interrupt any system calls we are blocked in
-     */
-    sigemptyset(&sigact.sa_mask);
+  /*
+   * All signals whose handlers call fvwmSetTerminate()
+   * must be mutually exclusive - we mustn't receive one
+   * while processing any of the others ...
+   */
+  sigemptyset(&sigact.sa_mask);
+  sigaddset(&sigact.sa_mask, SIGINT);
+  sigaddset(&sigact.sa_mask, SIGHUP);
+  sigaddset(&sigact.sa_mask, SIGQUIT);
+  sigaddset(&sigact.sa_mask, SIGTERM);
+  sigaddset(&sigact.sa_mask, SIGPIPE);
+  sigaddset(&sigact.sa_mask, SIGUSR1);
+
+#ifdef SA_RESTART
+  sigact.sa_flags = SA_RESTART;
+#else
+  sigact.sa_flags = 0;
+#endif;
+  sigact.sa_handler = DeadPipe;
+  sigaction(SIGPIPE, &sigact, NULL);
+
 #ifdef SA_INTERRUPT
-    sigact.sa_flags = SA_INTERRUPT;
+  sigact.sa_flags = SA_INTERRUPT;
 #else
-    sigact.sa_flags = 0;
+  sigact.sa_flags = 0;
 #endif
-    sigact.sa_handler = SigDone;
-    sigaction(sig, &sigact, NULL);
-  }
+  sigact.sa_handler = Restart;
+  sigaction(SIGUSR1, &sigact, NULL);
+
+  sigact.sa_handler = SigDone;
+  sigaction(SIGINT,  &sigact, NULL);
+  sigaction(SIGHUP,  &sigact, NULL);
+  sigaction(SIGQUIT, &sigact, NULL);
+  sigaction(SIGTERM, &sigact, NULL);
 #else
-  /* We don't have sigaction(), so use less robust methods.  */
-  if (signal(sig,SIG_IGN) == SIG_IGN)
-    signal(sig,SigDone);
+#ifdef USE_BSD_SIGNALS
+  fvwmSetSignalMask( sigmask(SIGPIPE) |
+                     sigmask(SIGUSR1) |
+                     sigmask(SIGINT)  |
+                     sigmask(SIGHUP)  |
+                     sigmask(SIGQUIT) |
+                     sigmask(SIGTERM) );
+#endif
+
+  /*
+   * We don't have sigaction(), so fall back on
+   * less reliable methods ...
+   */
+  signal(SIGPIPE, DeadPipe);
+  signal(SIGUSR1, Restart);
+#ifdef HAVE_SIGINTERRUPT
+  siginterrupt(SIGUSR1, 1);
+#endif
+
+  signal(SIGINT, SigDone);
+#ifdef HAVE_SIGINTERRUPT
+  siginterrupt(SIGINT, 1);
+#endif
+  signal(SIGHUP, SigDone);
+#ifdef HAVE_SIGINTERRUPT
+  siginterrupt(SIGHUP, 1);
+#endif
+  signal(SIGQUIT, SigDone);
+#ifdef HAVE_SIGINTERRUPT
+  siginterrupt(SIGQUIT, 1);
+#endif
+  signal(SIGTERM, SigDone);
+#ifdef HAVE_SIGINTERRUPT
+  siginterrupt(SIGTERM, 1);
+#endif
 #endif
 }
 
@@ -1082,8 +1085,14 @@
  ************************************************************************/
 RETSIGTYPE Restart(int nonsense)
 {
-  isTerminated = True;
   fvwmRunState = FVWM_RESTART;
+
+  /*
+   * This function might not return - it could "long-jump"
+   * right out, so we need to do everything we need to do
+   * BEFORE we call it ...
+   */
+  fvwmSetTerminate();
 }
 
 /***********************************************************************
@@ -1673,8 +1682,14 @@
  */
 RETSIGTYPE SigDone(int nonsense)
 {
-  isTerminated = True;
   fvwmRunState = FVWM_DONE;
+
+  /*
+   * This function might not return - it could "long-jump"
+   * right out, so we need to do everything we need to do
+   * BEFORE we call it ...
+   */
+  fvwmSetTerminate();
 }
 
 void Done(int restart, char *command)
--- module_interface.c.orig	Wed May 19 23:55:47 1999
+++ module_interface.c	Wed May 19 22:40:30 1999
@@ -39,6 +39,7 @@
 #include <sys/socket.h>
 
 #include "fvwm.h"
+#include "fvwmsignal.h"
 #include "events.h"
 #include "misc.h"
 #include "parse.h"
@@ -398,6 +399,12 @@
 
 RETSIGTYPE DeadPipe(int nonsense)
 {
+  /*
+   * This function might not return - it could "long-jump"
+   * right out, so we need to do everything we need to do
+   * BEFORE we call it ...
+   */
+  fvwmSetTerminate();
 }

--
Visit the official FVWM web page at <URL:http://www.fvwm.org/>.
To unsubscribe from the list, send "unsubscribe fvwm-workers" in the
body of a message to [email-masked].
To report problems, send mail to [email-masked]

---

From: Dominik Vogt <[email-masked]>
Date: Wed, 19 May 1999 17:18:17 +0200

> This is the diff made against fvwm/fvwm/events.c,

I assume that means that you still don't have CVS access. Are there
any objections to give it out to Chris? If not you can send an
encrypted UNIX password to Jason and he will put you on the access
list.

Bye

Dominik ^_^

-- 
Dominik Vogt, Hewlett-Packard GmbH, Dept. BVS
Herrenberger Str.130, 71034 Boeblingen, Germany
phone: 07031/14-4596, fax: 07031/14-3883, [email-masked]
--
Visit the official FVWM web page at <URL:http://www.fvwm.org/>.
To unsubscribe from the list, send "unsubscribe fvwm-workers" in the
body of a message to [email-masked].
To report problems, send mail to [email-masked]

---

From: Steve Robbins <[email-masked]>
Date: Wed, 19 May 1999 21:42:34 -0400 (EDT)

Hi Chris,

I haven't really looked at your patch, but I think there is a bug in the
select() handling.  

We are guaranteed by autoconf to have the type "fd_set" defined, and your
select() wrapper uses it.  So all the calls to your select wrapper should
just use fd_set* variables for arguments 2-4.  *Inside* fvmwSelect(),
however, you make a call to the real select.  This call potentially needs
a cast for args 2-4.

You've done things the other way 'round.

>                       struct timeval *timeout);

[ ... ]

>       iRet = select(nfds, readfds, writefds, exceptfds, timeout);
                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
You probably meant to put in the SELECT_FD_SET_CAST here

> +	          SELECT_FD_SET_CAST &out_fdset,

The call to fvwmSelect() needs no cast, since we know it is defined using
"fd_set *" for args 2-4.

---

From: Chris Rankin <[email-masked]>
Date: Thu, 20 May 1999 21:23:35 +1000 (EST)

> >       iRet = select(nfds, readfds, writefds, exceptfds, timeout);

Yes, I see your point - my intention was simply to make a "drop-in"
replacement for select(); I tested it by "dropping it in" :-). No
problem on a linux-2.2 box, of course... I'll move the autoconf cast
inside ...

Cheers,
Chris.

---

From: Steve Robbins <[email-masked]>
Date: Thu, 20 May 1999 09:09:38 -0400 (EDT)

> problem on a linux-2.2 box, of course... I'll move the autoconf cast

In fact, your wrapper is BETTER than just a drop-in replacement in that we
only have to muck around with casts in *one* place!

--
Steve Robbins                              <[email-masked]>

When I was dead, I wore a strong perfume.
When I was dead, I never left the room.