Mailing List Archive

[patch] move signal handlers out to threads
Hi,

attached patch moves sig handlers out of signal context. actual
signals are taken by a stub handler, after which a timer (currently
running every 2s) thread runs the actual user supplied handlers
safely outside of sig context.

i've tested this with a wee test case programme, should work fine.
patch only cleans up bgpd, but its trivial to convert over.

regards,
--
Paul Jakma paul@clubi.ie paul@jakma.org Key ID: 64A2FF6A
warning: do not ever send email to spam@dishone.st
Fortune:
I can hire one half of the working class to kill the other half.
-- Jay Gould
Re: [patch] move signal handlers out to threads [ In reply to ]
updated:

- setup bgp signals correctly
- isisd, zebra, ospfd, ripd and ripngd modified

regards,
--
Paul Jakma paul@clubi.ie paul@jakma.org Key ID: 64A2FF6A
warning: do not ever send email to spam@dishone.st
Fortune:
Don't tell me how hard you work. Tell me how much you get done.
-- James J. Ling
Re: [patch] move signal handlers out to threads [ In reply to ]
It is a very good idea. It should fix many random issues with the
signals. The code and the architecture of the patch seems OK.

Regards,
Vincent



Paul Jakma wrote:

>updated:
>
>- setup bgp signals correctly
>- isisd, zebra, ospfd, ripd and ripngd modified
>
>regards,
>
>
>------------------------------------------------------------------------
>
>? ripd/rip_main.
>? tools/test-sig.c
>Index: bgpd/bgp_main.c
>===================================================================
>RCS file: /var/cvsroot/quagga/bgpd/bgp_main.c,v
>retrieving revision 1.7
>diff -u -r1.7 bgp_main.c
>--- bgpd/bgp_main.c 14 Aug 2003 05:32:12 -0000 1.7
>+++ bgpd/bgp_main.c 16 Jan 2004 06:20:17 -0000
>@@ -30,6 +30,7 @@
> #include "prefix.h"
> #include "log.h"
> #include "privs.h"
>+#include "sigevent.h"
>
> #include "bgpd/bgpd.h"
> #include "bgpd/bgp_attr.h"
>@@ -52,6 +53,27 @@
> { 0 }
> };
>
>+/* signal definitions */
>+void sighup (void);
>+void sigint (void);
>+void sigusr1 (void);
>+
>+struct quagga_signal_t bgp_signals[] =
>+{
>+ {
>+ .signal = SIGHUP,
>+ .handler = &sighup,
>+ },
>+ {
>+ .signal = SIGUSR1,
>+ .handler = &sigusr1,
>+ },
>+ {
>+ .signal = SIGINT,
>+ .handler = &sigint,
>+ },
>+};
>+
> /* Configuration file and directory. */
> char config_current[] = BGP_DEFAULT_CONFIG;
> char config_default[] = SYSCONFDIR BGP_DEFAULT_CONFIG;
>@@ -123,7 +145,7 @@
>
> /* SIGHUP handler. */
> void
>-sighup (int sig)
>+sighup (void)
> {
> zlog (NULL, LOG_INFO, "SIGHUP received");
>
>@@ -143,7 +165,7 @@
>
> /* SIGINT handler. */
> void
>-sigint (int sig)
>+sigint (void)
> {
> zlog (NULL, LOG_INFO, "Terminating on signal");
>
>@@ -155,44 +177,10 @@
>
> /* SIGUSR1 handler. */
> void
>-sigusr1 (int sig)
>+sigusr1 (void)
> {
> zlog_rotate (NULL);
> }
>-
>-/* Signale wrapper. */
>-RETSIGTYPE *
>-signal_set (int signo, void (*func)(int))
>-{
>- int ret;
>- struct sigaction sig;
>- struct sigaction osig;
>-
>- sig.sa_handler = func;
>- sigemptyset (&sig.sa_mask);
>- sig.sa_flags = 0;
>-#ifdef SA_RESTART
>- sig.sa_flags |= SA_RESTART;
>-#endif /* SA_RESTART */
>-
>- ret = sigaction (signo, &sig, &osig);
>-
>- if (ret < 0)
>- return (SIG_ERR);
>- else
>- return (osig.sa_handler);
>-}
>-
>-/* Initialization of signal handles. */
>-void
>-signal_init ()
>-{
>- signal_set (SIGHUP, sighup);
>- signal_set (SIGINT, sigint);
>- signal_set (SIGTERM, sigint);
>- signal_set (SIGPIPE, SIG_IGN);
>- signal_set (SIGUSR1, sigusr1);
>-}
>
> /* Main routine of bgpd. Treatment of argument and start bgp finite
> state machine is handled at here. */
>@@ -282,7 +270,7 @@
>
> /* Initializations. */
> srand (time (NULL));
>- signal_init ();
>+ signal_init (master, Q_SIGC(bgp_signals), bgp_signals);
> zprivs_init (&bgpd_privs);
> cmd_init (1);
> vty_init (master);
>Index: isisd/isis_main.c
>===================================================================
>RCS file: /var/cvsroot/quagga/isisd/isis_main.c,v
>retrieving revision 1.3
>diff -u -r1.3 isis_main.c
>--- isisd/isis_main.c 23 Dec 2003 10:42:45 -0000 1.3
>+++ isisd/isis_main.c 16 Jan 2004 06:20:17 -0000
>@@ -34,6 +34,7 @@
> #include "stream.h"
> #include "if.h"
> #include "privs.h"
>+#include "sigevent.h"
>
> #include "isisd/dict.h"
> #include "include-netbsd/iso.h"
>@@ -148,8 +149,9 @@
> /*
> * Signal handlers
> */
>+
> void
>-sighup (int sig)
>+sighup (void)
> {
> zlog_info ("SIGHUP received");
> reload ();
>@@ -158,7 +160,7 @@
> }
>
> void
>-sigint (int sig)
>+sigint (void)
> {
> zlog_info ("SIGINT received");
> terminate (0);
>@@ -167,62 +169,38 @@
> }
>
> void
>-sigterm (int sig)
>+sigterm (void)
> {
> zlog_info ("SIGTERM received");
> terminate (0);
> }
>
> void
>-sigusr1 (int sig)
>+sigusr1 (void)
> {
> zlog_info ("SIGUSR1 received");
> zlog_rotate (NULL);
> }
>
>-/*
>- * Signal wrapper.
>- */
>-RETSIGTYPE *
>-signal_set (int signo, void (*func)(int))
>-{
>- int ret;
>- struct sigaction sig;
>- struct sigaction osig;
>-
>- sig.sa_handler = func;
>- sigemptyset (&sig.sa_mask);
>- sig.sa_flags = 0;
>-#ifdef SA_RESTART
>- sig.sa_flags |= SA_RESTART;
>-#endif /* SA_RESTART */
>-
>- ret = sigaction (signo, &sig, &osig);
>-
>- if (ret < 0)
>- return (SIG_ERR);
>- else
>- return (osig.sa_handler);
>-}
>-
>-void
>-signal_init ()
>-{
>- signal_set (SIGHUP, sighup);
>- signal_set (SIGINT, sigint);
>- signal_set (SIGTERM, sigterm);
>- signal_set (SIGPIPE, SIG_IGN);
>-#ifdef SIGTSTP
>- signal_set (SIGTSTP, SIG_IGN);
>-#endif
>-#ifdef SIGTTIN
>- signal_set (SIGTTIN, SIG_IGN);
>-#endif
>-#ifdef SIGTTOU
>- signal_set (SIGTTOU, SIG_IGN);
>-#endif
>- signal_set (SIGUSR1, sigusr1);
>-}
>+struct quagga_signal_t isisd_signals[] =
>+{
>+ {
>+ .signal = SIGHUP,
>+ .handler = &sighup,
>+ },
>+ {
>+ .signal = SIGUSR1,
>+ .handler = &sigusr1,
>+ },
>+ {
>+ .signal = SIGINT,
>+ .handler = &sigint,
>+ },
>+ {
>+ .signal = SIGTERM,
>+ .handler = &sigterm,
>+ },
>+};
>
> /*
> * Main routine of isisd. Parse arguments and handle IS-IS state machine.
>@@ -315,7 +293,7 @@
> * initializations
> */
> zprivs_init (&isisd_privs);
>- signal_init ();
>+ signal_init (master, Q_SIGC(isisd_signals), isisd_signals);
> cmd_init (1);
> vty_init (master);
> memory_init ();
>Index: lib/Makefile.am
>===================================================================
>RCS file: /var/cvsroot/quagga/lib/Makefile.am,v
>retrieving revision 1.8
>diff -u -r1.8 Makefile.am
>--- lib/Makefile.am 8 Dec 2003 18:12:34 -0000 1.8
>+++ lib/Makefile.am 16 Jan 2004 06:20:17 -0000
>@@ -10,7 +10,8 @@
> print_version.c checksum.c vector.c linklist.c vty.c command.c \
> sockunion.c prefix.c thread.c if.c memory.c buffer.c table.c hash.c \
> filter.c routemap.c distribute.c stream.c str.c log.c plist.c \
>- zclient.c sockopt.c smux.c md5.c if_rmap.c keychain.c privs.c debug.c
>+ zclient.c sockopt.c smux.c md5.c if_rmap.c keychain.c privs.c \
>+ debug.c sigevent.c
>
> libzebra_a_DEPENDENCIES = @LIB_REGEX@
>
>@@ -21,7 +22,7 @@
> memory.h network.h prefix.h routemap.h distribute.h sockunion.h \
> str.h stream.h table.h thread.h vector.h version.h vty.h zebra.h \
> plist.h zclient.h sockopt.h smux.h md5-gnu.h if_rmap.h keychain.h \
>- privs.h debug.h
>+ privs.h debug.h sigevent.h
>
> EXTRA_DIST = regex.c regex-gnu.h
>
>Index: lib/sigevent.c
>===================================================================
>RCS file: lib/sigevent.c
>diff -N lib/sigevent.c
>--- /dev/null 1 Jan 1970 00:00:00 -0000
>+++ lib/sigevent.c 16 Jan 2004 06:20:18 -0000
>@@ -0,0 +1,140 @@
>+/* Quagga signal handling functions.
>+ * Copyright (C) 2004 Paul Jakma,
>+ *
>+ * This file is part of Quagga.
>+ *
>+ * Quagga is free software; you can redistribute it and/or modify it
>+ * under the terms of the GNU General Public License as published by the
>+ * Free Software Foundation; either version 2, or (at your option) any
>+ * later version.
>+ *
>+ * Quagga is distributed in the hope that it will be useful, but
>+ * WITHOUT ANY WARRANTY; without even the implied warranty of
>+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
>+ * General Public License for more details.
>+ *
>+ * You should have received a copy of the GNU General Public License
>+ * along with Quagga; see the file COPYING. If not, write to the Free
>+ * Software Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA
>+ * 02111-1307, USA.
>+ */
>+
>+#include <zebra.h>
>+#include <sigevent.h>
>+
>+struct quagga_sigevent_master_t
>+{
>+ struct thread_master *tm;
>+ struct thread *t;
>+
>+ struct quagga_signal_t *signals;
>+ int sigc;
>+
>+} sigmaster;
>+
>+/* Generic signal handler
>+ * Schedules signal event thread
>+ */
>+void
>+quagga_signal_handler (int signo)
>+{
>+ int i;
>+ struct quagga_signal_t *sig;
>+
>+ for (i = 0; i < sigmaster.sigc; i++)
>+ {
>+ sig = &(sigmaster.signals[i]);
>+
>+ if (sig->signal == signo)
>+ sig->caught++;
>+ }
>+}
>+
>+int
>+quagga_signal_timer (struct thread *t)
>+{
>+ sigset_t newmask, oldmask;
>+ struct quagga_sigevent_master_t *sigm;
>+ struct quagga_signal_t *sig;
>+ int i;
>+
>+ /* block all signals */
>+
>+ sigfillset (&newmask);
>+
>+ sigm = THREAD_ARG (t);
>+
>+ for (i = 0; i < sigm->sigc; i++)
>+ {
>+ sig = &(sigm->signals[i]);
>+ if (sig->caught > 0)
>+ {
>+ sig->caught = 0;
>+ sig->handler();
>+ }
>+ }
>+
>+ sigm->t = thread_add_timer (sigm->tm, quagga_signal_timer, &sigmaster,
>+ QUAGGA_SIGNAL_TIMER_INTERVAL);
>+
>+ if ( sigprocmask (SIG_UNBLOCK, &oldmask, NULL) < 0 );
>+ return -1;
>+
>+ return 0;
>+}
>+
>+/* Initialization of signal handles. */
>+/* Signale wrapper. */
>+int
>+signal_set (int signo)
>+{
>+ int ret;
>+ struct sigaction sig;
>+ struct sigaction osig;
>+
>+ sig.sa_handler = &quagga_signal_handler;
>+ sigfillset (&sig.sa_mask);
>+ sig.sa_flags = 0;
>+ if (signo == SIGALRM) {
>+#ifdef SA_INTERRUPT
>+ sig.sa_flags |= SA_INTERRUPT; /* SunOS */
>+#endif
>+ } else {
>+#ifdef SA_RESTART
>+ sig.sa_flags |= SA_RESTART;
>+#endif /* SA_RESTART */
>+ }
>+
>+ ret = sigaction (signo, &sig, &osig);
>+ if (ret < 0)
>+ return ret;
>+ else
>+ return 0;
>+}
>+
>+void
>+signal_init (struct thread_master *m,
>+ int sigc, struct quagga_signal_t signals[])
>+{
>+
>+ int i = 0;
>+ struct quagga_signal_t *sig;
>+
>+ while (i < sigc)
>+ {
>+ sig = &signals[i];
>+ if ( signal_set (sig->signal) < 0 )
>+ exit (-1);
>+ i++;
>+ }
>+
>+ sigmaster.sigc = sigc;
>+ sigmaster.signals = signals;
>+ sigmaster.tm = m;
>+
>+ sigmaster.t =
>+ thread_add_timer (m, quagga_signal_timer, &sigmaster,
>+ QUAGGA_SIGNAL_TIMER_INTERVAL);
>+
>+}
>+
>Index: lib/sigevent.h
>===================================================================
>RCS file: lib/sigevent.h
>diff -N lib/sigevent.h
>--- /dev/null 1 Jan 1970 00:00:00 -0000
>+++ lib/sigevent.h 16 Jan 2004 06:20:18 -0000
>@@ -0,0 +1,41 @@
>+/*
>+ * Quagga Signal handling header.
>+ *
>+ * Copyright (C) 2004 Paul Jakma.
>+ *
>+ * This file is part of Quagga.
>+ *
>+ * Quagga is free software; you can redistribute it and/or modify it
>+ * under the terms of the GNU General Public License as published by the
>+ * Free Software Foundation; either version 2, or (at your option) any
>+ * later version.
>+ *
>+ * Quagga is distributed in the hope that it will be useful, but
>+ * WITHOUT ANY WARRANTY; without even the implied warranty of
>+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
>+ * General Public License for more details.
>+ *
>+ * You should have received a copy of the GNU General Public License
>+ * along with Quagga; see the file COPYING. If not, write to the Free
>+ * Software Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA
>+ * 02111-1307, USA.
>+ */
>+
>+#ifndef _QUAGGA_SIGNAL_H
>+#define _QUAGGA_SIGNAL_H
>+#include <thread.h>
>+
>+#define QUAGGA_SIGNAL_TIMER_INTERVAL 2L
>+#define Q_SIGC(sig) (sizeof(sig)/sizeof(sig[0]))
>+
>+struct quagga_signal_t
>+{
>+ int signal;
>+ void (*handler) (void);
>+ volatile sig_atomic_t caught;
>+};
>+
>+void signal_init (struct thread_master *m, int sigc,
>+ struct quagga_signal_t signals[]);
>+
>+#endif /* _QUAGGA_SIGNAL_H */
>Index: ospfd/ospf_main.c
>===================================================================
>RCS file: /var/cvsroot/quagga/ospfd/ospf_main.c,v
>retrieving revision 1.9
>diff -u -r1.9 ospf_main.c
>--- ospfd/ospf_main.c 14 Aug 2003 05:32:12 -0000 1.9
>+++ ospfd/ospf_main.c 16 Jan 2004 06:20:18 -0000
>@@ -38,6 +38,7 @@
> #include "memory.h"
> #include "privs.h"
> #include "debug.h"
>+#include "sigevent.h"
>
> #include "ospfd/ospfd.h"
> #include "ospfd/ospf_interface.h"
>@@ -125,14 +126,14 @@
>
> /* SIGHUP handler. */
> void
>-sighup (int sig)
>+sighup (void)
> {
> zlog (NULL, LOG_INFO, "SIGHUP received");
> }
>
> /* SIGINT handler. */
> void
>-sigint (int sig)
>+sigint (void)
> {
> zlog (NULL, LOG_INFO, "Terminating on signal");
>
>@@ -143,58 +144,26 @@
>
> /* SIGUSR1 handler. */
> void
>-sigusr1 (int sig)
>+sigusr1 (void)
> {
> zlog_rotate (NULL);
> }
>
>-/* Signal wrapper. */
>-RETSIGTYPE *
>-signal_set (int signo, void (*func)(int))
>-{
>- int ret;
>- struct sigaction sig;
>- struct sigaction osig;
>-
>- sig.sa_handler = func;
>- sigemptyset (&sig.sa_mask);
>- sig.sa_flags = 0;
>-#ifdef SA_RESTART
>- sig.sa_flags |= SA_RESTART;
>-#endif /* SA_RESTART */
>-
>- ret = sigaction (signo, &sig, &osig);
>-
>- if (ret < 0)
>- return (SIG_ERR);
>- else
>- return (osig.sa_handler);
>-}
>-
>-/* Initialization of signal handles. */
>-void
>-signal_init ()
>+struct quagga_signal_t ospf_signals[] =
> {
>- signal_set (SIGHUP, sighup);
>- signal_set (SIGINT, sigint);
>- signal_set (SIGTERM, sigint);
>- signal_set (SIGPIPE, SIG_IGN);
>-#ifdef SIGTSTP
>- signal_set (SIGTSTP, SIG_IGN);
>-#endif
>-#ifdef SIGTTIN
>- signal_set (SIGTTIN, SIG_IGN);
>-#endif
>-#ifdef SIGTTOU
>- signal_set (SIGTTOU, SIG_IGN);
>-#endif
>- signal_set (SIGUSR1, sigusr1);
>-#ifdef HAVE_GLIBC_BACKTRACE
>- signal_set (SIGBUS, debug_print_trace);
>- signal_set (SIGSEGV, debug_print_trace);
>- signal_set (SIGILL, debug_print_trace);
>-#endif /* HAVE_GLIBC_BACKTRACE */
>-}
>+ {
>+ .signal = SIGHUP,
>+ .handler = &sighup,
>+ },
>+ {
>+ .signal = SIGUSR1,
>+ .handler = &sigusr1,
>+ },
>+ {
>+ .signal = SIGINT,
>+ .handler = &sigint,
>+ },
>+};
>
> /* OSPFd main routine. */
> int
>@@ -285,7 +254,7 @@
>
> /* Library inits. */
> zprivs_init (&ospfd_privs);
>- signal_init ();
>+ signal_init (master, Q_SIGC(ospf_signals), ospf_signals);
> cmd_init (1);
> debug_init ();
> vty_init (master);
>Index: ripd/rip_main.c
>===================================================================
>RCS file: /var/cvsroot/quagga/ripd/rip_main.c,v
>retrieving revision 1.6
>diff -u -r1.6 rip_main.c
>--- ripd/rip_main.c 14 Aug 2003 05:32:12 -0000 1.6
>+++ ripd/rip_main.c 16 Jan 2004 06:20:18 -0000
>@@ -31,6 +31,7 @@
> #include "keychain.h"
> #include "log.h"
> #include "privs.h"
>+#include "sigevent.h"
>
> #include "ripd/ripd.h"
>
>@@ -120,32 +121,9 @@
> exit (status);
> }
>
>-/* Signale wrapper. */
>-RETSIGTYPE *
>-signal_set (int signo, void (*func)(int))
>-{
>- int ret;
>- struct sigaction sig;
>- struct sigaction osig;
>-
>- sig.sa_handler = func;
>- sigemptyset (&sig.sa_mask);
>- sig.sa_flags = 0;
>-#ifdef SA_RESTART
>- sig.sa_flags |= SA_RESTART;
>-#endif /* SA_RESTART */
>-
>- ret = sigaction (signo, &sig, &osig);
>-
>- if (ret < 0)
>- return (SIG_ERR);
>- else
>- return (osig.sa_handler);
>-}
>-
> /* SIGHUP handler. */
> void
>-sighup (int sig)
>+sighup (void)
> {
> zlog_info ("SIGHUP received");
> rip_clean ();
>@@ -163,7 +141,7 @@
>
> /* SIGINT handler. */
> void
>-sigint (int sig)
>+sigint (void)
> {
> zlog (NULL, LOG_INFO, "Terminating on signal");
>
>@@ -175,21 +153,26 @@
>
> /* SIGUSR1 handler. */
> void
>-sigusr1 (int sig)
>+sigusr1 (void)
> {
> zlog_rotate (NULL);
> }
>
>-/* Initialization of signal handles. */
>-void
>-signal_init ()
>+struct quagga_signal_t ripd_signals[] =
> {
>- signal_set (SIGHUP, sighup);
>- signal_set (SIGINT, sigint);
>- signal_set (SIGTERM, sigint);
>- signal_set (SIGPIPE, SIG_IGN);
>- signal_set (SIGUSR1, sigusr1);
>-}
>+ {
>+ .signal = SIGHUP,
>+ .handler = &sighup,
>+ },
>+ {
>+ .signal = SIGUSR1,
>+ .handler = &sigusr1,
>+ },
>+ {
>+ .signal = SIGINT,
>+ .handler = &sigusr1,
>+ },
>+};
>
> /* Main routine of ripd. */
> int
>@@ -271,7 +254,7 @@
>
> /* Library initialization. */
> zprivs_init (&ripd_privs);
>- signal_init ();
>+ signal_init (master, Q_SIGC(ripd_signals), ripd_signals);
> cmd_init (1);
> vty_init (master);
> memory_init ();
>Index: ripngd/ripng_main.c
>===================================================================
>RCS file: /var/cvsroot/quagga/ripngd/ripng_main.c,v
>retrieving revision 1.7
>diff -u -r1.7 ripng_main.c
>--- ripngd/ripng_main.c 14 Aug 2003 05:32:12 -0000 1.7
>+++ ripngd/ripng_main.c 16 Jan 2004 06:20:18 -0000
>@@ -33,6 +33,7 @@
> #include "prefix.h"
> #include "if.h"
> #include "privs.h"
>+#include "sigevent.h"
>
> #include "ripngd/ripngd.h"
>
>@@ -126,7 +127,7 @@
>
> /* SIGHUP handler. */
> void
>-sighup (int sig)
>+sighup (void)
> {
> zlog_info ("SIGHUP received");
> ripng_clean ();
>@@ -143,7 +144,7 @@
>
> /* SIGINT handler. */
> void
>-sigint (int sig)
>+sigint (void)
> {
> zlog_info ("Terminating on signal");
>
>@@ -155,44 +156,26 @@
>
> /* SIGUSR1 handler. */
> void
>-sigusr1 (int sig)
>+sigusr1 (void)
> {
> zlog_rotate (NULL);
> }
>
>-/* Signale wrapper. */
>-RETSIGTYPE *
>-signal_set (int signo, void (*func)(int))
>-{
>- int ret;
>- struct sigaction sig;
>- struct sigaction osig;
>-
>- sig.sa_handler = func;
>- sigemptyset (&sig.sa_mask);
>- sig.sa_flags = 0;
>-#ifdef SA_RESTART
>- sig.sa_flags |= SA_RESTART;
>-#endif /* SA_RESTART */
>-
>- ret = sigaction (signo, &sig, &osig);
>-
>- if (ret < 0)
>- return (SIG_ERR);
>- else
>- return (osig.sa_handler);
>-}
>-
>-/* Initialization of signal handles. */
>-void
>-signal_init ()
>+struct quagga_signal_t ripng_signals[] =
> {
>- signal_set (SIGHUP, sighup);
>- signal_set (SIGINT, sigint);
>- signal_set (SIGTERM, sigint);
>- signal_set (SIGPIPE, SIG_IGN);
>- signal_set (SIGUSR1, sigusr1);
>-}
>+ {
>+ .signal = SIGHUP,
>+ .handler = &sighup,
>+ },
>+ {
>+ .signal = SIGUSR1,
>+ .handler = &sigusr1,
>+ },
>+ {
>+ .signal = SIGINT,
>+ .handler = &sigint,
>+ },
>+};
>
> /* RIPngd main routine. */
> int
>@@ -275,7 +258,7 @@
>
> /* Library inits. */
> zprivs_init (&ripngd_privs);
>- signal_init ();
>+ signal_init (master, Q_SIGC(ripng_signals), ripng_signals);
> cmd_init (1);
> vty_init (master);
> memory_init ();
>Index: zebra/main.c
>===================================================================
>RCS file: /var/cvsroot/quagga/zebra/main.c,v
>retrieving revision 1.7
>diff -u -r1.7 main.c
>--- zebra/main.c 14 Aug 2003 05:32:12 -0000 1.7
>+++ zebra/main.c 16 Jan 2004 06:20:19 -0000
>@@ -30,6 +30,7 @@
> #include "prefix.h"
> #include "log.h"
> #include "privs.h"
>+#include "sigevent.h"
>
> #include "zebra/rib.h"
> #include "zebra/zserv.h"
>@@ -131,7 +132,7 @@
>
> /* SIGHUP handler. */
> void
>-sighup (int sig)
>+sighup (void)
> {
> zlog_info ("SIGHUP received");
>
>@@ -141,7 +142,7 @@
>
> /* SIGINT handler. */
> void
>-sigint (int sig)
>+sigint (void)
> {
> /* Decrared in rib.c */
> void rib_close ();
>@@ -156,44 +157,26 @@
>
> /* SIGUSR1 handler. */
> void
>-sigusr1 (int sig)
>+sigusr1 (void)
> {
> zlog_rotate (NULL);
> }
>
>-/* Signale wrapper. */
>-RETSIGTYPE *
>-signal_set (int signo, void (*func)(int))
>+struct quagga_signal_t zebra_signals[] =
> {
>- int ret;
>- struct sigaction sig;
>- struct sigaction osig;
>-
>- sig.sa_handler = func;
>- sigemptyset (&sig.sa_mask);
>- sig.sa_flags = 0;
>-#ifdef SA_RESTART
>- sig.sa_flags |= SA_RESTART;
>-#endif /* SA_RESTART */
>-
>- ret = sigaction (signo, &sig, &osig);
>-
>- if (ret < 0)
>- return (SIG_ERR);
>- else
>- return (osig.sa_handler);
>-}
>-
>-/* Initialization of signal handles. */
>-void
>-signal_init ()
>-{
>- signal_set (SIGHUP, sighup);
>- signal_set (SIGINT, sigint);
>- signal_set (SIGTERM, sigint);
>- signal_set (SIGPIPE, SIG_IGN);
>- signal_set (SIGUSR1, sigusr1);
>-}
>+ {
>+ .signal = SIGHUP,
>+ .handler = &sighup,
>+ },
>+ {
>+ .signal = SIGUSR1,
>+ .handler = &sigusr1,
>+ },
>+ {
>+ .signal = SIGINT,
>+ .handler = &sigusr1,
>+ },
>+};
>
> /* Main startup routine. */
> int
>@@ -289,7 +272,7 @@
> zprivs_init (&zserv_privs);
>
> /* Vty related initialize. */
>- signal_init ();
>+ signal_init (zebrad.master, Q_SIGC(zebra_signals), zebra_signals);
> cmd_init (1);
> vty_init (zebrad.master);
> memory_init ();
>
>
>------------------------------------------------------------------------
>
>_______________________________________________
>Quagga-dev mailing list
>Quagga-dev@lists.quagga.net
>http://lists.quagga.net/mailman/listinfo/quagga-dev
>
>
Re: [patch] move signal handlers out to threads [ In reply to ]
On Fri, 16 Jan 2004, Vincent Jardin wrote:

> It is a very good idea. It should fix many random issues with the
> signals. The code and the architecture of the patch seems OK.

Yes, I've tested it with a standalone test case (which i might commit
too - could be useful to start collecting little regression test utils)
and it works well.

One question though is whether _NSIG is portable, it doesnt appear to
be, nor does Steven's make mention of any portable way to retrieve the
maximum number of signals. Anyone know how to do this (other than by
defining _NSIG ourselves for each system)? (there doesnt appear to be a
sysconf for it either).

Also, somehow the patch appears to be missing the sigprocmask in the
timer handler to block all signals.

> Regards,
> Vincent

regards,

--paulj
Re: [patch] move signal handlers out to threads [ In reply to ]
On Fri, 16 Jan 2004, Vincent Jardin wrote:

> Right, it depends of the OS:

ah fun.

> on a BSD: #define NSIG 32 /* number of old signals
> (counting 0) */
> see sys/signal.h

ok, useful.

> When it is not defined, I think that you can assume that it is 32.
> However NSIG is often used without any issues.

Ok.

> Could the following strategy be used with Zebra:
>
> lib/thread.c:
> ...
> sigprocmask(SIG_UNBLOCK, &sigs, NULL);
> num = select (FD_SETSIZE, &readfd, &writefd, &exceptfd, timer_wait);
> sigprocmask(SIG_BLOCK, &sigs, NULL);
> ...
>
> +
>
> Then a signal is triggered only during the select() calls. It matches
> the Quagga threading architecture with which a schuduling should only
> happen after the select() call based on:
> - read, write, oob fd
> - timer
> - then signal (with this patch)

Hmmm... possibly. However, signal handled via timer means signals will
be reliably handled. It'd be annoying if your logs didnt get rotated
because <daemon> was busy with IO.

> Regards,
> Vincent

--paulj
Re: [patch] move signal handlers out to threads [ In reply to ]
Paul Jakma wrote:

>On Fri, 16 Jan 2004, Vincent Jardin wrote:
>
>
>
>>It is a very good idea. It should fix many random issues with the
>>signals. The code and the architecture of the patch seems OK.
>>
>>
>
>Yes, I've tested it with a standalone test case (which i might commit
>too - could be useful to start collecting little regression test utils)
>and it works well.
>
>One question though is whether _NSIG is portable, it doesnt appear to
>be, nor does Steven's make mention of any portable way to retrieve the
>maximum number of signals. Anyone know how to do this (other than by
>defining _NSIG ourselves for each system)? (there doesnt appear to be a
>sysconf for it either).
>
Right, it depends of the OS:
on a BSD: #define NSIG 32 /* number of old signals
(counting 0) */
see sys/signal.h

When it is not defined, I think that you can assume that it is 32.
However NSIG is often used without any issues.


>Also, somehow the patch appears to be missing the sigprocmask in the
>timer handler to block all signals.
>
Could the following strategy be used with Zebra:

lib/thread.c:
...
sigprocmask(SIG_UNBLOCK, &sigs, NULL);
num = select (FD_SETSIZE, &readfd, &writefd, &exceptfd, timer_wait);
sigprocmask(SIG_BLOCK, &sigs, NULL);
...

+

Then a signal is triggered only during the select() calls. It matches
the Quagga threading architecture with which a schuduling should only
happen after the select() call based on:
- read, write, oob fd
- timer
- then signal (with this patch)

Regards,
Vincent
Re: [patch] move signal handlers out to threads [ In reply to ]
+ for (i = 0; i < sigmaster.sigc; i++)
+ {
+ sig = &(sigmaster.signals[i]);
+
+ if (sig->signal == signo)
+ sig->caught++;
+ }

+ for (i = 0; i < sigm->sigc; i++)
+ {
+ sig = &(sigm->signals[i]);
+ if (sig->caught > 0)
+ {
+ sig->caught = 0;
+ sig->handler();
+ }
+ }

And no break because you want to allow two handlers for the same
signal? Cool, but subtle enough to need a comment.

--
Greg Troxel <gdt@ir.bbn.com>
Re: [patch] move signal handlers out to threads [ In reply to ]
Hi Paul & Vincent,

> It is a very good idea. It should fix many random issues with the
> signals. The code and the architecture of the patch seems OK.

Yes, this sounds like a very good idea to me too!
And I think I will implement the same in SRRD...

Cheers
- Amir
--
Amir Guindehi, nospam.amir@datacore.ch
DataCore GmbH, Witikonerstrasse 289, 8053 Zurich, Switzerland