Discussion:
[PATCH] Factor out dhcpd's and syslogd's main loops into a library function
(too old to reply)
Felix Janda
2013-09-01 13:54:27 UTC
Permalink
Both syslogd and dhcpd wait for sockets, signals or a timeout. Currently,
they use select() with its timeout to listen to the sockets and a
self-pipe for the signals.

The new library function instead uses pselect() without a timeout and
alarm(). With pselect() signals are only unblocked during this system
call; so the xwrappers don't need to be aware about interrupted syscalls.
alarm() is used instead of the timeout argument to pselect() so that
the length of the intervals between timeouts is not affected by being
interrupted.

On SIGHUP syslogd restarts. Previously everything relevant was contained
in syslogd_main() and the restarting was implemented by a goto. Now a
longjmp() would be necessary. I instead used an xexec(). Since xexec()
doesn't change the pid the pidfile needs to be unlinked() before
terminating so that xpidfile() doesn't complain. Furthermore if not run
in the foreground syslogd will daemonize() again when restarting. Could
one detect in daemonize() whether daemonizing is necessary?

Felix

# HG changeset patch
# User Felix Janda <felix.janda at posteo.de>
# Date 1378041908 -7200
# Node ID 752d22ece0fe3fa9763fd0b6e231e653cc173890
# Parent 587b7572aac2a64988503458155e1f2546672525
Factor out daemon select() loops into a library function

diff -r 587b7572aac2 -r 752d22ece0fe lib/lib.h
--- a/lib/lib.h Sun Sep 01 15:13:58 2013 +0200
+++ b/lib/lib.h Sun Sep 01 15:25:08 2013 +0200
@@ -201,4 +201,7 @@
char *astrcat (char *, char *);
char *xastrcat (char *, char *);

+// daemon helper functions
void daemonize(void);
+void daemon_main_loop(int nfds, fd_set *readfds, long timeout, int *signals,
+ void (*sighandler)(int), void (*selecthandler)(fd_set *fds));
diff -r 587b7572aac2 -r 752d22ece0fe lib/pending.c
--- a/lib/pending.c Sun Sep 01 15:13:58 2013 +0200
+++ b/lib/pending.c Sun Sep 01 15:25:08 2013 +0200
@@ -92,3 +92,41 @@
dup2(fd, 2);
if (fd > 2) close(fd);
}
+
+static int daemon_sig;
+
+static void daemon_sighandler(int sig)
+{
+ daemon_sig = sig;
+}
+
+/*
+ * main loop for daemons listening to signals and file handles
+*/
+void daemon_main_loop(int nfds, fd_set *readfds, long timeout, int *signals,
+ void (*sighandler)(int), void (*selecthandler)(fd_set *fds))
+{
+ fd_set copy;
+ sigset_t sigmask;
+ struct sigaction action;
+ int *i, ret;
+
+ sigemptyset(&sigmask);
+ action.sa_handler = daemon_sighandler;
+ sigemptyset(&action.sa_mask);
+ action.sa_flags = 0;
+ if (timeout) alarm(timeout);
+ for (i = signals; *i; i++) sigaddset(&sigmask, *i);
+ sigprocmask(SIG_SETMASK, &sigmask, 0);
+ for (i = signals; *i; i++) sigaction(*i, &action, 0);
+
+ for (;;) {
+ memcpy(&copy, &readfds, sizeof(copy));
+ ret = pselect(nfds, &copy, 0, 0, 0, &action.sa_mask);
+ if (ret == -1) {
+ if (errno != EINTR) error_exit ("pselect");
+ if (timeout && daemon_sig == SIGALRM) alarm(timeout);
+ sighandler(daemon_sig);
+ } else selecthandler(&copy);
+ }
+}
diff -r 587b7572aac2 -r 752d22ece0fe toys/pending/dhcpd.c
--- a/toys/pending/dhcpd.c Sun Sep 01 15:13:58 2013 +0200
+++ b/toys/pending/dhcpd.c Sun Sep 01 15:25:08 2013 +0200
@@ -206,11 +206,9 @@
{"msstaticroutes" , DHCP_STCRTS | 0xf9, NULL, 0},
};

-struct fd_pair { int rd; int wr; };
static server_config_t gconfig;
static server_state_t gstate;
static uint8_t infomode;
-static struct fd_pair sigfd;
static int constone = 1;

// calculate options size.
@@ -316,29 +314,6 @@
}
}

-// Generic signal handler real handling is done in main funcrion.
-static void signal_handler(int sig)
-{
- unsigned char ch = sig;
- if (write(sigfd.wr, &ch, 1) != 1) dbg("can't send signal\n");
-}
-
-// signal setup for SIGUSR1 SIGTERM
-static int setup_signal()
-{
- if (pipe((int *)&sigfd) < 0) {
- dbg("signal pipe failed\n");
- return -1;
- }
- fcntl(sigfd.wr , F_SETFD, FD_CLOEXEC);
- fcntl(sigfd.rd , F_SETFD, FD_CLOEXEC);
- int flags = fcntl(sigfd.wr, F_GETFL);
- fcntl(sigfd.wr, F_SETFL, flags | O_NONBLOCK);
- signal(SIGUSR1, signal_handler);
- signal(SIGTERM, signal_handler);
- return 0;
-}
-
// String STR to UINT32 conversion strored in VAR
static int strtou32(const char *str, void *var)
{
@@ -1073,15 +1048,114 @@
return ret;
}

+static void sig_handler(int sig)
+{
+ switch (sig) {
+ case SIGALRM:
+ write_leasefile();
+ if (get_interface(gconfig.interface, &gconfig.ifindex, &gconfig.server_nip, gconfig.server_mac)<0)
+ perror_exit("Interface lost %s\n", gconfig.interface);
+ gconfig.server_nip = htonl(gconfig.server_nip);
+ break;
+ case SIGUSR1:
+ infomsg(infomode, "Received SIGUSR1");
+ write_leasefile();
+ break;
+ case SIGTERM:
+ infomsg(infomode, "Received SIGTERM");
+ write_leasefile();
+ unlink(gconfig.pidfile);
+ exit(0);
+ break;
+ }
+}
+
+static void select_handler(fd_set *rfds)
+{
+ uint8_t *optptr, msgtype = 0;
+ uint32_t serverid = 0, requested_nip = 0;
+ uint32_t reqested_lease = 0;
+ char *hstname = NULL;
+
+ dbg("select listen sock read\n");
+ if (read_packet() < 0) {
+ open_listensock();
+ return;
+ }
+ get_optval((uint8_t*)&gstate.rcvd_pkt.options, DHCP_OPT_MESSAGE_TYPE, &gstate.rqcode);
+ if (gstate.rqcode == 0 || gstate.rqcode < DHCPDISCOVER
+ || gstate.rqcode > DHCPINFORM) {
+ dbg("no or bad message type option, ignoring packet.\n");
+ return;
+ }
+ get_optval((uint8_t*) &gstate.rcvd_pkt.options, DHCP_OPT_SERVER_ID, &serverid);
+ if (serverid && (serverid != gconfig.server_nip)) {
+ dbg("server ID doesn't match, ignoring packet.\n");
+ return;
+ }
+ switch (gstate.rqcode) {
+ case DHCPDISCOVER:
+ msgtype = DHCPOFFER;
+ dbg("Message Type : DHCPDISCOVER\n");
+ get_optval((uint8_t*) &gstate.rcvd_pkt.options, DHCP_OPT_REQUESTED_IP, &requested_nip);
+ get_optval((uint8_t*) &gstate.rcvd_pkt.options, DHCP_OPT_HOST_NAME, &hstname);
+ reqested_lease = gconfig.offer_time;
+ get_reqparam(&gstate.rqopt);
+ optptr = prepare_send_pkt();
+ gstate.send_pkt.yiaddr = getip_from_pool(requested_nip, gstate.rcvd_pkt.chaddr, &reqested_lease, hstname);
+ if(!gstate.send_pkt.yiaddr){
+ msgtype = DHCPNAK;
+ optptr = set_optval(optptr, DHCP_OPT_MESSAGE_TYPE, &msgtype, 1);
+ send_packet(1);
+ break;
+ }
+ get_optval((uint8_t*) &gstate.rcvd_pkt.options, DHCP_OPT_LEASE_TIME, &reqested_lease);
+ reqested_lease = htonl(get_lease(reqested_lease + time(NULL)));
+ optptr = set_optval(optptr, DHCP_OPT_MESSAGE_TYPE, &msgtype, 1);
+ optptr = set_optval(optptr, DHCP_OPT_SERVER_ID, &gconfig.server_nip, 4);
+ optptr = set_optval(optptr, DHCP_OPT_LEASE_TIME, &reqested_lease, 4);
+ optptr = set_reqparam(optptr, gstate.rqopt);
+ send_packet(1);
+ break;
+ case DHCPREQUEST:
+ msgtype = DHCPACK;
+ dbg("Message Type : DHCPREQUEST\n");
+ optptr = prepare_send_pkt();
+ get_optval((uint8_t*) &gstate.rcvd_pkt.options, DHCP_OPT_REQUESTED_IP, &requested_nip);
+ get_optval((uint8_t*) &gstate.rcvd_pkt.options, DHCP_OPT_LEASE_TIME, &reqested_lease);
+ get_optval((uint8_t*) &gstate.rcvd_pkt.options, DHCP_OPT_HOST_NAME, &hstname);
+ gstate.send_pkt.yiaddr = getip_from_pool(requested_nip, gstate.rcvd_pkt.chaddr, &reqested_lease, hstname);
+ if (!serverid) reqested_lease = gconfig.max_lease_sec;
+ if (!gstate.send_pkt.yiaddr) {
+ msgtype = DHCPNAK;
+ optptr = set_optval(optptr, DHCP_OPT_MESSAGE_TYPE, &msgtype, 1);
+ send_packet(1);
+ break;
+ }
+ optptr = set_optval(optptr, DHCP_OPT_MESSAGE_TYPE, &msgtype, 1);
+ optptr = set_optval(optptr, DHCP_OPT_SERVER_ID, &gconfig.server_nip, 4);
+ reqested_lease = htonl(reqested_lease);
+ optptr = set_optval(optptr, DHCP_OPT_LEASE_TIME, &reqested_lease, 4);
+ send_packet(1);
+ write_leasefile();
+ break;
+ case DHCPDECLINE:// FALL THROUGH
+ case DHCPRELEASE:
+ dbg("Message Type : DHCPDECLINE or DHCPRELEASE \n");
+ get_optval((uint8_t*) &gstate.rcvd_pkt.options, DHCP_OPT_SERVER_ID, &serverid);
+ if (serverid != gconfig.server_nip) break;
+ get_optval((uint8_t*) &gstate.rcvd_pkt.options, DHCP_OPT_REQUESTED_IP, &requested_nip);
+ delip_from_lease(requested_nip, gstate.rcvd_pkt.chaddr, (gstate.rqcode==DHCPRELEASE)?0:gconfig.decline_time);
+ break;
+ default:
+ dbg("Message Type : %u\n", gstate.rqcode);
+ break;
+ }
+}
+
void dhcpd_main(void)
{
- struct timeval tv;
- int retval;
- uint8_t *optptr, msgtype = 0;
- uint32_t waited = 0, serverid = 0, requested_nip = 0;
- uint32_t reqested_lease = 0, ip_pool_size = 0;
- char *hstname = NULL;
- fd_set rfds;
+ uint32_t ip_pool_size;

infomode = LOG_CONSOLE;
if (!(flag_chk(FLAG_f))) {
@@ -1111,137 +1185,15 @@
perror_exit("Failed to get interface %s", gconfig.interface);
gconfig.server_nip = htonl(gconfig.server_nip);

- setup_signal();
open_listensock();
fcntl(gstate.listensock, F_SETFD, FD_CLOEXEC);

- for (;;) {
- uint32_t timestmp = time(NULL);
- FD_ZERO(&rfds);
- FD_SET(gstate.listensock, &rfds);
- FD_SET(sigfd.rd, &rfds);
- tv.tv_sec = gconfig.auto_time - waited;
- tv.tv_usec = 0;
- retval = 0;
- serverid = 0;
- msgtype = 0;
+ fd_set rfds;
+ FD_ZERO(&rfds);
+ FD_SET(gstate.listensock, &rfds);

- int maxfd = (sigfd.rd > gstate.listensock)? sigfd.rd : gstate.listensock;
- dbg("select waiting ....\n");
- retval = select(maxfd + 1, &rfds, NULL, NULL, (gconfig.auto_time?&tv:NULL));
- if (retval < 0) {
- if (errno == EINTR) {
- waited += (unsigned) time(NULL) - timestmp;
- continue;
- }
- dbg("Error in select wait again...\n");
- continue;
- }
- if (!retval) { // Timed out
- dbg("select wait Timed Out...\n");
- waited = 0;
- write_leasefile();
- if (get_interface(gconfig.interface, &gconfig.ifindex, &gconfig.server_nip, gconfig.server_mac)<0)
- perror_exit("Interface lost %s\n", gconfig.interface);
- gconfig.server_nip = htonl(gconfig.server_nip);
- continue;
- }
- if (FD_ISSET(sigfd.rd, &rfds)) { // Some Activity on RDFDs : is signal
- unsigned char sig;
- if (read(sigfd.rd, &sig, 1) != 1) {
- dbg("signal read failed.\n");
- continue;
- }
- switch (sig) {
- case SIGUSR1:
- infomsg(infomode, "Received SIGUSR1");
- write_leasefile();
- continue;
- case SIGTERM:
- infomsg(infomode, "Received SIGTERM");
- write_leasefile();
- unlink(gconfig.pidfile);
- exit(0);
- break;
- default: break;
- }
- }
- if (FD_ISSET(gstate.listensock, &rfds)) { // Some Activity on RDFDs : is socket
- dbg("select listen sock read\n");
- if (read_packet() < 0) {
- open_listensock();
- continue;
- }
- waited += time(NULL) - timestmp;
- get_optval((uint8_t*)&gstate.rcvd_pkt.options, DHCP_OPT_MESSAGE_TYPE, &gstate.rqcode);
- if (gstate.rqcode == 0 || gstate.rqcode < DHCPDISCOVER
- || gstate.rqcode > DHCPINFORM) {
- dbg("no or bad message type option, ignoring packet.\n");
- continue;
- }
- get_optval((uint8_t*) &gstate.rcvd_pkt.options, DHCP_OPT_SERVER_ID, &serverid);
- if (serverid && (serverid != gconfig.server_nip)) {
- dbg("server ID doesn't match, ignoring packet.\n");
- continue;
- }
- switch (gstate.rqcode) {
- case DHCPDISCOVER:
- msgtype = DHCPOFFER;
- dbg("Message Type : DHCPDISCOVER\n");
- get_optval((uint8_t*) &gstate.rcvd_pkt.options, DHCP_OPT_REQUESTED_IP, &requested_nip);
- get_optval((uint8_t*) &gstate.rcvd_pkt.options, DHCP_OPT_HOST_NAME, &hstname);
- reqested_lease = gconfig.offer_time;
- get_reqparam(&gstate.rqopt);
- optptr = prepare_send_pkt();
- gstate.send_pkt.yiaddr = getip_from_pool(requested_nip, gstate.rcvd_pkt.chaddr, &reqested_lease, hstname);
- if(!gstate.send_pkt.yiaddr){
- msgtype = DHCPNAK;
- optptr = set_optval(optptr, DHCP_OPT_MESSAGE_TYPE, &msgtype, 1);
- send_packet(1);
- break;
- }
- get_optval((uint8_t*) &gstate.rcvd_pkt.options, DHCP_OPT_LEASE_TIME, &reqested_lease);
- reqested_lease = htonl(get_lease(reqested_lease + time(NULL)));
- optptr = set_optval(optptr, DHCP_OPT_MESSAGE_TYPE, &msgtype, 1);
- optptr = set_optval(optptr, DHCP_OPT_SERVER_ID, &gconfig.server_nip, 4);
- optptr = set_optval(optptr, DHCP_OPT_LEASE_TIME, &reqested_lease, 4);
- optptr = set_reqparam(optptr, gstate.rqopt);
- send_packet(1);
- break;
- case DHCPREQUEST:
- msgtype = DHCPACK;
- dbg("Message Type : DHCPREQUEST\n");
- optptr = prepare_send_pkt();
- get_optval((uint8_t*) &gstate.rcvd_pkt.options, DHCP_OPT_REQUESTED_IP, &requested_nip);
- get_optval((uint8_t*) &gstate.rcvd_pkt.options, DHCP_OPT_LEASE_TIME, &reqested_lease);
- get_optval((uint8_t*) &gstate.rcvd_pkt.options, DHCP_OPT_HOST_NAME, &hstname);
- gstate.send_pkt.yiaddr = getip_from_pool(requested_nip, gstate.rcvd_pkt.chaddr, &reqested_lease, hstname);
- if (!serverid) reqested_lease = gconfig.max_lease_sec;
- if (!gstate.send_pkt.yiaddr) {
- msgtype = DHCPNAK;
- optptr = set_optval(optptr, DHCP_OPT_MESSAGE_TYPE, &msgtype, 1);
- send_packet(1);
- break;
- }
- optptr = set_optval(optptr, DHCP_OPT_MESSAGE_TYPE, &msgtype, 1);
- optptr = set_optval(optptr, DHCP_OPT_SERVER_ID, &gconfig.server_nip, 4);
- reqested_lease = htonl(reqested_lease);
- optptr = set_optval(optptr, DHCP_OPT_LEASE_TIME, &reqested_lease, 4);
- send_packet(1);
- write_leasefile();
- break;
- case DHCPDECLINE:// FALL THROUGH
- case DHCPRELEASE:
- dbg("Message Type : DHCPDECLINE or DHCPRELEASE \n");
- get_optval((uint8_t*) &gstate.rcvd_pkt.options, DHCP_OPT_SERVER_ID, &serverid);
- if (serverid != gconfig.server_nip) break;
- get_optval((uint8_t*) &gstate.rcvd_pkt.options, DHCP_OPT_REQUESTED_IP, &requested_nip);
- delip_from_lease(requested_nip, gstate.rcvd_pkt.chaddr, (gstate.rqcode==DHCPRELEASE)?0:gconfig.decline_time);
- break;
- default:
- dbg("Message Type : %u\n", gstate.rqcode);
- break;
- }
- }
- }
+ dbg("enter main loop\n");
+ int signals[] = {SIGALRM, SIGUSR1, SIGTERM, 0};
+ daemon_main_loop(gstate.listensock + 1, &rfds, gconfig.auto_time, signals,
+ sig_handler, select_handler);
}
diff -r 587b7572aac2 -r 752d22ece0fe toys/pending/syslogd.c
--- a/toys/pending/syslogd.c Sun Sep 01 15:13:58 2013 +0200
+++ b/toys/pending/syslogd.c Sun Sep 01 15:25:08 2013 +0200
@@ -68,7 +68,6 @@

struct unsocks *lsocks; // list of listen sockets
struct logfile *lfiles; // list of write logfiles
- int sigfd[2];
)

// Lookup numerical code from name
@@ -388,28 +387,71 @@
TT.lfiles = fnode->next;
free(fnode);
}
+ unlink("/var/run/syslogd.pid");
}

-static void signal_handler(int sig)
+static void sighandler(int sig)
{
- unsigned char ch = sig;
- if (write(TT.sigfd[1], &ch, 1) != 1) error_msg("can't send signal");
+ switch(sig) {
+ case SIGALRM:
+ logmsg("<46>-- MARK --", 14);
+ break;
+ case SIGTERM: /* FALLTHROUGH */
+ case SIGINT: /* FALLTHROUGH */
+ case SIGQUIT:
+ logmsg("<46>syslogd exiting", 19);
+ if (CFG_TOYBOX_FREE ) cleanup();
+ signal(sig, SIG_DFL);
+ sigset_t ss;
+ sigemptyset(&ss);
+ sigaddset(&ss, sig);
+ sigprocmask(SIG_UNBLOCK, &ss, NULL);
+ raise(sig);
+ _exit(1); /* Should not reach it */
+ break;
+ case SIGHUP:
+ logmsg("<46>syslogd exiting", 19);
+ cleanup(); //cleanup is done, as we restart syslog.
+ xexec(toys.argv);
+ }
+}
+
+static void selecthandler(fd_set *rfds)
+{
+ struct unsocks *tsd;
+ char *buffer = (toybuf + 2048), *last_buf = (toybuf + 3072); //these two buffs are of 1K each
+ int last_len=0;
+
+ for (tsd = TT.lsocks; tsd; tsd = tsd->next) {
+ int sd = tsd->sd;
+ if (FD_ISSET(sd, rfds)) {
+ int len = read(sd, buffer, 1023); //buffer is of 1K, hence readingonly 1023 bytes, 1 for NUL
+ if (len > 0) {
+ buffer[len] = '\0';
+ if((toys.optflags & FLAG_D) && (len == last_len))
+ if (!memcmp(last_buf, buffer, len)) break;
+
+ memcpy(last_buf, buffer, len);
+ last_len = len;
+ logmsg(buffer, len);
+ }
+ break;
+ }
+ }
}

void syslogd_main(void)
{
struct unsocks *tsd;
- int nfds, retval, last_len=0;
- struct timeval tv;
+ int nfds;
fd_set rfds; // fds for reading
- char *temp, *buffer = (toybuf +2048), *last_buf = (toybuf + 3072); //these two buffs are of 1K each
+ char *temp;

if ((toys.optflags & FLAG_p) && (strlen(TT.unix_socket) > 108))
error_exit("Socket path should not be more than 108");

TT.config_file = (toys.optflags & FLAG_f) ?
TT.config_file : "/etc/syslog.conf"; //DEFCONFFILE
-init_jumpin:
tsd = xzalloc(sizeof(struct unsocks));

tsd->path = (toys.optflags & FLAG_p) ? TT.unix_socket : "/dev/log"; // DEFLOGSOCK
@@ -445,94 +487,25 @@
continue;
}
chmod(tsd->path, 0777);
- nfds++;
+ nfds = tsd->sd;
}
if (!nfds) {
error_msg("Can't open single socket for listenning.");
goto clean_and_exit;
}

- // Setup signals
- if (pipe(TT.sigfd) < 0) error_exit("pipe failed\n");
-
- fcntl(TT.sigfd[1] , F_SETFD, FD_CLOEXEC);
- fcntl(TT.sigfd[0] , F_SETFD, FD_CLOEXEC);
- int flags = fcntl(TT.sigfd[1], F_GETFL);
- fcntl(TT.sigfd[1], F_SETFL, flags | O_NONBLOCK);
- signal(SIGHUP, signal_handler);
- signal(SIGTERM, signal_handler);
- signal(SIGINT, signal_handler);
- signal(SIGQUIT, signal_handler);
-
if (parse_config_file() == -1) goto clean_and_exit;
open_logfiles();
- if (!(toys.optflags & FLAG_n)) {
- daemonize();
- //don't daemonize again if SIGHUP received.
- toys.optflags |= FLAG_n;
- }
+ if (!(toys.optflags & FLAG_n)) daemonize();
xpidfile("syslogd");

logmsg("<46>Toybox: syslogd started", 27); //27 : the length of message
- for (;;) {
- // Add opened socks to rfds for select()
- FD_ZERO(&rfds);
- for (tsd = TT.lsocks; tsd; tsd = tsd->next) FD_SET(tsd->sd, &rfds);
- FD_SET(TT.sigfd[0], &rfds);
- tv.tv_usec = 0;
- tv.tv_sec = TT.interval*60;

- retval = select(TT.sigfd[0] + 1, &rfds, NULL, NULL, (TT.interval)?&tv:NULL);
- if (retval < 0) {
- if (errno != EINTR) perror_msg("Error in select ");
- }
- else if (!retval) logmsg("<46>-- MARK --", 14);
- else if (FD_ISSET(TT.sigfd[0], &rfds)) { /* May be a signal */
- unsigned char sig;
-
- if (read(TT.sigfd[0], &sig, 1) != 1) {
- error_msg("signal read failed.\n");
- continue;
- }
- switch(sig) {
- case SIGTERM: /* FALLTHROUGH */
- case SIGINT: /* FALLTHROUGH */
- case SIGQUIT:
- logmsg("<46>syslogd exiting", 19);
- if (CFG_TOYBOX_FREE ) cleanup();
- signal(sig, SIG_DFL);
- sigset_t ss;
- sigemptyset(&ss);
- sigaddset(&ss, sig);
- sigprocmask(SIG_UNBLOCK, &ss, NULL);
- raise(sig);
- _exit(1); /* Should not reach it */
- break;
- case SIGHUP:
- logmsg("<46>syslogd exiting", 19);
- cleanup(); //cleanup is done, as we restart syslog.
- goto init_jumpin;
- default: break;
- }
- } else { /* Some activity on listen sockets. */
- for (tsd = TT.lsocks; tsd; tsd = tsd->next) {
- int sd = tsd->sd;
- if (FD_ISSET(sd, &rfds)) {
- int len = read(sd, buffer, 1023); //buffer is of 1K, hence readingonly 1023 bytes, 1 for NUL
- if (len > 0) {
- buffer[len] = '\0';
- if((toys.optflags & FLAG_D) && (len == last_len))
- if (!memcmp(last_buf, buffer, len)) break;
-
- memcpy(last_buf, buffer, len);
- last_len = len;
- logmsg(buffer, len);
- }
- break;
- }
- }
- }
- }
+ FD_ZERO(&rfds);
+ for (tsd = TT.lsocks; tsd; tsd = tsd->next) FD_SET(tsd->sd, &rfds);
+ int signals[] = {SIGALRM, SIGHUP, SIGTERM, SIGINT, SIGQUIT, 0};
+ daemon_main_loop(nfds + 1, &rfds, TT.interval*60, signals,
+ sighandler, selecthandler);
clean_and_exit:
logmsg("<46>syslogd exiting", 19);
if (CFG_TOYBOX_FREE ) cleanup();
Rob Landley
2013-09-06 11:19:47 UTC
Permalink
Post by Felix Janda
Both syslogd and dhcpd wait for sockets, signals or a timeout.
Currently,
they use select() with its timeout to listen to the sockets and a
self-pipe for the signals.
The new library function instead uses pselect() without a timeout and
alarm(). With pselect() signals are only unblocked during this system
call; so the xwrappers don't need to be aware about interrupted
syscalls.
Are we normally blocking a lot of signals in other commands? Using the
6 argument version instead of the 5 argument version addresses an issue
for which existing command?
Post by Felix Janda
alarm() is used instead of the timeout argument to pselect() so that
the length of the intervals between timeouts is not affected by being
interrupted.
QEMU recently changed its timer system to _not_ use alarm:

http://blog.alex.org.uk/2013/08/24/changes-to-qemus-timer-system/

I'm not convinced sending signals to ourselves is preferable to telling
the system call when to timeout. Of the three options (select adjusting
its timespec, us manually reading the clock and feeding pselect a
timeout based on when we next want to wake up, and us sending ourselves
an alarm), us sending ourselves an alarm and constantly having to reset
it seems like it has the most moving parts and the most things that can
go wrong.

Ok, maybe I've been slightly burned by SIGALRM before:

http://landley.net/notes-2011.html#05-09-2011
Post by Felix Janda
On SIGHUP syslogd restarts. Previously everything relevant was
contained
in syslogd_main() and the restarting was implemented by a goto. Now a
longjmp() would be necessary. I instead used an xexec().
Since commit 955 (and pondering toysh to finally tackle that sucker
within a finite amount of time) I'm a bit worried that restarting a
command without OS involvement might misbehave if the signal mask gets
corrupted, or there are funky environment variables, or memory leaks
build up, or we have leftover filehandles, or a couple dozen other
things.

We haven't got "generic reset this process's state" code, longjmp()
leaves a lot of debris, and xexec() callign a new main doesn't clear
the old stuff off the stack; do that in a tight enough loop for long
enough and eventually you will run out of stack even in a normal linux
userspace. (Default's 8 megs: ulimit -s says 8192 and the bash man page
says the units are kilobytes.)

I added NOFORK because some commands (like cd) _must_ happen in the
current toysh process context, but also because I could very very
carefully audit commands to make sure it was safe to call them in the
same process context and continue when they return. But these commands
are in pending because I haven't even done an initial audit pass on
them.

Discarding your process on restart and execing a new one covers a
multitude of sins. Possibly syslogd is clean enough it can restart
itself without accumulating trash, but as a long-lived daemon I have
yet to closely read (hence it being in the pending directory), this
does not fill me with confidence.

Probably just the "it's 5am, sun coming up soon" things. I should step
away from the keyboard...
Post by Felix Janda
Since xexec()
doesn't change the pid the pidfile needs to be unlinked() before
terminating so that xpidfile() doesn't complain.
Um... I agree this is a symptom of a problem. I'm not sure that's the
right fix. The restart is maintaining state and then getting confused
by the state it's maintaining...

(I can just see some horrible systemd thing hitting a race window where
the PID file wasn't there and trying to restart the process...)
Post by Felix Janda
Furthermore if not run
in the foreground syslogd will daemonize() again when restarting.
Could
one detect in daemonize() whether daemonizing is necessary?
"The restart is maintaining state and then getting..."

Are these the _only_ instances of this? Is a main loop that exits and
restarts itself but avoids initial setup better?

Is there a clean way to do this where we _don't_ have to be absolutely
sure we've thought of everything ever? execv("/proc/self/exe",
toys.argc) and then fall back to something else if it returns, perhaps?
Or can we get an exhaustive list of all process resources (from the
container suspend/resume guys, presumably) and check for ourselves that
we're not leaking anything?

Very old concern of mine, a full decade before containers:

http://lkml.indiana.edu/hypermail/linux/kernel/0206.2/0835.html
Post by Felix Janda
Felix
# HG changeset patch
# User Felix Janda <felix.janda at posteo.de>
# Date 1378041908 -7200
# Node ID 752d22ece0fe3fa9763fd0b6e231e653cc173890
# Parent 587b7572aac2a64988503458155e1f2546672525
Factor out daemon select() loops into a library function
Ok, the new select() lib function I'd probably be up for reviewing on
its own, and the cleanups to a function I haven't even read yet I'd
just pass through and read/cleanup the new baseline when I got around
to it.

But both at once? Not up for that right now. Might try again in the
morning...

Rob
Felix Janda
2013-09-06 22:35:27 UTC
Permalink
Post by Rob Landley
Post by Felix Janda
Both syslogd and dhcpd wait for sockets, signals or a timeout. Currently,
they use select() with its timeout to listen to the sockets and a
self-pipe for the signals.
The new library function instead uses pselect() without a timeout and
alarm(). With pselect() signals are only unblocked during this system
call; so the xwrappers don't need to be aware about interrupted syscalls.
Are we normally blocking a lot of signals in other commands? Using the
6 argument version instead of the 5 argument version addresses an issue
for which existing command?
If a signal occurs during the block it will still be delivered in the next
pselect loop. pselect implements exactly the signal unblocking I am talking
about above. We block all signals. But during the signal call some are
unblocked. When it returns they are again blocked and we can safely check
what has happened. See the article

https://lwn.net/Articles/176911/

for some discussion on the system call.
Post by Rob Landley
Post by Felix Janda
alarm() is used instead of the timeout argument to pselect() so that
the length of the intervals between timeouts is not affected by being
interrupted.
http://blog.alex.org.uk/2013/08/24/changes-to-qemus-timer-system/
I'm not convinced sending signals to ourselves is preferable to telling
the system call when to timeout. Of the three options (select adjusting
its timespec, us manually reading the clock and feeding pselect a
timeout based on when we next want to wake up, and us sending ourselves
an alarm), us sending ourselves an alarm and constantly having to reset
it seems like it has the most moving parts and the most things that can
go wrong.
select() adjusts its timespecs on linux. But that's only one possible
behavior specified by POSIX. Since we are already doing signals SIGALRM
seemed more elegant then constantly adjusting the timeout to me.
Post by Rob Landley
http://landley.net/notes-2011.html#05-09-2011
Post by Felix Janda
On SIGHUP syslogd restarts. Previously everything relevant was contained
in syslogd_main() and the restarting was implemented by a goto. Now a
longjmp() would be necessary. I instead used an xexec().
Since commit 955 (and pondering toysh to finally tackle that sucker
within a finite amount of time) I'm a bit worried that restarting a
command without OS involvement might misbehave if the signal mask gets
corrupted, or there are funky environment variables, or memory leaks
build up, or we have leftover filehandles, or a couple dozen other
things.
We haven't got "generic reset this process's state" code, longjmp()
leaves a lot of debris, and xexec() callign a new main doesn't clear
the old stuff off the stack; do that in a tight enough loop for long
enough and eventually you will run out of stack even in a normal linux
userspace. (Default's 8 megs: ulimit -s says 8192 and the bash man page
says the units are kilobytes.)
I added NOFORK because some commands (like cd) _must_ happen in the
current toysh process context, but also because I could very very
carefully audit commands to make sure it was safe to call them in the
same process context and continue when they return. But these commands
are in pending because I haven't even done an initial audit pass on
them.
Discarding your process on restart and execing a new one covers a
multitude of sins. Possibly syslogd is clean enough it can restart
itself without accumulating trash, but as a long-lived daemon I have
yet to closely read (hence it being in the pending directory), this
does not fill me with confidence.
Probably just the "it's 5am, sun coming up soon" things. I should step
away from the keyboard...
You are of course right. I think you have some good ideas below how to
fix it.
Post by Rob Landley
Post by Felix Janda
Since xexec()
doesn't change the pid the pidfile needs to be unlinked() before
terminating so that xpidfile() doesn't complain.
Um... I agree this is a symptom of a problem. I'm not sure that's the
right fix. The restart is maintaining state and then getting confused
by the state it's maintaining...
(I can just see some horrible systemd thing hitting a race window where
the PID file wasn't there and trying to restart the process...)
Does systemd still need PID files?
Post by Rob Landley
Post by Felix Janda
Furthermore if not run
in the foreground syslogd will daemonize() again when restarting. Could
one detect in daemonize() whether daemonizing is necessary?
"The restart is maintaining state and then getting..."
Are these the _only_ instances of this? Is a main loop that exits and
restarts itself but avoids initial setup better?
Is there a clean way to do this where we _don't_ have to be absolutely
sure we've thought of everything ever? execv("/proc/self/exe",
toys.argc) and then fall back to something else if it returns, perhaps?
Or can we get an exhaustive list of all process resources (from the
container suspend/resume guys, presumably) and check for ourselves that
we're not leaking anything?
So how about making the two helper functions for daemon_main_loop()
return a value and possibly returning from daemon_main_loop() based on
that value? Then in syslogd_main() add a infinite loop around (or almost)
everything.
Post by Rob Landley
http://lkml.indiana.edu/hypermail/linux/kernel/0206.2/0835.html
Post by Felix Janda
Felix
# HG changeset patch
# User Felix Janda <felix.janda at posteo.de>
# Date 1378041908 -7200
# Node ID 752d22ece0fe3fa9763fd0b6e231e653cc173890
# Parent 587b7572aac2a64988503458155e1f2546672525
Factor out daemon select() loops into a library function
Ok, the new select() lib function I'd probably be up for reviewing on
its own, and the cleanups to a function I haven't even read yet I'd
just pass through and read/cleanup the new baseline when I got around
to it.
This patch really just implements the helper function and makes the two
toys use it. It's just a big function, causes the big main functions to
be split up and makes the self-pipe stuff unnecessary.
Post by Rob Landley
But both at once? Not up for that right now. Might try again in the
morning...
I wouldn't consider syslogd to be of high priority.

On the other hand I'd like (or maybe would have liked some days ago)
to continue with cleaning it up. Now that the main function is not such
a monster anymore one can maybe inline more stuff. OTOH I'm still not
sure about the behavior when something goes wrong when the toy starts.

If we come to an conclusion here, I'll send you a new version of the
patch, right?

Felix
Felix Janda
2013-09-06 22:35:27 UTC
Permalink
Post by Rob Landley
Post by Felix Janda
Both syslogd and dhcpd wait for sockets, signals or a timeout. Currently,
they use select() with its timeout to listen to the sockets and a
self-pipe for the signals.
The new library function instead uses pselect() without a timeout and
alarm(). With pselect() signals are only unblocked during this system
call; so the xwrappers don't need to be aware about interrupted syscalls.
Are we normally blocking a lot of signals in other commands? Using the
6 argument version instead of the 5 argument version addresses an issue
for which existing command?
If a signal occurs during the block it will still be delivered in the next
pselect loop. pselect implements exactly the signal unblocking I am talking
about above. We block all signals. But during the signal call some are
unblocked. When it returns they are again blocked and we can safely check
what has happened. See the article

https://lwn.net/Articles/176911/

for some discussion on the system call.
Post by Rob Landley
Post by Felix Janda
alarm() is used instead of the timeout argument to pselect() so that
the length of the intervals between timeouts is not affected by being
interrupted.
http://blog.alex.org.uk/2013/08/24/changes-to-qemus-timer-system/
I'm not convinced sending signals to ourselves is preferable to telling
the system call when to timeout. Of the three options (select adjusting
its timespec, us manually reading the clock and feeding pselect a
timeout based on when we next want to wake up, and us sending ourselves
an alarm), us sending ourselves an alarm and constantly having to reset
it seems like it has the most moving parts and the most things that can
go wrong.
select() adjusts its timespecs on linux. But that's only one possible
behavior specified by POSIX. Since we are already doing signals SIGALRM
seemed more elegant then constantly adjusting the timeout to me.
Post by Rob Landley
http://landley.net/notes-2011.html#05-09-2011
Post by Felix Janda
On SIGHUP syslogd restarts. Previously everything relevant was contained
in syslogd_main() and the restarting was implemented by a goto. Now a
longjmp() would be necessary. I instead used an xexec().
Since commit 955 (and pondering toysh to finally tackle that sucker
within a finite amount of time) I'm a bit worried that restarting a
command without OS involvement might misbehave if the signal mask gets
corrupted, or there are funky environment variables, or memory leaks
build up, or we have leftover filehandles, or a couple dozen other
things.
We haven't got "generic reset this process's state" code, longjmp()
leaves a lot of debris, and xexec() callign a new main doesn't clear
the old stuff off the stack; do that in a tight enough loop for long
enough and eventually you will run out of stack even in a normal linux
userspace. (Default's 8 megs: ulimit -s says 8192 and the bash man page
says the units are kilobytes.)
I added NOFORK because some commands (like cd) _must_ happen in the
current toysh process context, but also because I could very very
carefully audit commands to make sure it was safe to call them in the
same process context and continue when they return. But these commands
are in pending because I haven't even done an initial audit pass on
them.
Discarding your process on restart and execing a new one covers a
multitude of sins. Possibly syslogd is clean enough it can restart
itself without accumulating trash, but as a long-lived daemon I have
yet to closely read (hence it being in the pending directory), this
does not fill me with confidence.
Probably just the "it's 5am, sun coming up soon" things. I should step
away from the keyboard...
You are of course right. I think you have some good ideas below how to
fix it.
Post by Rob Landley
Post by Felix Janda
Since xexec()
doesn't change the pid the pidfile needs to be unlinked() before
terminating so that xpidfile() doesn't complain.
Um... I agree this is a symptom of a problem. I'm not sure that's the
right fix. The restart is maintaining state and then getting confused
by the state it's maintaining...
(I can just see some horrible systemd thing hitting a race window where
the PID file wasn't there and trying to restart the process...)
Does systemd still need PID files?
Post by Rob Landley
Post by Felix Janda
Furthermore if not run
in the foreground syslogd will daemonize() again when restarting. Could
one detect in daemonize() whether daemonizing is necessary?
"The restart is maintaining state and then getting..."
Are these the _only_ instances of this? Is a main loop that exits and
restarts itself but avoids initial setup better?
Is there a clean way to do this where we _don't_ have to be absolutely
sure we've thought of everything ever? execv("/proc/self/exe",
toys.argc) and then fall back to something else if it returns, perhaps?
Or can we get an exhaustive list of all process resources (from the
container suspend/resume guys, presumably) and check for ourselves that
we're not leaking anything?
So how about making the two helper functions for daemon_main_loop()
return a value and possibly returning from daemon_main_loop() based on
that value? Then in syslogd_main() add a infinite loop around (or almost)
everything.
Post by Rob Landley
http://lkml.indiana.edu/hypermail/linux/kernel/0206.2/0835.html
Post by Felix Janda
Felix
# HG changeset patch
# User Felix Janda <felix.janda at posteo.de>
# Date 1378041908 -7200
# Node ID 752d22ece0fe3fa9763fd0b6e231e653cc173890
# Parent 587b7572aac2a64988503458155e1f2546672525
Factor out daemon select() loops into a library function
Ok, the new select() lib function I'd probably be up for reviewing on
its own, and the cleanups to a function I haven't even read yet I'd
just pass through and read/cleanup the new baseline when I got around
to it.
This patch really just implements the helper function and makes the two
toys use it. It's just a big function, causes the big main functions to
be split up and makes the self-pipe stuff unnecessary.
Post by Rob Landley
But both at once? Not up for that right now. Might try again in the
morning...
I wouldn't consider syslogd to be of high priority.

On the other hand I'd like (or maybe would have liked some days ago)
to continue with cleaning it up. Now that the main function is not such
a monster anymore one can maybe inline more stuff. OTOH I'm still not
sure about the behavior when something goes wrong when the toy starts.

If we come to an conclusion here, I'll send you a new version of the
patch, right?

Felix
Rob Landley
2013-09-06 16:46:26 UTC
Permalink
Post by Felix Janda
diff -r 587b7572aac2 -r 752d22ece0fe lib/lib.h
--- a/lib/lib.h Sun Sep 01 15:13:58 2013 +0200
+++ b/lib/lib.h Sun Sep 01 15:25:08 2013 +0200
@@ -201,4 +201,7 @@
char *astrcat (char *, char *);
char *xastrcat (char *, char *);
+// daemon helper functions
void daemonize(void);
+void daemon_main_loop(int nfds, fd_set *readfds, long timeout, int
*signals,
+ void (*sighandler)(int), void
(*selecthandler)(fd_set *fds));
It's not really "daemon", is it? Netcat isn't a daemon, less isn't a
daemon, tail -f isn't a daemon... All of 'em could potentially benefit
from a select loop. It's pretty much just "select_loop()".

Also, the current code in netcat is using poll(), not select(). (Array
of file descriptors vs magic sized bitfield where you have to calculate
nfds, which _this_ function is asking the caller to do. Neither one
actually scales to thousands of filehandles gracefully, but if that's
an issue it's probably not common library code the one or two
filehandle case is going to want to use...)
Post by Felix Janda
diff -r 587b7572aac2 -r 752d22ece0fe lib/pending.c
--- a/lib/pending.c Sun Sep 01 15:13:58 2013 +0200
+++ b/lib/pending.c Sun Sep 01 15:25:08 2013 +0200
@@ -92,3 +92,41 @@
dup2(fd, 2);
if (fd > 2) close(fd);
}
+
+static int daemon_sig;
+
Gratuitous global variable. I've been trying to eliminate those. (I
just added libbuf analogous to toybuf but for use by lib/*.c,
presumably it could use that. But... is it worth doing?)
Post by Felix Janda
+static void daemon_sighandler(int sig)
+{
+ daemon_sig = sig;
+}
+
If we weren't messing with alarm, and instead of using the timeouts
built into select, would signals be our problem at all? (Might have to
calculate a timeout, but we do that anyway when actual data comes in if
we need a periodic action, and you can get data with a bad checksum
that causes spurious "data arrived, no wait it didn't" wakeups anyway.
Getting interrupted happens, we need to cope.)

(And still confused about the syscall having a timeout but us not using
it...)
Post by Felix Janda
+/*
+ * main loop for daemons listening to signals and file handles
+*/
+void daemon_main_loop(int nfds, fd_set *readfds, long timeout, int
*signals,
+ void (*sighandler)(int), void
(*selecthandler)(fd_set *fds))
+{
+ fd_set copy;
+ sigset_t sigmask;
+ struct sigaction action;
+ int *i, ret;
+
+ sigemptyset(&sigmask);
+ action.sa_handler = daemon_sighandler;
+ sigemptyset(&action.sa_mask);
+ action.sa_flags = 0;
+ if (timeout) alarm(timeout);
+ for (i = signals; *i; i++) sigaddset(&sigmask, *i);
+ sigprocmask(SIG_SETMASK, &sigmask, 0);
+ for (i = signals; *i; i++) sigaction(*i, &action, 0);
To quote from the ppoll man page:

Other than the difference in the precision of the timeout
argument, the
following ppoll() call:

ready = ppoll(&fds, nfds, timeout_ts, &sigmask);

is equivalent to atomically executing the following calls:

sigset_t origmask;
int timeout;

timeout = (timeout_ts == NULL) ? -1 :
(timeout_ts.tv_sec * 1000 + timeout_ts.tv_nsec /
1000000);
sigprocmask(SIG_SETMASK, &sigmask, &origmask);
ready = poll(&fds, nfds, timeout);
sigprocmask(SIG_SETMASK, &origmask, NULL);

And yet a call to pselect needs its own sigprocmask() call as setup?
So... why are we using pselect again?
Post by Felix Janda
+ for (;;) {
+ memcpy(&copy, &readfds, sizeof(copy));
+ ret = pselect(nfds, &copy, 0, 0, 0, &action.sa_mask);
+ if (ret == -1) {
+ if (errno != EINTR) error_exit ("pselect");
+ if (timeout && daemon_sig == SIGALRM) alarm(timeout);
+ sighandler(daemon_sig);
Unconditionally calling sighandler(), so we don't have the option of it
being null and just getting the default behavior for signals. And the
caller can't have its own use of alarm outside this helper.
Post by Felix Janda
+ } else selecthandler(&copy);
+ }
+}
diff -r 587b7572aac2 -r 752d22ece0fe toys/pending/dhcpd.c
--- a/toys/pending/dhcpd.c Sun Sep 01 15:13:58 2013 +0200
+++ b/toys/pending/dhcpd.c Sun Sep 01 15:25:08 2013 +0200
@@ -206,11 +206,9 @@
{"msstaticroutes" , DHCP_STCRTS | 0xf9, NULL, 0},
};
-struct fd_pair { int rd; int wr; };
static server_config_t gconfig;
static server_state_t gstate;
static uint8_t infomode;
-static struct fd_pair sigfd;
static int constone = 1;
Isn't an fd pair the common case? That's what netcat's doing, that's
what less is doing... tail cares about arbitrary numbers of
filehandles, but that says to me poll_pair_loop(rfd, wfd, callback,
timeout) would be a wrapper around poll_pair(pollfds, count, callback,
timeout). So you _can_ muck about in the guts of poll setup yourself,
but aren't required to.
Post by Felix Janda
// calculate options size.
@@ -316,29 +314,6 @@
}
}
-// Generic signal handler real handling is done in main funcrion.
-static void signal_handler(int sig)
-{
- unsigned char ch = sig;
- if (write(sigfd.wr, &ch, 1) != 1) dbg("can't send signal\n");
-}
-
-// signal setup for SIGUSR1 SIGTERM
-static int setup_signal()
-{
- if (pipe((int *)&sigfd) < 0) {
- dbg("signal pipe failed\n");
- return -1;
- }
- fcntl(sigfd.wr , F_SETFD, FD_CLOEXEC);
- fcntl(sigfd.rd , F_SETFD, FD_CLOEXEC);
- int flags = fcntl(sigfd.wr, F_GETFL);
- fcntl(sigfd.wr, F_SETFL, flags | O_NONBLOCK);
- signal(SIGUSR1, signal_handler);
- signal(SIGTERM, signal_handler);
- return 0;
-}
-
It's nice to remove code...
Post by Felix Janda
// String STR to UINT32 conversion strored in VAR
static int strtou32(const char *str, void *var)
{
@@ -1073,15 +1048,114 @@
return ret;
}
+static void sig_handler(int sig)
+{
+ switch (sig) {
+ write_leasefile();
+ if (get_interface(gconfig.interface, &gconfig.ifindex,
&gconfig.server_nip, gconfig.server_mac)<0)
+ perror_exit("Interface lost %s\n", gconfig.interface);
+ gconfig.server_nip = htonl(gconfig.server_nip);
+ break;
+ infomsg(infomode, "Received SIGUSR1");
+ write_leasefile();
+ break;
+ infomsg(infomode, "Received SIGTERM");
+ write_leasefile();
+ unlink(gconfig.pidfile);
+ exit(0);
+ break;
+ }
+}
+
+static void select_handler(fd_set *rfds)
+{
+ uint8_t *optptr, msgtype = 0;
+ uint32_t serverid = 0, requested_nip = 0;
+ uint32_t reqested_lease = 0;
+ char *hstname = NULL;
+
+ dbg("select listen sock read\n");
+ if (read_packet() < 0) {
+ open_listensock();
+ return;
+ }
+ get_optval((uint8_t*)&gstate.rcvd_pkt.options,
DHCP_OPT_MESSAGE_TYPE, &gstate.rqcode);
+ if (gstate.rqcode == 0 || gstate.rqcode < DHCPDISCOVER
+ || gstate.rqcode > DHCPINFORM) {
+ dbg("no or bad message type option, ignoring packet.\n");
+ return;
+ }
+ get_optval((uint8_t*) &gstate.rcvd_pkt.options,
DHCP_OPT_SERVER_ID, &serverid);
+ if (serverid && (serverid != gconfig.server_nip)) {
+ dbg("server ID doesn't match, ignoring packet.\n");
+ return;
+ }
+ switch (gstate.rqcode) {
+ msgtype = DHCPOFFER;
+ dbg("Message Type : DHCPDISCOVER\n");
+ get_optval((uint8_t*) &gstate.rcvd_pkt.options,
DHCP_OPT_REQUESTED_IP, &requested_nip);
+ get_optval((uint8_t*) &gstate.rcvd_pkt.options,
DHCP_OPT_HOST_NAME, &hstname);
+ reqested_lease = gconfig.offer_time;
+ get_reqparam(&gstate.rqopt);
+ optptr = prepare_send_pkt();
+ gstate.send_pkt.yiaddr = getip_from_pool(requested_nip,
gstate.rcvd_pkt.chaddr, &reqested_lease, hstname);
+ if(!gstate.send_pkt.yiaddr){
+ msgtype = DHCPNAK;
+ optptr = set_optval(optptr, DHCP_OPT_MESSAGE_TYPE, &msgtype,
1);
+ send_packet(1);
+ break;
+ }
+ get_optval((uint8_t*) &gstate.rcvd_pkt.options,
DHCP_OPT_LEASE_TIME, &reqested_lease);
+ reqested_lease = htonl(get_lease(reqested_lease + time(NULL)));
+ optptr = set_optval(optptr, DHCP_OPT_MESSAGE_TYPE, &msgtype,
1);
+ optptr = set_optval(optptr, DHCP_OPT_SERVER_ID,
&gconfig.server_nip, 4);
+ optptr = set_optval(optptr, DHCP_OPT_LEASE_TIME,
&reqested_lease, 4);
+ optptr = set_reqparam(optptr, gstate.rqopt);
+ send_packet(1);
+ break;
+ msgtype = DHCPACK;
+ dbg("Message Type : DHCPREQUEST\n");
+ optptr = prepare_send_pkt();
+ get_optval((uint8_t*) &gstate.rcvd_pkt.options,
DHCP_OPT_REQUESTED_IP, &requested_nip);
+ get_optval((uint8_t*) &gstate.rcvd_pkt.options,
DHCP_OPT_LEASE_TIME, &reqested_lease);
+ get_optval((uint8_t*) &gstate.rcvd_pkt.options,
DHCP_OPT_HOST_NAME, &hstname);
+ gstate.send_pkt.yiaddr = getip_from_pool(requested_nip,
gstate.rcvd_pkt.chaddr, &reqested_lease, hstname);
+ if (!serverid) reqested_lease = gconfig.max_lease_sec;
+ if (!gstate.send_pkt.yiaddr) {
+ msgtype = DHCPNAK;
+ optptr = set_optval(optptr, DHCP_OPT_MESSAGE_TYPE, &msgtype,
1);
+ send_packet(1);
+ break;
+ }
+ optptr = set_optval(optptr, DHCP_OPT_MESSAGE_TYPE, &msgtype,
1);
+ optptr = set_optval(optptr, DHCP_OPT_SERVER_ID,
&gconfig.server_nip, 4);
+ reqested_lease = htonl(reqested_lease);
+ optptr = set_optval(optptr, DHCP_OPT_LEASE_TIME,
&reqested_lease, 4);
+ send_packet(1);
+ write_leasefile();
+ break;
+ case DHCPDECLINE:// FALL THROUGH
+ dbg("Message Type : DHCPDECLINE or DHCPRELEASE \n");
+ get_optval((uint8_t*) &gstate.rcvd_pkt.options,
DHCP_OPT_SERVER_ID, &serverid);
+ if (serverid != gconfig.server_nip) break;
+ get_optval((uint8_t*) &gstate.rcvd_pkt.options,
DHCP_OPT_REQUESTED_IP, &requested_nip);
+ delip_from_lease(requested_nip, gstate.rcvd_pkt.chaddr,
(gstate.rqcode==DHCPRELEASE)?0:gconfig.decline_time);
+ break;
+ dbg("Message Type : %u\n", gstate.rqcode);
+ break;
+ }
+}
+
Not sure it's a net win, though. Really just seems to be moving stuff
around...

Let's see, ctrl-alt-diffstat:

$ diffstat felix2.patch
lib/lib.h | 3
lib/pending.c | 38 ++++++
toys/pending/dhcpd.c | 274
++++++++++++++++++++-----------------------------
toys/pending/syslogd.c | 141 ++++++++++---------------
4 files changed, 211 insertions(+), 245 deletions(-)

Ok, net win. (I need a lib/pending.h. Right now adding stuff to
lib/pending.c and then yanking it out again affects the commit history
of permanent files...)

But this patch is doing a lot more than just introducing the new
library functions and making stuff use them, so I've got to do _more_
review before I can come to an actual conclusion about whether or not
to apply it...

I really do appreciate this work. Sorry to give you such a hard time
about it. I just want to get it _right_...

Rob
Felix Janda
2013-09-10 21:22:41 UTC
Permalink
Post by Rob Landley
Post by Felix Janda
diff -r 587b7572aac2 -r 752d22ece0fe lib/lib.h
--- a/lib/lib.h Sun Sep 01 15:13:58 2013 +0200
+++ b/lib/lib.h Sun Sep 01 15:25:08 2013 +0200
@@ -201,4 +201,7 @@
char *astrcat (char *, char *);
char *xastrcat (char *, char *);
+// daemon helper functions
void daemonize(void);
+void daemon_main_loop(int nfds, fd_set *readfds, long timeout, int *signals,
+ void (*sighandler)(int), void
(*selecthandler)(fd_set *fds));
It's not really "daemon", is it? Netcat isn't a daemon, less isn't a
daemon, tail -f isn't a daemon... All of 'em could potentially benefit
from a select loop. It's pretty much just "select_loop()".
I hadn't thought of it that general. Right now it unconditionally sets
up signals but that could be easily fixed.
Post by Rob Landley
Also, the current code in netcat is using poll(), not select(). (Array
of file descriptors vs magic sized bitfield where you have to calculate
nfds, which _this_ function is asking the caller to do. Neither one
actually scales to thousands of filehandles gracefully, but if that's
an issue it's probably not common library code the one or two
filehandle case is going to want to use...)
Post by Felix Janda
diff -r 587b7572aac2 -r 752d22ece0fe lib/pending.c
--- a/lib/pending.c Sun Sep 01 15:13:58 2013 +0200
+++ b/lib/pending.c Sun Sep 01 15:25:08 2013 +0200
@@ -92,3 +92,41 @@
dup2(fd, 2);
if (fd > 2) close(fd);
}
+
+static int daemon_sig;
+
Gratuitous global variable. I've been trying to eliminate those. (I
just added libbuf analogous to toybuf but for use by lib/*.c,
presumably it could use that. But... is it worth doing?)
It seems inevitable to me to keep some global state for the signal
handling.
Post by Rob Landley
Post by Felix Janda
+static void daemon_sighandler(int sig)
+{
+ daemon_sig = sig;
+}
+
If we weren't messing with alarm, and instead of using the timeouts
built into select, would signals be our problem at all? (Might have to
calculate a timeout, but we do that anyway when actual data comes in if
we need a periodic action, and you can get data with a bad checksum
that causes spurious "data arrived, no wait it didn't" wakeups anyway.
Getting interrupted happens, we need to cope.)
(And still confused about the syscall having a timeout but us not using
it...)
SIG_ALRM was an afterthought when I realized that interrupting pselect
messes up the timeouts.

pselect is a replacement for the select + signal self-pipe.
Post by Rob Landley
Post by Felix Janda
+/*
+ * main loop for daemons listening to signals and file handles
+*/
+void daemon_main_loop(int nfds, fd_set *readfds, long timeout, int *signals,
+ void (*sighandler)(int), void
(*selecthandler)(fd_set *fds))
+{
+ fd_set copy;
+ sigset_t sigmask;
+ struct sigaction action;
+ int *i, ret;
+
+ sigemptyset(&sigmask);
+ action.sa_handler = daemon_sighandler;
+ sigemptyset(&action.sa_mask);
+ action.sa_flags = 0;
+ if (timeout) alarm(timeout);
+ for (i = signals; *i; i++) sigaddset(&sigmask, *i);
+ sigprocmask(SIG_SETMASK, &sigmask, 0);
+ for (i = signals; *i; i++) sigaction(*i, &action, 0);
Other than the difference in the precision of the timeout
argument, the
ready = ppoll(&fds, nfds, timeout_ts, &sigmask);
sigset_t origmask;
int timeout;
(timeout_ts.tv_sec * 1000 + timeout_ts.tv_nsec /
1000000);
sigprocmask(SIG_SETMASK, &sigmask, &origmask);
ready = poll(&fds, nfds, timeout);
sigprocmask(SIG_SETMASK, &origmask, NULL);
And yet a call to pselect needs its own sigprocmask() call as setup?
So... why are we using pselect again?
Didn't I say so? Before the call to pselect we block everything. Then
pselect atomically unblocks everything, does the select and afterwards
blocks everything again. So the only thing that can be interrupted by
signals is pselect. No need to think about race conditions. The relation
between pselect and select is the same as between ppoll and poll.

The main reason for not using ppoll is that it is linux only. pselect
is in POSIX. OpenBSD however doesn't seem to have it... The portable
way would be to use the self-pipe as before. But then there is even
more global state...
Post by Rob Landley
Post by Felix Janda
+ for (;;) {
+ memcpy(&copy, &readfds, sizeof(copy));
+ ret = pselect(nfds, &copy, 0, 0, 0, &action.sa_mask);
+ if (ret == -1) {
+ if (errno != EINTR) error_exit ("pselect");
+ if (timeout && daemon_sig == SIGALRM) alarm(timeout);
+ sighandler(daemon_sig);
Unconditionally calling sighandler(), so we don't have the option of it
being null and just getting the default behavior for signals. And the
caller can't have its own use of alarm outside this helper.
Yes, for the broader use it would be good to make the signal handling
optional (but that's actually the main point of this function). The caller
can have its own use of alarm if it doesn't use the one built into this
function.
Post by Rob Landley
Post by Felix Janda
+ } else selecthandler(&copy);
+ }
+}
diff -r 587b7572aac2 -r 752d22ece0fe toys/pending/dhcpd.c
--- a/toys/pending/dhcpd.c Sun Sep 01 15:13:58 2013 +0200
+++ b/toys/pending/dhcpd.c Sun Sep 01 15:25:08 2013 +0200
@@ -206,11 +206,9 @@
{"msstaticroutes" , DHCP_STCRTS | 0xf9, NULL, 0},
};
-struct fd_pair { int rd; int wr; };
static server_config_t gconfig;
static server_state_t gstate;
static uint8_t infomode;
-static struct fd_pair sigfd;
static int constone = 1;
Isn't an fd pair the common case? That's what netcat's doing, that's
what less is doing... tail cares about arbitrary numbers of
filehandles, but that says to me poll_pair_loop(rfd, wfd, callback,
timeout) would be a wrapper around poll_pair(pollfds, count, callback,
timeout). So you _can_ muck about in the guts of poll setup yourself,
but aren't required to.
fd pair is here aka self-pipe. dhcpd actually listens to only one socket
and signals. In the old version it listened to the socket and one end of
the signal self-pipe via select. syslogd needs to listen to arbitray many
sockets.
Post by Rob Landley
Post by Felix Janda
// calculate options size.
@@ -316,29 +314,6 @@
}
}
-// Generic signal handler real handling is done in main funcrion.
-static void signal_handler(int sig)
-{
- unsigned char ch = sig;
- if (write(sigfd.wr, &ch, 1) != 1) dbg("can't send signal\n");
-}
-
-// signal setup for SIGUSR1 SIGTERM
-static int setup_signal()
-{
- if (pipe((int *)&sigfd) < 0) {
- dbg("signal pipe failed\n");
- return -1;
- }
- fcntl(sigfd.wr , F_SETFD, FD_CLOEXEC);
- fcntl(sigfd.rd , F_SETFD, FD_CLOEXEC);
- int flags = fcntl(sigfd.wr, F_GETFL);
- fcntl(sigfd.wr, F_SETFL, flags | O_NONBLOCK);
- signal(SIGUSR1, signal_handler);
- signal(SIGTERM, signal_handler);
- return 0;
-}
-
It's nice to remove code...
Post by Felix Janda
// String STR to UINT32 conversion strored in VAR
static int strtou32(const char *str, void *var)
{
@@ -1073,15 +1048,114 @@
return ret;
}
+static void sig_handler(int sig)
+{
+ switch (sig) {
+ write_leasefile();
+ if (get_interface(gconfig.interface, &gconfig.ifindex,
&gconfig.server_nip, gconfig.server_mac)<0)
+ perror_exit("Interface lost %s\n", gconfig.interface);
+ gconfig.server_nip = htonl(gconfig.server_nip);
+ break;
+ infomsg(infomode, "Received SIGUSR1");
+ write_leasefile();
+ break;
+ infomsg(infomode, "Received SIGTERM");
+ write_leasefile();
+ unlink(gconfig.pidfile);
+ exit(0);
+ break;
+ }
+}
+
+static void select_handler(fd_set *rfds)
+{
+ uint8_t *optptr, msgtype = 0;
+ uint32_t serverid = 0, requested_nip = 0;
+ uint32_t reqested_lease = 0;
+ char *hstname = NULL;
+
+ dbg("select listen sock read\n");
+ if (read_packet() < 0) {
+ open_listensock();
+ return;
+ }
+ get_optval((uint8_t*)&gstate.rcvd_pkt.options,
DHCP_OPT_MESSAGE_TYPE, &gstate.rqcode);
+ if (gstate.rqcode == 0 || gstate.rqcode < DHCPDISCOVER
+ || gstate.rqcode > DHCPINFORM) {
+ dbg("no or bad message type option, ignoring packet.\n");
+ return;
+ }
+ get_optval((uint8_t*) &gstate.rcvd_pkt.options,
DHCP_OPT_SERVER_ID, &serverid);
+ if (serverid && (serverid != gconfig.server_nip)) {
+ dbg("server ID doesn't match, ignoring packet.\n");
+ return;
+ }
+ switch (gstate.rqcode) {
+ msgtype = DHCPOFFER;
+ dbg("Message Type : DHCPDISCOVER\n");
+ get_optval((uint8_t*) &gstate.rcvd_pkt.options,
DHCP_OPT_REQUESTED_IP, &requested_nip);
+ get_optval((uint8_t*) &gstate.rcvd_pkt.options,
DHCP_OPT_HOST_NAME, &hstname);
+ reqested_lease = gconfig.offer_time;
+ get_reqparam(&gstate.rqopt);
+ optptr = prepare_send_pkt();
+ gstate.send_pkt.yiaddr = getip_from_pool(requested_nip,
gstate.rcvd_pkt.chaddr, &reqested_lease, hstname);
+ if(!gstate.send_pkt.yiaddr){
+ msgtype = DHCPNAK;
+ optptr = set_optval(optptr, DHCP_OPT_MESSAGE_TYPE, &msgtype, 1);
+ send_packet(1);
+ break;
+ }
+ get_optval((uint8_t*) &gstate.rcvd_pkt.options,
DHCP_OPT_LEASE_TIME, &reqested_lease);
+ reqested_lease = htonl(get_lease(reqested_lease + time(NULL)));
+ optptr = set_optval(optptr, DHCP_OPT_MESSAGE_TYPE, &msgtype, 1);
+ optptr = set_optval(optptr, DHCP_OPT_SERVER_ID,
&gconfig.server_nip, 4);
+ optptr = set_optval(optptr, DHCP_OPT_LEASE_TIME,
&reqested_lease, 4);
+ optptr = set_reqparam(optptr, gstate.rqopt);
+ send_packet(1);
+ break;
+ msgtype = DHCPACK;
+ dbg("Message Type : DHCPREQUEST\n");
+ optptr = prepare_send_pkt();
+ get_optval((uint8_t*) &gstate.rcvd_pkt.options,
DHCP_OPT_REQUESTED_IP, &requested_nip);
+ get_optval((uint8_t*) &gstate.rcvd_pkt.options,
DHCP_OPT_LEASE_TIME, &reqested_lease);
+ get_optval((uint8_t*) &gstate.rcvd_pkt.options,
DHCP_OPT_HOST_NAME, &hstname);
+ gstate.send_pkt.yiaddr = getip_from_pool(requested_nip,
gstate.rcvd_pkt.chaddr, &reqested_lease, hstname);
+ if (!serverid) reqested_lease = gconfig.max_lease_sec;
+ if (!gstate.send_pkt.yiaddr) {
+ msgtype = DHCPNAK;
+ optptr = set_optval(optptr, DHCP_OPT_MESSAGE_TYPE, &msgtype, 1);
+ send_packet(1);
+ break;
+ }
+ optptr = set_optval(optptr, DHCP_OPT_MESSAGE_TYPE, &msgtype, 1);
+ optptr = set_optval(optptr, DHCP_OPT_SERVER_ID,
&gconfig.server_nip, 4);
+ reqested_lease = htonl(reqested_lease);
+ optptr = set_optval(optptr, DHCP_OPT_LEASE_TIME,
&reqested_lease, 4);
+ send_packet(1);
+ write_leasefile();
+ break;
+ case DHCPDECLINE:// FALL THROUGH
+ dbg("Message Type : DHCPDECLINE or DHCPRELEASE \n");
+ get_optval((uint8_t*) &gstate.rcvd_pkt.options,
DHCP_OPT_SERVER_ID, &serverid);
+ if (serverid != gconfig.server_nip) break;
+ get_optval((uint8_t*) &gstate.rcvd_pkt.options,
DHCP_OPT_REQUESTED_IP, &requested_nip);
+ delip_from_lease(requested_nip, gstate.rcvd_pkt.chaddr,
(gstate.rqcode==DHCPRELEASE)?0:gconfig.decline_time);
+ break;
+ dbg("Message Type : %u\n", gstate.rqcode);
+ break;
+ }
+}
+
Not sure it's a net win, though. Really just seems to be moving stuff
around...
$ diffstat felix2.patch
lib/lib.h | 3
lib/pending.c | 38 ++++++
toys/pending/dhcpd.c | 274
++++++++++++++++++++-----------------------------
toys/pending/syslogd.c | 141 ++++++++++---------------
4 files changed, 211 insertions(+), 245 deletions(-)
Ok, net win. (I need a lib/pending.h. Right now adding stuff to
lib/pending.c and then yanking it out again affects the commit history
of permanent files...)
I guess using signal() instead of sigaction() would save some lines. But I
didn't want to mix the new pselect with the old signal().
Post by Rob Landley
But this patch is doing a lot more than just introducing the new
library functions and making stuff use them, so I've got to do _more_
review before I can come to an actual conclusion about whether or not
to apply it...
It is really just moving stuff around. Luckily the local variables in the
main functions nicely broke up into the three seperate functions each.
Anyway, I still need to improve the restarting of syslogd.
Post by Rob Landley
I really do appreciate this work. Sorry to give you such a hard time
about it. I just want to get it _right_...
No problem. Please feel free to wait with coming back to this until after
the (impending?) release.

Felix
Felix Janda
2013-09-10 21:22:41 UTC
Permalink
Post by Rob Landley
Post by Felix Janda
diff -r 587b7572aac2 -r 752d22ece0fe lib/lib.h
--- a/lib/lib.h Sun Sep 01 15:13:58 2013 +0200
+++ b/lib/lib.h Sun Sep 01 15:25:08 2013 +0200
@@ -201,4 +201,7 @@
char *astrcat (char *, char *);
char *xastrcat (char *, char *);
+// daemon helper functions
void daemonize(void);
+void daemon_main_loop(int nfds, fd_set *readfds, long timeout, int *signals,
+ void (*sighandler)(int), void
(*selecthandler)(fd_set *fds));
It's not really "daemon", is it? Netcat isn't a daemon, less isn't a
daemon, tail -f isn't a daemon... All of 'em could potentially benefit
from a select loop. It's pretty much just "select_loop()".
I hadn't thought of it that general. Right now it unconditionally sets
up signals but that could be easily fixed.
Post by Rob Landley
Also, the current code in netcat is using poll(), not select(). (Array
of file descriptors vs magic sized bitfield where you have to calculate
nfds, which _this_ function is asking the caller to do. Neither one
actually scales to thousands of filehandles gracefully, but if that's
an issue it's probably not common library code the one or two
filehandle case is going to want to use...)
Post by Felix Janda
diff -r 587b7572aac2 -r 752d22ece0fe lib/pending.c
--- a/lib/pending.c Sun Sep 01 15:13:58 2013 +0200
+++ b/lib/pending.c Sun Sep 01 15:25:08 2013 +0200
@@ -92,3 +92,41 @@
dup2(fd, 2);
if (fd > 2) close(fd);
}
+
+static int daemon_sig;
+
Gratuitous global variable. I've been trying to eliminate those. (I
just added libbuf analogous to toybuf but for use by lib/*.c,
presumably it could use that. But... is it worth doing?)
It seems inevitable to me to keep some global state for the signal
handling.
Post by Rob Landley
Post by Felix Janda
+static void daemon_sighandler(int sig)
+{
+ daemon_sig = sig;
+}
+
If we weren't messing with alarm, and instead of using the timeouts
built into select, would signals be our problem at all? (Might have to
calculate a timeout, but we do that anyway when actual data comes in if
we need a periodic action, and you can get data with a bad checksum
that causes spurious "data arrived, no wait it didn't" wakeups anyway.
Getting interrupted happens, we need to cope.)
(And still confused about the syscall having a timeout but us not using
it...)
SIG_ALRM was an afterthought when I realized that interrupting pselect
messes up the timeouts.

pselect is a replacement for the select + signal self-pipe.
Post by Rob Landley
Post by Felix Janda
+/*
+ * main loop for daemons listening to signals and file handles
+*/
+void daemon_main_loop(int nfds, fd_set *readfds, long timeout, int *signals,
+ void (*sighandler)(int), void
(*selecthandler)(fd_set *fds))
+{
+ fd_set copy;
+ sigset_t sigmask;
+ struct sigaction action;
+ int *i, ret;
+
+ sigemptyset(&sigmask);
+ action.sa_handler = daemon_sighandler;
+ sigemptyset(&action.sa_mask);
+ action.sa_flags = 0;
+ if (timeout) alarm(timeout);
+ for (i = signals; *i; i++) sigaddset(&sigmask, *i);
+ sigprocmask(SIG_SETMASK, &sigmask, 0);
+ for (i = signals; *i; i++) sigaction(*i, &action, 0);
Other than the difference in the precision of the timeout
argument, the
ready = ppoll(&fds, nfds, timeout_ts, &sigmask);
sigset_t origmask;
int timeout;
(timeout_ts.tv_sec * 1000 + timeout_ts.tv_nsec /
1000000);
sigprocmask(SIG_SETMASK, &sigmask, &origmask);
ready = poll(&fds, nfds, timeout);
sigprocmask(SIG_SETMASK, &origmask, NULL);
And yet a call to pselect needs its own sigprocmask() call as setup?
So... why are we using pselect again?
Didn't I say so? Before the call to pselect we block everything. Then
pselect atomically unblocks everything, does the select and afterwards
blocks everything again. So the only thing that can be interrupted by
signals is pselect. No need to think about race conditions. The relation
between pselect and select is the same as between ppoll and poll.

The main reason for not using ppoll is that it is linux only. pselect
is in POSIX. OpenBSD however doesn't seem to have it... The portable
way would be to use the self-pipe as before. But then there is even
more global state...
Post by Rob Landley
Post by Felix Janda
+ for (;;) {
+ memcpy(&copy, &readfds, sizeof(copy));
+ ret = pselect(nfds, &copy, 0, 0, 0, &action.sa_mask);
+ if (ret == -1) {
+ if (errno != EINTR) error_exit ("pselect");
+ if (timeout && daemon_sig == SIGALRM) alarm(timeout);
+ sighandler(daemon_sig);
Unconditionally calling sighandler(), so we don't have the option of it
being null and just getting the default behavior for signals. And the
caller can't have its own use of alarm outside this helper.
Yes, for the broader use it would be good to make the signal handling
optional (but that's actually the main point of this function). The caller
can have its own use of alarm if it doesn't use the one built into this
function.
Post by Rob Landley
Post by Felix Janda
+ } else selecthandler(&copy);
+ }
+}
diff -r 587b7572aac2 -r 752d22ece0fe toys/pending/dhcpd.c
--- a/toys/pending/dhcpd.c Sun Sep 01 15:13:58 2013 +0200
+++ b/toys/pending/dhcpd.c Sun Sep 01 15:25:08 2013 +0200
@@ -206,11 +206,9 @@
{"msstaticroutes" , DHCP_STCRTS | 0xf9, NULL, 0},
};
-struct fd_pair { int rd; int wr; };
static server_config_t gconfig;
static server_state_t gstate;
static uint8_t infomode;
-static struct fd_pair sigfd;
static int constone = 1;
Isn't an fd pair the common case? That's what netcat's doing, that's
what less is doing... tail cares about arbitrary numbers of
filehandles, but that says to me poll_pair_loop(rfd, wfd, callback,
timeout) would be a wrapper around poll_pair(pollfds, count, callback,
timeout). So you _can_ muck about in the guts of poll setup yourself,
but aren't required to.
fd pair is here aka self-pipe. dhcpd actually listens to only one socket
and signals. In the old version it listened to the socket and one end of
the signal self-pipe via select. syslogd needs to listen to arbitray many
sockets.
Post by Rob Landley
Post by Felix Janda
// calculate options size.
@@ -316,29 +314,6 @@
}
}
-// Generic signal handler real handling is done in main funcrion.
-static void signal_handler(int sig)
-{
- unsigned char ch = sig;
- if (write(sigfd.wr, &ch, 1) != 1) dbg("can't send signal\n");
-}
-
-// signal setup for SIGUSR1 SIGTERM
-static int setup_signal()
-{
- if (pipe((int *)&sigfd) < 0) {
- dbg("signal pipe failed\n");
- return -1;
- }
- fcntl(sigfd.wr , F_SETFD, FD_CLOEXEC);
- fcntl(sigfd.rd , F_SETFD, FD_CLOEXEC);
- int flags = fcntl(sigfd.wr, F_GETFL);
- fcntl(sigfd.wr, F_SETFL, flags | O_NONBLOCK);
- signal(SIGUSR1, signal_handler);
- signal(SIGTERM, signal_handler);
- return 0;
-}
-
It's nice to remove code...
Post by Felix Janda
// String STR to UINT32 conversion strored in VAR
static int strtou32(const char *str, void *var)
{
@@ -1073,15 +1048,114 @@
return ret;
}
+static void sig_handler(int sig)
+{
+ switch (sig) {
+ write_leasefile();
+ if (get_interface(gconfig.interface, &gconfig.ifindex,
&gconfig.server_nip, gconfig.server_mac)<0)
+ perror_exit("Interface lost %s\n", gconfig.interface);
+ gconfig.server_nip = htonl(gconfig.server_nip);
+ break;
+ infomsg(infomode, "Received SIGUSR1");
+ write_leasefile();
+ break;
+ infomsg(infomode, "Received SIGTERM");
+ write_leasefile();
+ unlink(gconfig.pidfile);
+ exit(0);
+ break;
+ }
+}
+
+static void select_handler(fd_set *rfds)
+{
+ uint8_t *optptr, msgtype = 0;
+ uint32_t serverid = 0, requested_nip = 0;
+ uint32_t reqested_lease = 0;
+ char *hstname = NULL;
+
+ dbg("select listen sock read\n");
+ if (read_packet() < 0) {
+ open_listensock();
+ return;
+ }
+ get_optval((uint8_t*)&gstate.rcvd_pkt.options,
DHCP_OPT_MESSAGE_TYPE, &gstate.rqcode);
+ if (gstate.rqcode == 0 || gstate.rqcode < DHCPDISCOVER
+ || gstate.rqcode > DHCPINFORM) {
+ dbg("no or bad message type option, ignoring packet.\n");
+ return;
+ }
+ get_optval((uint8_t*) &gstate.rcvd_pkt.options,
DHCP_OPT_SERVER_ID, &serverid);
+ if (serverid && (serverid != gconfig.server_nip)) {
+ dbg("server ID doesn't match, ignoring packet.\n");
+ return;
+ }
+ switch (gstate.rqcode) {
+ msgtype = DHCPOFFER;
+ dbg("Message Type : DHCPDISCOVER\n");
+ get_optval((uint8_t*) &gstate.rcvd_pkt.options,
DHCP_OPT_REQUESTED_IP, &requested_nip);
+ get_optval((uint8_t*) &gstate.rcvd_pkt.options,
DHCP_OPT_HOST_NAME, &hstname);
+ reqested_lease = gconfig.offer_time;
+ get_reqparam(&gstate.rqopt);
+ optptr = prepare_send_pkt();
+ gstate.send_pkt.yiaddr = getip_from_pool(requested_nip,
gstate.rcvd_pkt.chaddr, &reqested_lease, hstname);
+ if(!gstate.send_pkt.yiaddr){
+ msgtype = DHCPNAK;
+ optptr = set_optval(optptr, DHCP_OPT_MESSAGE_TYPE, &msgtype, 1);
+ send_packet(1);
+ break;
+ }
+ get_optval((uint8_t*) &gstate.rcvd_pkt.options,
DHCP_OPT_LEASE_TIME, &reqested_lease);
+ reqested_lease = htonl(get_lease(reqested_lease + time(NULL)));
+ optptr = set_optval(optptr, DHCP_OPT_MESSAGE_TYPE, &msgtype, 1);
+ optptr = set_optval(optptr, DHCP_OPT_SERVER_ID,
&gconfig.server_nip, 4);
+ optptr = set_optval(optptr, DHCP_OPT_LEASE_TIME,
&reqested_lease, 4);
+ optptr = set_reqparam(optptr, gstate.rqopt);
+ send_packet(1);
+ break;
+ msgtype = DHCPACK;
+ dbg("Message Type : DHCPREQUEST\n");
+ optptr = prepare_send_pkt();
+ get_optval((uint8_t*) &gstate.rcvd_pkt.options,
DHCP_OPT_REQUESTED_IP, &requested_nip);
+ get_optval((uint8_t*) &gstate.rcvd_pkt.options,
DHCP_OPT_LEASE_TIME, &reqested_lease);
+ get_optval((uint8_t*) &gstate.rcvd_pkt.options,
DHCP_OPT_HOST_NAME, &hstname);
+ gstate.send_pkt.yiaddr = getip_from_pool(requested_nip,
gstate.rcvd_pkt.chaddr, &reqested_lease, hstname);
+ if (!serverid) reqested_lease = gconfig.max_lease_sec;
+ if (!gstate.send_pkt.yiaddr) {
+ msgtype = DHCPNAK;
+ optptr = set_optval(optptr, DHCP_OPT_MESSAGE_TYPE, &msgtype, 1);
+ send_packet(1);
+ break;
+ }
+ optptr = set_optval(optptr, DHCP_OPT_MESSAGE_TYPE, &msgtype, 1);
+ optptr = set_optval(optptr, DHCP_OPT_SERVER_ID,
&gconfig.server_nip, 4);
+ reqested_lease = htonl(reqested_lease);
+ optptr = set_optval(optptr, DHCP_OPT_LEASE_TIME,
&reqested_lease, 4);
+ send_packet(1);
+ write_leasefile();
+ break;
+ case DHCPDECLINE:// FALL THROUGH
+ dbg("Message Type : DHCPDECLINE or DHCPRELEASE \n");
+ get_optval((uint8_t*) &gstate.rcvd_pkt.options,
DHCP_OPT_SERVER_ID, &serverid);
+ if (serverid != gconfig.server_nip) break;
+ get_optval((uint8_t*) &gstate.rcvd_pkt.options,
DHCP_OPT_REQUESTED_IP, &requested_nip);
+ delip_from_lease(requested_nip, gstate.rcvd_pkt.chaddr,
(gstate.rqcode==DHCPRELEASE)?0:gconfig.decline_time);
+ break;
+ dbg("Message Type : %u\n", gstate.rqcode);
+ break;
+ }
+}
+
Not sure it's a net win, though. Really just seems to be moving stuff
around...
$ diffstat felix2.patch
lib/lib.h | 3
lib/pending.c | 38 ++++++
toys/pending/dhcpd.c | 274
++++++++++++++++++++-----------------------------
toys/pending/syslogd.c | 141 ++++++++++---------------
4 files changed, 211 insertions(+), 245 deletions(-)
Ok, net win. (I need a lib/pending.h. Right now adding stuff to
lib/pending.c and then yanking it out again affects the commit history
of permanent files...)
I guess using signal() instead of sigaction() would save some lines. But I
didn't want to mix the new pselect with the old signal().
Post by Rob Landley
But this patch is doing a lot more than just introducing the new
library functions and making stuff use them, so I've got to do _more_
review before I can come to an actual conclusion about whether or not
to apply it...
It is really just moving stuff around. Luckily the local variables in the
main functions nicely broke up into the three seperate functions each.
Anyway, I still need to improve the restarting of syslogd.
Post by Rob Landley
I really do appreciate this work. Sorry to give you such a hard time
about it. I just want to get it _right_...
No problem. Please feel free to wait with coming back to this until after
the (impending?) release.

Felix
Rob Landley
2013-09-15 15:23:18 UTC
Permalink
Post by Felix Janda
Post by Rob Landley
And yet a call to pselect needs its own sigprocmask() call as setup?
So... why are we using pselect again?
Didn't I say so? Before the call to pselect we block everything.
But why do we do that? (So that only kill -9 will kill daemons?)
Post by Felix Janda
Then
pselect atomically unblocks everything, does the select and afterwards
blocks everything again. So the only thing that can be interrupted by
signals is pselect. No need to think about race conditions. The
relation
between pselect and select is the same as between ppoll and poll.
There was no need to think about signal handling race conditions in
netcat. I haven't read through the dhcpd or syslogd implementation very
closely yet, but I don't know what problem you're trying to solve here.
Post by Felix Janda
The main reason for not using ppoll is that it is linux only. pselect
is in POSIX. OpenBSD however doesn't seem to have it... The portable
way would be to use the self-pipe as before. But then there is even
more global state...
I don't understand what problem this addresses. I wrote a dnsd that
didn't need this (although it just handled udp, not tcp). I've written
my own little web servers, my own little vpn and a star topology
network redirector... Not remembering ever playing games with signals
like this for any of them?
Post by Felix Janda
Post by Rob Landley
Post by Felix Janda
+ for (;;) {
+ memcpy(&copy, &readfds, sizeof(copy));
+ ret = pselect(nfds, &copy, 0, 0, 0, &action.sa_mask);
+ if (ret == -1) {
+ if (errno != EINTR) error_exit ("pselect");
+ if (timeout && daemon_sig == SIGALRM) alarm(timeout);
+ sighandler(daemon_sig);
Unconditionally calling sighandler(), so we don't have the option
of it
Post by Rob Landley
being null and just getting the default behavior for signals. And
the
Post by Rob Landley
caller can't have its own use of alarm outside this helper.
Yes, for the broader use it would be good to make the signal handling
optional (but that's actually the main point of this function). The
caller
can have its own use of alarm if it doesn't use the one built into
this
function.
There's a timeout built into the blocking function. Possibly you have
to recalculate the timeout before calling the function again if you
care about next scheduled action instead of just "it took too long".
This is a heavyweight operation?
Post by Felix Janda
Post by Rob Landley
Ok, net win. (I need a lib/pending.h. Right now adding stuff to
lib/pending.c and then yanking it out again affects the commit
history
Post by Rob Landley
of permanent files...)
I guess using signal() instead of sigaction() would save some lines.
But I
didn't want to mix the new pselect with the old signal().
I still don't understand why you're doing all this stuff with signals.
Post by Felix Janda
Post by Rob Landley
But this patch is doing a lot more than just introducing the new
library functions and making stuff use them, so I've got to do
_more_
Post by Rob Landley
review before I can come to an actual conclusion about whether or
not
Post by Rob Landley
to apply it...
It is really just moving stuff around. Luckily the local variables in
the
main functions nicely broke up into the three seperate functions each.
Anyway, I still need to improve the restarting of syslogd.
Post by Rob Landley
I really do appreciate this work. Sorry to give you such a hard time
about it. I just want to get it _right_...
No problem. Please feel free to wait with coming back to this until
after
the (impending?) release.
I have an extra day in ohio (the linuxfest sunday content wasn't worth
staying for), so I'm trying to catch up on my email a bit and
downloading the LFS 7.4 files in the background...

Rob
Felix Janda
2013-09-17 17:59:26 UTC
Permalink
Post by Rob Landley
Post by Felix Janda
Post by Rob Landley
And yet a call to pselect needs its own sigprocmask() call as setup?
So... why are we using pselect again?
Didn't I say so? Before the call to pselect we block everything.
But why do we do that? (So that only kill -9 will kill daemons?)
So that we can precisely control when signals come in. If a signal occurs
during the time signals are blocked, it is pending to be delivered after
signals are unblocked, that's here during the pselect call. You don't need
kill -9 unless the program decides to go into an infinite loop or read()
from an fd with no data or something like that.
Post by Rob Landley
Post by Felix Janda
Then
pselect atomically unblocks everything, does the select and afterwards
blocks everything again. So the only thing that can be interrupted by
signals is pselect. No need to think about race conditions. The relation
between pselect and select is the same as between ppoll and poll.
There was no need to think about signal handling race conditions in
netcat. I haven't read through the dhcpd or syslogd implementation very
closely yet, but I don't know what problem you're trying to solve here.
From what I see netcat only uses SIGALRM. syslogd has SIGHUP, SIGTERM,
SIGINT and SIGQUIT - these can occur at any time, during a signal handler,
during a system call... (ok, SIGALRM can also interrupt a system call,
but in netcat it terminates the toy anyway.)

I was trying to replace the self-pipe logic in these toys by something
equivalent. Race conditions occur when you try to make your sighandlers
reentrant by making the sighandlers stubs and doing the real work in the
main loop.

I can't say that I've understood all of syslogd, yet... For dhcpd I've
only tested the signal handling.
Post by Rob Landley
Post by Felix Janda
The main reason for not using ppoll is that it is linux only. pselect
is in POSIX. OpenBSD however doesn't seem to have it... The portable
way would be to use the self-pipe as before. But then there is even
more global state...
I don't understand what problem this addresses. I wrote a dnsd that
didn't need this (although it just handled udp, not tcp). I've written
my own little web servers, my own little vpn and a star topology
network redirector... Not remembering ever playing games with signals
like this for any of them?
If they didn't do any signal handling at all, that might have been a
reason.

For syslogd one issue might be if two SIGHUPs (telling it to restart in
order to reread its configuration file) occur in quick succession so
that cleanup() is interrupted, and then called a second time which might
cause a double free().
Post by Rob Landley
Post by Felix Janda
Post by Rob Landley
Post by Felix Janda
+ for (;;) {
+ memcpy(&copy, &readfds, sizeof(copy));
+ ret = pselect(nfds, &copy, 0, 0, 0, &action.sa_mask);
+ if (ret == -1) {
+ if (errno != EINTR) error_exit ("pselect");
+ if (timeout && daemon_sig == SIGALRM) alarm(timeout);
+ sighandler(daemon_sig);
Unconditionally calling sighandler(), so we don't have the option
of it
Post by Rob Landley
being null and just getting the default behavior for signals. And
the
Post by Rob Landley
caller can't have its own use of alarm outside this helper.
Yes, for the broader use it would be good to make the signal handling
optional (but that's actually the main point of this function). The caller
can have its own use of alarm if it doesn't use the one built into this
function.
There's a timeout built into the blocking function. Possibly you have
to recalculate the timeout before calling the function again if you
care about next scheduled action instead of just "it took too long".
This is a heavyweight operation?
No. Is alarm() a heavyweight operation? (Unlike the wrapper function
the linux system call even updates the timeout like select() does.)
Post by Rob Landley
Post by Felix Janda
Post by Rob Landley
Ok, net win. (I need a lib/pending.h. Right now adding stuff to
lib/pending.c and then yanking it out again affects the commit
history
Post by Rob Landley
of permanent files...)
I guess using signal() instead of sigaction() would save some lines. But I
didn't want to mix the new pselect with the old signal().
I still don't understand why you're doing all this stuff with signals.
Post by Felix Janda
Post by Rob Landley
But this patch is doing a lot more than just introducing the new
library functions and making stuff use them, so I've got to do
_more_
Post by Rob Landley
review before I can come to an actual conclusion about whether or
not
Post by Rob Landley
to apply it...
It is really just moving stuff around. Luckily the local variables in the
main functions nicely broke up into the three seperate functions each.
Anyway, I still need to improve the restarting of syslogd.
Post by Rob Landley
I really do appreciate this work. Sorry to give you such a hard time
about it. I just want to get it _right_...
No problem. Please feel free to wait with coming back to this until after
the (impending?) release.
I have an extra day in ohio (the linuxfest sunday content wasn't worth
staying for), so I'm trying to catch up on my email a bit and
downloading the LFS 7.4 files in the background...
Rob
Felix
Felix Janda
2013-09-17 17:59:26 UTC
Permalink
Post by Rob Landley
Post by Felix Janda
Post by Rob Landley
And yet a call to pselect needs its own sigprocmask() call as setup?
So... why are we using pselect again?
Didn't I say so? Before the call to pselect we block everything.
But why do we do that? (So that only kill -9 will kill daemons?)
So that we can precisely control when signals come in. If a signal occurs
during the time signals are blocked, it is pending to be delivered after
signals are unblocked, that's here during the pselect call. You don't need
kill -9 unless the program decides to go into an infinite loop or read()
from an fd with no data or something like that.
Post by Rob Landley
Post by Felix Janda
Then
pselect atomically unblocks everything, does the select and afterwards
blocks everything again. So the only thing that can be interrupted by
signals is pselect. No need to think about race conditions. The relation
between pselect and select is the same as between ppoll and poll.
There was no need to think about signal handling race conditions in
netcat. I haven't read through the dhcpd or syslogd implementation very
closely yet, but I don't know what problem you're trying to solve here.
From what I see netcat only uses SIGALRM. syslogd has SIGHUP, SIGTERM,
SIGINT and SIGQUIT - these can occur at any time, during a signal handler,
during a system call... (ok, SIGALRM can also interrupt a system call,
but in netcat it terminates the toy anyway.)

I was trying to replace the self-pipe logic in these toys by something
equivalent. Race conditions occur when you try to make your sighandlers
reentrant by making the sighandlers stubs and doing the real work in the
main loop.

I can't say that I've understood all of syslogd, yet... For dhcpd I've
only tested the signal handling.
Post by Rob Landley
Post by Felix Janda
The main reason for not using ppoll is that it is linux only. pselect
is in POSIX. OpenBSD however doesn't seem to have it... The portable
way would be to use the self-pipe as before. But then there is even
more global state...
I don't understand what problem this addresses. I wrote a dnsd that
didn't need this (although it just handled udp, not tcp). I've written
my own little web servers, my own little vpn and a star topology
network redirector... Not remembering ever playing games with signals
like this for any of them?
If they didn't do any signal handling at all, that might have been a
reason.

For syslogd one issue might be if two SIGHUPs (telling it to restart in
order to reread its configuration file) occur in quick succession so
that cleanup() is interrupted, and then called a second time which might
cause a double free().
Post by Rob Landley
Post by Felix Janda
Post by Rob Landley
Post by Felix Janda
+ for (;;) {
+ memcpy(&copy, &readfds, sizeof(copy));
+ ret = pselect(nfds, &copy, 0, 0, 0, &action.sa_mask);
+ if (ret == -1) {
+ if (errno != EINTR) error_exit ("pselect");
+ if (timeout && daemon_sig == SIGALRM) alarm(timeout);
+ sighandler(daemon_sig);
Unconditionally calling sighandler(), so we don't have the option
of it
Post by Rob Landley
being null and just getting the default behavior for signals. And
the
Post by Rob Landley
caller can't have its own use of alarm outside this helper.
Yes, for the broader use it would be good to make the signal handling
optional (but that's actually the main point of this function). The caller
can have its own use of alarm if it doesn't use the one built into this
function.
There's a timeout built into the blocking function. Possibly you have
to recalculate the timeout before calling the function again if you
care about next scheduled action instead of just "it took too long".
This is a heavyweight operation?
No. Is alarm() a heavyweight operation? (Unlike the wrapper function
the linux system call even updates the timeout like select() does.)
Post by Rob Landley
Post by Felix Janda
Post by Rob Landley
Ok, net win. (I need a lib/pending.h. Right now adding stuff to
lib/pending.c and then yanking it out again affects the commit
history
Post by Rob Landley
of permanent files...)
I guess using signal() instead of sigaction() would save some lines. But I
didn't want to mix the new pselect with the old signal().
I still don't understand why you're doing all this stuff with signals.
Post by Felix Janda
Post by Rob Landley
But this patch is doing a lot more than just introducing the new
library functions and making stuff use them, so I've got to do
_more_
Post by Rob Landley
review before I can come to an actual conclusion about whether or
not
Post by Rob Landley
to apply it...
It is really just moving stuff around. Luckily the local variables in the
main functions nicely broke up into the three seperate functions each.
Anyway, I still need to improve the restarting of syslogd.
Post by Rob Landley
I really do appreciate this work. Sorry to give you such a hard time
about it. I just want to get it _right_...
No problem. Please feel free to wait with coming back to this until after
the (impending?) release.
I have an extra day in ohio (the linuxfest sunday content wasn't worth
staying for), so I'm trying to catch up on my email a bit and
downloading the LFS 7.4 files in the background...
Rob
Felix
Felix Janda
2013-09-01 13:54:27 UTC
Permalink
Both syslogd and dhcpd wait for sockets, signals or a timeout. Currently,
they use select() with its timeout to listen to the sockets and a
self-pipe for the signals.

The new library function instead uses pselect() without a timeout and
alarm(). With pselect() signals are only unblocked during this system
call; so the xwrappers don't need to be aware about interrupted syscalls.
alarm() is used instead of the timeout argument to pselect() so that
the length of the intervals between timeouts is not affected by being
interrupted.

On SIGHUP syslogd restarts. Previously everything relevant was contained
in syslogd_main() and the restarting was implemented by a goto. Now a
longjmp() would be necessary. I instead used an xexec(). Since xexec()
doesn't change the pid the pidfile needs to be unlinked() before
terminating so that xpidfile() doesn't complain. Furthermore if not run
in the foreground syslogd will daemonize() again when restarting. Could
one detect in daemonize() whether daemonizing is necessary?

Felix

# HG changeset patch
# User Felix Janda <felix.janda at posteo.de>
# Date 1378041908 -7200
# Node ID 752d22ece0fe3fa9763fd0b6e231e653cc173890
# Parent 587b7572aac2a64988503458155e1f2546672525
Factor out daemon select() loops into a library function

diff -r 587b7572aac2 -r 752d22ece0fe lib/lib.h
--- a/lib/lib.h Sun Sep 01 15:13:58 2013 +0200
+++ b/lib/lib.h Sun Sep 01 15:25:08 2013 +0200
@@ -201,4 +201,7 @@
char *astrcat (char *, char *);
char *xastrcat (char *, char *);

+// daemon helper functions
void daemonize(void);
+void daemon_main_loop(int nfds, fd_set *readfds, long timeout, int *signals,
+ void (*sighandler)(int), void (*selecthandler)(fd_set *fds));
diff -r 587b7572aac2 -r 752d22ece0fe lib/pending.c
--- a/lib/pending.c Sun Sep 01 15:13:58 2013 +0200
+++ b/lib/pending.c Sun Sep 01 15:25:08 2013 +0200
@@ -92,3 +92,41 @@
dup2(fd, 2);
if (fd > 2) close(fd);
}
+
+static int daemon_sig;
+
+static void daemon_sighandler(int sig)
+{
+ daemon_sig = sig;
+}
+
+/*
+ * main loop for daemons listening to signals and file handles
+*/
+void daemon_main_loop(int nfds, fd_set *readfds, long timeout, int *signals,
+ void (*sighandler)(int), void (*selecthandler)(fd_set *fds))
+{
+ fd_set copy;
+ sigset_t sigmask;
+ struct sigaction action;
+ int *i, ret;
+
+ sigemptyset(&sigmask);
+ action.sa_handler = daemon_sighandler;
+ sigemptyset(&action.sa_mask);
+ action.sa_flags = 0;
+ if (timeout) alarm(timeout);
+ for (i = signals; *i; i++) sigaddset(&sigmask, *i);
+ sigprocmask(SIG_SETMASK, &sigmask, 0);
+ for (i = signals; *i; i++) sigaction(*i, &action, 0);
+
+ for (;;) {
+ memcpy(&copy, &readfds, sizeof(copy));
+ ret = pselect(nfds, &copy, 0, 0, 0, &action.sa_mask);
+ if (ret == -1) {
+ if (errno != EINTR) error_exit ("pselect");
+ if (timeout && daemon_sig == SIGALRM) alarm(timeout);
+ sighandler(daemon_sig);
+ } else selecthandler(&copy);
+ }
+}
diff -r 587b7572aac2 -r 752d22ece0fe toys/pending/dhcpd.c
--- a/toys/pending/dhcpd.c Sun Sep 01 15:13:58 2013 +0200
+++ b/toys/pending/dhcpd.c Sun Sep 01 15:25:08 2013 +0200
@@ -206,11 +206,9 @@
{"msstaticroutes" , DHCP_STCRTS | 0xf9, NULL, 0},
};

-struct fd_pair { int rd; int wr; };
static server_config_t gconfig;
static server_state_t gstate;
static uint8_t infomode;
-static struct fd_pair sigfd;
static int constone = 1;

// calculate options size.
@@ -316,29 +314,6 @@
}
}

-// Generic signal handler real handling is done in main funcrion.
-static void signal_handler(int sig)
-{
- unsigned char ch = sig;
- if (write(sigfd.wr, &ch, 1) != 1) dbg("can't send signal\n");
-}
-
-// signal setup for SIGUSR1 SIGTERM
-static int setup_signal()
-{
- if (pipe((int *)&sigfd) < 0) {
- dbg("signal pipe failed\n");
- return -1;
- }
- fcntl(sigfd.wr , F_SETFD, FD_CLOEXEC);
- fcntl(sigfd.rd , F_SETFD, FD_CLOEXEC);
- int flags = fcntl(sigfd.wr, F_GETFL);
- fcntl(sigfd.wr, F_SETFL, flags | O_NONBLOCK);
- signal(SIGUSR1, signal_handler);
- signal(SIGTERM, signal_handler);
- return 0;
-}
-
// String STR to UINT32 conversion strored in VAR
static int strtou32(const char *str, void *var)
{
@@ -1073,15 +1048,114 @@
return ret;
}

+static void sig_handler(int sig)
+{
+ switch (sig) {
+ case SIGALRM:
+ write_leasefile();
+ if (get_interface(gconfig.interface, &gconfig.ifindex, &gconfig.server_nip, gconfig.server_mac)<0)
+ perror_exit("Interface lost %s\n", gconfig.interface);
+ gconfig.server_nip = htonl(gconfig.server_nip);
+ break;
+ case SIGUSR1:
+ infomsg(infomode, "Received SIGUSR1");
+ write_leasefile();
+ break;
+ case SIGTERM:
+ infomsg(infomode, "Received SIGTERM");
+ write_leasefile();
+ unlink(gconfig.pidfile);
+ exit(0);
+ break;
+ }
+}
+
+static void select_handler(fd_set *rfds)
+{
+ uint8_t *optptr, msgtype = 0;
+ uint32_t serverid = 0, requested_nip = 0;
+ uint32_t reqested_lease = 0;
+ char *hstname = NULL;
+
+ dbg("select listen sock read\n");
+ if (read_packet() < 0) {
+ open_listensock();
+ return;
+ }
+ get_optval((uint8_t*)&gstate.rcvd_pkt.options, DHCP_OPT_MESSAGE_TYPE, &gstate.rqcode);
+ if (gstate.rqcode == 0 || gstate.rqcode < DHCPDISCOVER
+ || gstate.rqcode > DHCPINFORM) {
+ dbg("no or bad message type option, ignoring packet.\n");
+ return;
+ }
+ get_optval((uint8_t*) &gstate.rcvd_pkt.options, DHCP_OPT_SERVER_ID, &serverid);
+ if (serverid && (serverid != gconfig.server_nip)) {
+ dbg("server ID doesn't match, ignoring packet.\n");
+ return;
+ }
+ switch (gstate.rqcode) {
+ case DHCPDISCOVER:
+ msgtype = DHCPOFFER;
+ dbg("Message Type : DHCPDISCOVER\n");
+ get_optval((uint8_t*) &gstate.rcvd_pkt.options, DHCP_OPT_REQUESTED_IP, &requested_nip);
+ get_optval((uint8_t*) &gstate.rcvd_pkt.options, DHCP_OPT_HOST_NAME, &hstname);
+ reqested_lease = gconfig.offer_time;
+ get_reqparam(&gstate.rqopt);
+ optptr = prepare_send_pkt();
+ gstate.send_pkt.yiaddr = getip_from_pool(requested_nip, gstate.rcvd_pkt.chaddr, &reqested_lease, hstname);
+ if(!gstate.send_pkt.yiaddr){
+ msgtype = DHCPNAK;
+ optptr = set_optval(optptr, DHCP_OPT_MESSAGE_TYPE, &msgtype, 1);
+ send_packet(1);
+ break;
+ }
+ get_optval((uint8_t*) &gstate.rcvd_pkt.options, DHCP_OPT_LEASE_TIME, &reqested_lease);
+ reqested_lease = htonl(get_lease(reqested_lease + time(NULL)));
+ optptr = set_optval(optptr, DHCP_OPT_MESSAGE_TYPE, &msgtype, 1);
+ optptr = set_optval(optptr, DHCP_OPT_SERVER_ID, &gconfig.server_nip, 4);
+ optptr = set_optval(optptr, DHCP_OPT_LEASE_TIME, &reqested_lease, 4);
+ optptr = set_reqparam(optptr, gstate.rqopt);
+ send_packet(1);
+ break;
+ case DHCPREQUEST:
+ msgtype = DHCPACK;
+ dbg("Message Type : DHCPREQUEST\n");
+ optptr = prepare_send_pkt();
+ get_optval((uint8_t*) &gstate.rcvd_pkt.options, DHCP_OPT_REQUESTED_IP, &requested_nip);
+ get_optval((uint8_t*) &gstate.rcvd_pkt.options, DHCP_OPT_LEASE_TIME, &reqested_lease);
+ get_optval((uint8_t*) &gstate.rcvd_pkt.options, DHCP_OPT_HOST_NAME, &hstname);
+ gstate.send_pkt.yiaddr = getip_from_pool(requested_nip, gstate.rcvd_pkt.chaddr, &reqested_lease, hstname);
+ if (!serverid) reqested_lease = gconfig.max_lease_sec;
+ if (!gstate.send_pkt.yiaddr) {
+ msgtype = DHCPNAK;
+ optptr = set_optval(optptr, DHCP_OPT_MESSAGE_TYPE, &msgtype, 1);
+ send_packet(1);
+ break;
+ }
+ optptr = set_optval(optptr, DHCP_OPT_MESSAGE_TYPE, &msgtype, 1);
+ optptr = set_optval(optptr, DHCP_OPT_SERVER_ID, &gconfig.server_nip, 4);
+ reqested_lease = htonl(reqested_lease);
+ optptr = set_optval(optptr, DHCP_OPT_LEASE_TIME, &reqested_lease, 4);
+ send_packet(1);
+ write_leasefile();
+ break;
+ case DHCPDECLINE:// FALL THROUGH
+ case DHCPRELEASE:
+ dbg("Message Type : DHCPDECLINE or DHCPRELEASE \n");
+ get_optval((uint8_t*) &gstate.rcvd_pkt.options, DHCP_OPT_SERVER_ID, &serverid);
+ if (serverid != gconfig.server_nip) break;
+ get_optval((uint8_t*) &gstate.rcvd_pkt.options, DHCP_OPT_REQUESTED_IP, &requested_nip);
+ delip_from_lease(requested_nip, gstate.rcvd_pkt.chaddr, (gstate.rqcode==DHCPRELEASE)?0:gconfig.decline_time);
+ break;
+ default:
+ dbg("Message Type : %u\n", gstate.rqcode);
+ break;
+ }
+}
+
void dhcpd_main(void)
{
- struct timeval tv;
- int retval;
- uint8_t *optptr, msgtype = 0;
- uint32_t waited = 0, serverid = 0, requested_nip = 0;
- uint32_t reqested_lease = 0, ip_pool_size = 0;
- char *hstname = NULL;
- fd_set rfds;
+ uint32_t ip_pool_size;

infomode = LOG_CONSOLE;
if (!(flag_chk(FLAG_f))) {
@@ -1111,137 +1185,15 @@
perror_exit("Failed to get interface %s", gconfig.interface);
gconfig.server_nip = htonl(gconfig.server_nip);

- setup_signal();
open_listensock();
fcntl(gstate.listensock, F_SETFD, FD_CLOEXEC);

- for (;;) {
- uint32_t timestmp = time(NULL);
- FD_ZERO(&rfds);
- FD_SET(gstate.listensock, &rfds);
- FD_SET(sigfd.rd, &rfds);
- tv.tv_sec = gconfig.auto_time - waited;
- tv.tv_usec = 0;
- retval = 0;
- serverid = 0;
- msgtype = 0;
+ fd_set rfds;
+ FD_ZERO(&rfds);
+ FD_SET(gstate.listensock, &rfds);

- int maxfd = (sigfd.rd > gstate.listensock)? sigfd.rd : gstate.listensock;
- dbg("select waiting ....\n");
- retval = select(maxfd + 1, &rfds, NULL, NULL, (gconfig.auto_time?&tv:NULL));
- if (retval < 0) {
- if (errno == EINTR) {
- waited += (unsigned) time(NULL) - timestmp;
- continue;
- }
- dbg("Error in select wait again...\n");
- continue;
- }
- if (!retval) { // Timed out
- dbg("select wait Timed Out...\n");
- waited = 0;
- write_leasefile();
- if (get_interface(gconfig.interface, &gconfig.ifindex, &gconfig.server_nip, gconfig.server_mac)<0)
- perror_exit("Interface lost %s\n", gconfig.interface);
- gconfig.server_nip = htonl(gconfig.server_nip);
- continue;
- }
- if (FD_ISSET(sigfd.rd, &rfds)) { // Some Activity on RDFDs : is signal
- unsigned char sig;
- if (read(sigfd.rd, &sig, 1) != 1) {
- dbg("signal read failed.\n");
- continue;
- }
- switch (sig) {
- case SIGUSR1:
- infomsg(infomode, "Received SIGUSR1");
- write_leasefile();
- continue;
- case SIGTERM:
- infomsg(infomode, "Received SIGTERM");
- write_leasefile();
- unlink(gconfig.pidfile);
- exit(0);
- break;
- default: break;
- }
- }
- if (FD_ISSET(gstate.listensock, &rfds)) { // Some Activity on RDFDs : is socket
- dbg("select listen sock read\n");
- if (read_packet() < 0) {
- open_listensock();
- continue;
- }
- waited += time(NULL) - timestmp;
- get_optval((uint8_t*)&gstate.rcvd_pkt.options, DHCP_OPT_MESSAGE_TYPE, &gstate.rqcode);
- if (gstate.rqcode == 0 || gstate.rqcode < DHCPDISCOVER
- || gstate.rqcode > DHCPINFORM) {
- dbg("no or bad message type option, ignoring packet.\n");
- continue;
- }
- get_optval((uint8_t*) &gstate.rcvd_pkt.options, DHCP_OPT_SERVER_ID, &serverid);
- if (serverid && (serverid != gconfig.server_nip)) {
- dbg("server ID doesn't match, ignoring packet.\n");
- continue;
- }
- switch (gstate.rqcode) {
- case DHCPDISCOVER:
- msgtype = DHCPOFFER;
- dbg("Message Type : DHCPDISCOVER\n");
- get_optval((uint8_t*) &gstate.rcvd_pkt.options, DHCP_OPT_REQUESTED_IP, &requested_nip);
- get_optval((uint8_t*) &gstate.rcvd_pkt.options, DHCP_OPT_HOST_NAME, &hstname);
- reqested_lease = gconfig.offer_time;
- get_reqparam(&gstate.rqopt);
- optptr = prepare_send_pkt();
- gstate.send_pkt.yiaddr = getip_from_pool(requested_nip, gstate.rcvd_pkt.chaddr, &reqested_lease, hstname);
- if(!gstate.send_pkt.yiaddr){
- msgtype = DHCPNAK;
- optptr = set_optval(optptr, DHCP_OPT_MESSAGE_TYPE, &msgtype, 1);
- send_packet(1);
- break;
- }
- get_optval((uint8_t*) &gstate.rcvd_pkt.options, DHCP_OPT_LEASE_TIME, &reqested_lease);
- reqested_lease = htonl(get_lease(reqested_lease + time(NULL)));
- optptr = set_optval(optptr, DHCP_OPT_MESSAGE_TYPE, &msgtype, 1);
- optptr = set_optval(optptr, DHCP_OPT_SERVER_ID, &gconfig.server_nip, 4);
- optptr = set_optval(optptr, DHCP_OPT_LEASE_TIME, &reqested_lease, 4);
- optptr = set_reqparam(optptr, gstate.rqopt);
- send_packet(1);
- break;
- case DHCPREQUEST:
- msgtype = DHCPACK;
- dbg("Message Type : DHCPREQUEST\n");
- optptr = prepare_send_pkt();
- get_optval((uint8_t*) &gstate.rcvd_pkt.options, DHCP_OPT_REQUESTED_IP, &requested_nip);
- get_optval((uint8_t*) &gstate.rcvd_pkt.options, DHCP_OPT_LEASE_TIME, &reqested_lease);
- get_optval((uint8_t*) &gstate.rcvd_pkt.options, DHCP_OPT_HOST_NAME, &hstname);
- gstate.send_pkt.yiaddr = getip_from_pool(requested_nip, gstate.rcvd_pkt.chaddr, &reqested_lease, hstname);
- if (!serverid) reqested_lease = gconfig.max_lease_sec;
- if (!gstate.send_pkt.yiaddr) {
- msgtype = DHCPNAK;
- optptr = set_optval(optptr, DHCP_OPT_MESSAGE_TYPE, &msgtype, 1);
- send_packet(1);
- break;
- }
- optptr = set_optval(optptr, DHCP_OPT_MESSAGE_TYPE, &msgtype, 1);
- optptr = set_optval(optptr, DHCP_OPT_SERVER_ID, &gconfig.server_nip, 4);
- reqested_lease = htonl(reqested_lease);
- optptr = set_optval(optptr, DHCP_OPT_LEASE_TIME, &reqested_lease, 4);
- send_packet(1);
- write_leasefile();
- break;
- case DHCPDECLINE:// FALL THROUGH
- case DHCPRELEASE:
- dbg("Message Type : DHCPDECLINE or DHCPRELEASE \n");
- get_optval((uint8_t*) &gstate.rcvd_pkt.options, DHCP_OPT_SERVER_ID, &serverid);
- if (serverid != gconfig.server_nip) break;
- get_optval((uint8_t*) &gstate.rcvd_pkt.options, DHCP_OPT_REQUESTED_IP, &requested_nip);
- delip_from_lease(requested_nip, gstate.rcvd_pkt.chaddr, (gstate.rqcode==DHCPRELEASE)?0:gconfig.decline_time);
- break;
- default:
- dbg("Message Type : %u\n", gstate.rqcode);
- break;
- }
- }
- }
+ dbg("enter main loop\n");
+ int signals[] = {SIGALRM, SIGUSR1, SIGTERM, 0};
+ daemon_main_loop(gstate.listensock + 1, &rfds, gconfig.auto_time, signals,
+ sig_handler, select_handler);
}
diff -r 587b7572aac2 -r 752d22ece0fe toys/pending/syslogd.c
--- a/toys/pending/syslogd.c Sun Sep 01 15:13:58 2013 +0200
+++ b/toys/pending/syslogd.c Sun Sep 01 15:25:08 2013 +0200
@@ -68,7 +68,6 @@

struct unsocks *lsocks; // list of listen sockets
struct logfile *lfiles; // list of write logfiles
- int sigfd[2];
)

// Lookup numerical code from name
@@ -388,28 +387,71 @@
TT.lfiles = fnode->next;
free(fnode);
}
+ unlink("/var/run/syslogd.pid");
}

-static void signal_handler(int sig)
+static void sighandler(int sig)
{
- unsigned char ch = sig;
- if (write(TT.sigfd[1], &ch, 1) != 1) error_msg("can't send signal");
+ switch(sig) {
+ case SIGALRM:
+ logmsg("<46>-- MARK --", 14);
+ break;
+ case SIGTERM: /* FALLTHROUGH */
+ case SIGINT: /* FALLTHROUGH */
+ case SIGQUIT:
+ logmsg("<46>syslogd exiting", 19);
+ if (CFG_TOYBOX_FREE ) cleanup();
+ signal(sig, SIG_DFL);
+ sigset_t ss;
+ sigemptyset(&ss);
+ sigaddset(&ss, sig);
+ sigprocmask(SIG_UNBLOCK, &ss, NULL);
+ raise(sig);
+ _exit(1); /* Should not reach it */
+ break;
+ case SIGHUP:
+ logmsg("<46>syslogd exiting", 19);
+ cleanup(); //cleanup is done, as we restart syslog.
+ xexec(toys.argv);
+ }
+}
+
+static void selecthandler(fd_set *rfds)
+{
+ struct unsocks *tsd;
+ char *buffer = (toybuf + 2048), *last_buf = (toybuf + 3072); //these two buffs are of 1K each
+ int last_len=0;
+
+ for (tsd = TT.lsocks; tsd; tsd = tsd->next) {
+ int sd = tsd->sd;
+ if (FD_ISSET(sd, rfds)) {
+ int len = read(sd, buffer, 1023); //buffer is of 1K, hence readingonly 1023 bytes, 1 for NUL
+ if (len > 0) {
+ buffer[len] = '\0';
+ if((toys.optflags & FLAG_D) && (len == last_len))
+ if (!memcmp(last_buf, buffer, len)) break;
+
+ memcpy(last_buf, buffer, len);
+ last_len = len;
+ logmsg(buffer, len);
+ }
+ break;
+ }
+ }
}

void syslogd_main(void)
{
struct unsocks *tsd;
- int nfds, retval, last_len=0;
- struct timeval tv;
+ int nfds;
fd_set rfds; // fds for reading
- char *temp, *buffer = (toybuf +2048), *last_buf = (toybuf + 3072); //these two buffs are of 1K each
+ char *temp;

if ((toys.optflags & FLAG_p) && (strlen(TT.unix_socket) > 108))
error_exit("Socket path should not be more than 108");

TT.config_file = (toys.optflags & FLAG_f) ?
TT.config_file : "/etc/syslog.conf"; //DEFCONFFILE
-init_jumpin:
tsd = xzalloc(sizeof(struct unsocks));

tsd->path = (toys.optflags & FLAG_p) ? TT.unix_socket : "/dev/log"; // DEFLOGSOCK
@@ -445,94 +487,25 @@
continue;
}
chmod(tsd->path, 0777);
- nfds++;
+ nfds = tsd->sd;
}
if (!nfds) {
error_msg("Can't open single socket for listenning.");
goto clean_and_exit;
}

- // Setup signals
- if (pipe(TT.sigfd) < 0) error_exit("pipe failed\n");
-
- fcntl(TT.sigfd[1] , F_SETFD, FD_CLOEXEC);
- fcntl(TT.sigfd[0] , F_SETFD, FD_CLOEXEC);
- int flags = fcntl(TT.sigfd[1], F_GETFL);
- fcntl(TT.sigfd[1], F_SETFL, flags | O_NONBLOCK);
- signal(SIGHUP, signal_handler);
- signal(SIGTERM, signal_handler);
- signal(SIGINT, signal_handler);
- signal(SIGQUIT, signal_handler);
-
if (parse_config_file() == -1) goto clean_and_exit;
open_logfiles();
- if (!(toys.optflags & FLAG_n)) {
- daemonize();
- //don't daemonize again if SIGHUP received.
- toys.optflags |= FLAG_n;
- }
+ if (!(toys.optflags & FLAG_n)) daemonize();
xpidfile("syslogd");

logmsg("<46>Toybox: syslogd started", 27); //27 : the length of message
- for (;;) {
- // Add opened socks to rfds for select()
- FD_ZERO(&rfds);
- for (tsd = TT.lsocks; tsd; tsd = tsd->next) FD_SET(tsd->sd, &rfds);
- FD_SET(TT.sigfd[0], &rfds);
- tv.tv_usec = 0;
- tv.tv_sec = TT.interval*60;

- retval = select(TT.sigfd[0] + 1, &rfds, NULL, NULL, (TT.interval)?&tv:NULL);
- if (retval < 0) {
- if (errno != EINTR) perror_msg("Error in select ");
- }
- else if (!retval) logmsg("<46>-- MARK --", 14);
- else if (FD_ISSET(TT.sigfd[0], &rfds)) { /* May be a signal */
- unsigned char sig;
-
- if (read(TT.sigfd[0], &sig, 1) != 1) {
- error_msg("signal read failed.\n");
- continue;
- }
- switch(sig) {
- case SIGTERM: /* FALLTHROUGH */
- case SIGINT: /* FALLTHROUGH */
- case SIGQUIT:
- logmsg("<46>syslogd exiting", 19);
- if (CFG_TOYBOX_FREE ) cleanup();
- signal(sig, SIG_DFL);
- sigset_t ss;
- sigemptyset(&ss);
- sigaddset(&ss, sig);
- sigprocmask(SIG_UNBLOCK, &ss, NULL);
- raise(sig);
- _exit(1); /* Should not reach it */
- break;
- case SIGHUP:
- logmsg("<46>syslogd exiting", 19);
- cleanup(); //cleanup is done, as we restart syslog.
- goto init_jumpin;
- default: break;
- }
- } else { /* Some activity on listen sockets. */
- for (tsd = TT.lsocks; tsd; tsd = tsd->next) {
- int sd = tsd->sd;
- if (FD_ISSET(sd, &rfds)) {
- int len = read(sd, buffer, 1023); //buffer is of 1K, hence readingonly 1023 bytes, 1 for NUL
- if (len > 0) {
- buffer[len] = '\0';
- if((toys.optflags & FLAG_D) && (len == last_len))
- if (!memcmp(last_buf, buffer, len)) break;
-
- memcpy(last_buf, buffer, len);
- last_len = len;
- logmsg(buffer, len);
- }
- break;
- }
- }
- }
- }
+ FD_ZERO(&rfds);
+ for (tsd = TT.lsocks; tsd; tsd = tsd->next) FD_SET(tsd->sd, &rfds);
+ int signals[] = {SIGALRM, SIGHUP, SIGTERM, SIGINT, SIGQUIT, 0};
+ daemon_main_loop(nfds + 1, &rfds, TT.interval*60, signals,
+ sighandler, selecthandler);
clean_and_exit:
logmsg("<46>syslogd exiting", 19);
if (CFG_TOYBOX_FREE ) cleanup();
Rob Landley
2013-09-06 11:19:47 UTC
Permalink
Post by Felix Janda
Both syslogd and dhcpd wait for sockets, signals or a timeout.
Currently,
they use select() with its timeout to listen to the sockets and a
self-pipe for the signals.
The new library function instead uses pselect() without a timeout and
alarm(). With pselect() signals are only unblocked during this system
call; so the xwrappers don't need to be aware about interrupted
syscalls.
Are we normally blocking a lot of signals in other commands? Using the
6 argument version instead of the 5 argument version addresses an issue
for which existing command?
Post by Felix Janda
alarm() is used instead of the timeout argument to pselect() so that
the length of the intervals between timeouts is not affected by being
interrupted.
QEMU recently changed its timer system to _not_ use alarm:

http://blog.alex.org.uk/2013/08/24/changes-to-qemus-timer-system/

I'm not convinced sending signals to ourselves is preferable to telling
the system call when to timeout. Of the three options (select adjusting
its timespec, us manually reading the clock and feeding pselect a
timeout based on when we next want to wake up, and us sending ourselves
an alarm), us sending ourselves an alarm and constantly having to reset
it seems like it has the most moving parts and the most things that can
go wrong.

Ok, maybe I've been slightly burned by SIGALRM before:

http://landley.net/notes-2011.html#05-09-2011
Post by Felix Janda
On SIGHUP syslogd restarts. Previously everything relevant was
contained
in syslogd_main() and the restarting was implemented by a goto. Now a
longjmp() would be necessary. I instead used an xexec().
Since commit 955 (and pondering toysh to finally tackle that sucker
within a finite amount of time) I'm a bit worried that restarting a
command without OS involvement might misbehave if the signal mask gets
corrupted, or there are funky environment variables, or memory leaks
build up, or we have leftover filehandles, or a couple dozen other
things.

We haven't got "generic reset this process's state" code, longjmp()
leaves a lot of debris, and xexec() callign a new main doesn't clear
the old stuff off the stack; do that in a tight enough loop for long
enough and eventually you will run out of stack even in a normal linux
userspace. (Default's 8 megs: ulimit -s says 8192 and the bash man page
says the units are kilobytes.)

I added NOFORK because some commands (like cd) _must_ happen in the
current toysh process context, but also because I could very very
carefully audit commands to make sure it was safe to call them in the
same process context and continue when they return. But these commands
are in pending because I haven't even done an initial audit pass on
them.

Discarding your process on restart and execing a new one covers a
multitude of sins. Possibly syslogd is clean enough it can restart
itself without accumulating trash, but as a long-lived daemon I have
yet to closely read (hence it being in the pending directory), this
does not fill me with confidence.

Probably just the "it's 5am, sun coming up soon" things. I should step
away from the keyboard...
Post by Felix Janda
Since xexec()
doesn't change the pid the pidfile needs to be unlinked() before
terminating so that xpidfile() doesn't complain.
Um... I agree this is a symptom of a problem. I'm not sure that's the
right fix. The restart is maintaining state and then getting confused
by the state it's maintaining...

(I can just see some horrible systemd thing hitting a race window where
the PID file wasn't there and trying to restart the process...)
Post by Felix Janda
Furthermore if not run
in the foreground syslogd will daemonize() again when restarting.
Could
one detect in daemonize() whether daemonizing is necessary?
"The restart is maintaining state and then getting..."

Are these the _only_ instances of this? Is a main loop that exits and
restarts itself but avoids initial setup better?

Is there a clean way to do this where we _don't_ have to be absolutely
sure we've thought of everything ever? execv("/proc/self/exe",
toys.argc) and then fall back to something else if it returns, perhaps?
Or can we get an exhaustive list of all process resources (from the
container suspend/resume guys, presumably) and check for ourselves that
we're not leaking anything?

Very old concern of mine, a full decade before containers:

http://lkml.indiana.edu/hypermail/linux/kernel/0206.2/0835.html
Post by Felix Janda
Felix
# HG changeset patch
# User Felix Janda <felix.janda at posteo.de>
# Date 1378041908 -7200
# Node ID 752d22ece0fe3fa9763fd0b6e231e653cc173890
# Parent 587b7572aac2a64988503458155e1f2546672525
Factor out daemon select() loops into a library function
Ok, the new select() lib function I'd probably be up for reviewing on
its own, and the cleanups to a function I haven't even read yet I'd
just pass through and read/cleanup the new baseline when I got around
to it.

But both at once? Not up for that right now. Might try again in the
morning...

Rob
Rob Landley
2013-09-06 16:46:26 UTC
Permalink
Post by Felix Janda
diff -r 587b7572aac2 -r 752d22ece0fe lib/lib.h
--- a/lib/lib.h Sun Sep 01 15:13:58 2013 +0200
+++ b/lib/lib.h Sun Sep 01 15:25:08 2013 +0200
@@ -201,4 +201,7 @@
char *astrcat (char *, char *);
char *xastrcat (char *, char *);
+// daemon helper functions
void daemonize(void);
+void daemon_main_loop(int nfds, fd_set *readfds, long timeout, int
*signals,
+ void (*sighandler)(int), void
(*selecthandler)(fd_set *fds));
It's not really "daemon", is it? Netcat isn't a daemon, less isn't a
daemon, tail -f isn't a daemon... All of 'em could potentially benefit
from a select loop. It's pretty much just "select_loop()".

Also, the current code in netcat is using poll(), not select(). (Array
of file descriptors vs magic sized bitfield where you have to calculate
nfds, which _this_ function is asking the caller to do. Neither one
actually scales to thousands of filehandles gracefully, but if that's
an issue it's probably not common library code the one or two
filehandle case is going to want to use...)
Post by Felix Janda
diff -r 587b7572aac2 -r 752d22ece0fe lib/pending.c
--- a/lib/pending.c Sun Sep 01 15:13:58 2013 +0200
+++ b/lib/pending.c Sun Sep 01 15:25:08 2013 +0200
@@ -92,3 +92,41 @@
dup2(fd, 2);
if (fd > 2) close(fd);
}
+
+static int daemon_sig;
+
Gratuitous global variable. I've been trying to eliminate those. (I
just added libbuf analogous to toybuf but for use by lib/*.c,
presumably it could use that. But... is it worth doing?)
Post by Felix Janda
+static void daemon_sighandler(int sig)
+{
+ daemon_sig = sig;
+}
+
If we weren't messing with alarm, and instead of using the timeouts
built into select, would signals be our problem at all? (Might have to
calculate a timeout, but we do that anyway when actual data comes in if
we need a periodic action, and you can get data with a bad checksum
that causes spurious "data arrived, no wait it didn't" wakeups anyway.
Getting interrupted happens, we need to cope.)

(And still confused about the syscall having a timeout but us not using
it...)
Post by Felix Janda
+/*
+ * main loop for daemons listening to signals and file handles
+*/
+void daemon_main_loop(int nfds, fd_set *readfds, long timeout, int
*signals,
+ void (*sighandler)(int), void
(*selecthandler)(fd_set *fds))
+{
+ fd_set copy;
+ sigset_t sigmask;
+ struct sigaction action;
+ int *i, ret;
+
+ sigemptyset(&sigmask);
+ action.sa_handler = daemon_sighandler;
+ sigemptyset(&action.sa_mask);
+ action.sa_flags = 0;
+ if (timeout) alarm(timeout);
+ for (i = signals; *i; i++) sigaddset(&sigmask, *i);
+ sigprocmask(SIG_SETMASK, &sigmask, 0);
+ for (i = signals; *i; i++) sigaction(*i, &action, 0);
To quote from the ppoll man page:

Other than the difference in the precision of the timeout
argument, the
following ppoll() call:

ready = ppoll(&fds, nfds, timeout_ts, &sigmask);

is equivalent to atomically executing the following calls:

sigset_t origmask;
int timeout;

timeout = (timeout_ts == NULL) ? -1 :
(timeout_ts.tv_sec * 1000 + timeout_ts.tv_nsec /
1000000);
sigprocmask(SIG_SETMASK, &sigmask, &origmask);
ready = poll(&fds, nfds, timeout);
sigprocmask(SIG_SETMASK, &origmask, NULL);

And yet a call to pselect needs its own sigprocmask() call as setup?
So... why are we using pselect again?
Post by Felix Janda
+ for (;;) {
+ memcpy(&copy, &readfds, sizeof(copy));
+ ret = pselect(nfds, &copy, 0, 0, 0, &action.sa_mask);
+ if (ret == -1) {
+ if (errno != EINTR) error_exit ("pselect");
+ if (timeout && daemon_sig == SIGALRM) alarm(timeout);
+ sighandler(daemon_sig);
Unconditionally calling sighandler(), so we don't have the option of it
being null and just getting the default behavior for signals. And the
caller can't have its own use of alarm outside this helper.
Post by Felix Janda
+ } else selecthandler(&copy);
+ }
+}
diff -r 587b7572aac2 -r 752d22ece0fe toys/pending/dhcpd.c
--- a/toys/pending/dhcpd.c Sun Sep 01 15:13:58 2013 +0200
+++ b/toys/pending/dhcpd.c Sun Sep 01 15:25:08 2013 +0200
@@ -206,11 +206,9 @@
{"msstaticroutes" , DHCP_STCRTS | 0xf9, NULL, 0},
};
-struct fd_pair { int rd; int wr; };
static server_config_t gconfig;
static server_state_t gstate;
static uint8_t infomode;
-static struct fd_pair sigfd;
static int constone = 1;
Isn't an fd pair the common case? That's what netcat's doing, that's
what less is doing... tail cares about arbitrary numbers of
filehandles, but that says to me poll_pair_loop(rfd, wfd, callback,
timeout) would be a wrapper around poll_pair(pollfds, count, callback,
timeout). So you _can_ muck about in the guts of poll setup yourself,
but aren't required to.
Post by Felix Janda
// calculate options size.
@@ -316,29 +314,6 @@
}
}
-// Generic signal handler real handling is done in main funcrion.
-static void signal_handler(int sig)
-{
- unsigned char ch = sig;
- if (write(sigfd.wr, &ch, 1) != 1) dbg("can't send signal\n");
-}
-
-// signal setup for SIGUSR1 SIGTERM
-static int setup_signal()
-{
- if (pipe((int *)&sigfd) < 0) {
- dbg("signal pipe failed\n");
- return -1;
- }
- fcntl(sigfd.wr , F_SETFD, FD_CLOEXEC);
- fcntl(sigfd.rd , F_SETFD, FD_CLOEXEC);
- int flags = fcntl(sigfd.wr, F_GETFL);
- fcntl(sigfd.wr, F_SETFL, flags | O_NONBLOCK);
- signal(SIGUSR1, signal_handler);
- signal(SIGTERM, signal_handler);
- return 0;
-}
-
It's nice to remove code...
Post by Felix Janda
// String STR to UINT32 conversion strored in VAR
static int strtou32(const char *str, void *var)
{
@@ -1073,15 +1048,114 @@
return ret;
}
+static void sig_handler(int sig)
+{
+ switch (sig) {
+ write_leasefile();
+ if (get_interface(gconfig.interface, &gconfig.ifindex,
&gconfig.server_nip, gconfig.server_mac)<0)
+ perror_exit("Interface lost %s\n", gconfig.interface);
+ gconfig.server_nip = htonl(gconfig.server_nip);
+ break;
+ infomsg(infomode, "Received SIGUSR1");
+ write_leasefile();
+ break;
+ infomsg(infomode, "Received SIGTERM");
+ write_leasefile();
+ unlink(gconfig.pidfile);
+ exit(0);
+ break;
+ }
+}
+
+static void select_handler(fd_set *rfds)
+{
+ uint8_t *optptr, msgtype = 0;
+ uint32_t serverid = 0, requested_nip = 0;
+ uint32_t reqested_lease = 0;
+ char *hstname = NULL;
+
+ dbg("select listen sock read\n");
+ if (read_packet() < 0) {
+ open_listensock();
+ return;
+ }
+ get_optval((uint8_t*)&gstate.rcvd_pkt.options,
DHCP_OPT_MESSAGE_TYPE, &gstate.rqcode);
+ if (gstate.rqcode == 0 || gstate.rqcode < DHCPDISCOVER
+ || gstate.rqcode > DHCPINFORM) {
+ dbg("no or bad message type option, ignoring packet.\n");
+ return;
+ }
+ get_optval((uint8_t*) &gstate.rcvd_pkt.options,
DHCP_OPT_SERVER_ID, &serverid);
+ if (serverid && (serverid != gconfig.server_nip)) {
+ dbg("server ID doesn't match, ignoring packet.\n");
+ return;
+ }
+ switch (gstate.rqcode) {
+ msgtype = DHCPOFFER;
+ dbg("Message Type : DHCPDISCOVER\n");
+ get_optval((uint8_t*) &gstate.rcvd_pkt.options,
DHCP_OPT_REQUESTED_IP, &requested_nip);
+ get_optval((uint8_t*) &gstate.rcvd_pkt.options,
DHCP_OPT_HOST_NAME, &hstname);
+ reqested_lease = gconfig.offer_time;
+ get_reqparam(&gstate.rqopt);
+ optptr = prepare_send_pkt();
+ gstate.send_pkt.yiaddr = getip_from_pool(requested_nip,
gstate.rcvd_pkt.chaddr, &reqested_lease, hstname);
+ if(!gstate.send_pkt.yiaddr){
+ msgtype = DHCPNAK;
+ optptr = set_optval(optptr, DHCP_OPT_MESSAGE_TYPE, &msgtype,
1);
+ send_packet(1);
+ break;
+ }
+ get_optval((uint8_t*) &gstate.rcvd_pkt.options,
DHCP_OPT_LEASE_TIME, &reqested_lease);
+ reqested_lease = htonl(get_lease(reqested_lease + time(NULL)));
+ optptr = set_optval(optptr, DHCP_OPT_MESSAGE_TYPE, &msgtype,
1);
+ optptr = set_optval(optptr, DHCP_OPT_SERVER_ID,
&gconfig.server_nip, 4);
+ optptr = set_optval(optptr, DHCP_OPT_LEASE_TIME,
&reqested_lease, 4);
+ optptr = set_reqparam(optptr, gstate.rqopt);
+ send_packet(1);
+ break;
+ msgtype = DHCPACK;
+ dbg("Message Type : DHCPREQUEST\n");
+ optptr = prepare_send_pkt();
+ get_optval((uint8_t*) &gstate.rcvd_pkt.options,
DHCP_OPT_REQUESTED_IP, &requested_nip);
+ get_optval((uint8_t*) &gstate.rcvd_pkt.options,
DHCP_OPT_LEASE_TIME, &reqested_lease);
+ get_optval((uint8_t*) &gstate.rcvd_pkt.options,
DHCP_OPT_HOST_NAME, &hstname);
+ gstate.send_pkt.yiaddr = getip_from_pool(requested_nip,
gstate.rcvd_pkt.chaddr, &reqested_lease, hstname);
+ if (!serverid) reqested_lease = gconfig.max_lease_sec;
+ if (!gstate.send_pkt.yiaddr) {
+ msgtype = DHCPNAK;
+ optptr = set_optval(optptr, DHCP_OPT_MESSAGE_TYPE, &msgtype,
1);
+ send_packet(1);
+ break;
+ }
+ optptr = set_optval(optptr, DHCP_OPT_MESSAGE_TYPE, &msgtype,
1);
+ optptr = set_optval(optptr, DHCP_OPT_SERVER_ID,
&gconfig.server_nip, 4);
+ reqested_lease = htonl(reqested_lease);
+ optptr = set_optval(optptr, DHCP_OPT_LEASE_TIME,
&reqested_lease, 4);
+ send_packet(1);
+ write_leasefile();
+ break;
+ case DHCPDECLINE:// FALL THROUGH
+ dbg("Message Type : DHCPDECLINE or DHCPRELEASE \n");
+ get_optval((uint8_t*) &gstate.rcvd_pkt.options,
DHCP_OPT_SERVER_ID, &serverid);
+ if (serverid != gconfig.server_nip) break;
+ get_optval((uint8_t*) &gstate.rcvd_pkt.options,
DHCP_OPT_REQUESTED_IP, &requested_nip);
+ delip_from_lease(requested_nip, gstate.rcvd_pkt.chaddr,
(gstate.rqcode==DHCPRELEASE)?0:gconfig.decline_time);
+ break;
+ dbg("Message Type : %u\n", gstate.rqcode);
+ break;
+ }
+}
+
Not sure it's a net win, though. Really just seems to be moving stuff
around...

Let's see, ctrl-alt-diffstat:

$ diffstat felix2.patch
lib/lib.h | 3
lib/pending.c | 38 ++++++
toys/pending/dhcpd.c | 274
++++++++++++++++++++-----------------------------
toys/pending/syslogd.c | 141 ++++++++++---------------
4 files changed, 211 insertions(+), 245 deletions(-)

Ok, net win. (I need a lib/pending.h. Right now adding stuff to
lib/pending.c and then yanking it out again affects the commit history
of permanent files...)

But this patch is doing a lot more than just introducing the new
library functions and making stuff use them, so I've got to do _more_
review before I can come to an actual conclusion about whether or not
to apply it...

I really do appreciate this work. Sorry to give you such a hard time
about it. I just want to get it _right_...

Rob
Rob Landley
2013-09-15 15:23:18 UTC
Permalink
Post by Felix Janda
Post by Rob Landley
And yet a call to pselect needs its own sigprocmask() call as setup?
So... why are we using pselect again?
Didn't I say so? Before the call to pselect we block everything.
But why do we do that? (So that only kill -9 will kill daemons?)
Post by Felix Janda
Then
pselect atomically unblocks everything, does the select and afterwards
blocks everything again. So the only thing that can be interrupted by
signals is pselect. No need to think about race conditions. The
relation
between pselect and select is the same as between ppoll and poll.
There was no need to think about signal handling race conditions in
netcat. I haven't read through the dhcpd or syslogd implementation very
closely yet, but I don't know what problem you're trying to solve here.
Post by Felix Janda
The main reason for not using ppoll is that it is linux only. pselect
is in POSIX. OpenBSD however doesn't seem to have it... The portable
way would be to use the self-pipe as before. But then there is even
more global state...
I don't understand what problem this addresses. I wrote a dnsd that
didn't need this (although it just handled udp, not tcp). I've written
my own little web servers, my own little vpn and a star topology
network redirector... Not remembering ever playing games with signals
like this for any of them?
Post by Felix Janda
Post by Rob Landley
Post by Felix Janda
+ for (;;) {
+ memcpy(&copy, &readfds, sizeof(copy));
+ ret = pselect(nfds, &copy, 0, 0, 0, &action.sa_mask);
+ if (ret == -1) {
+ if (errno != EINTR) error_exit ("pselect");
+ if (timeout && daemon_sig == SIGALRM) alarm(timeout);
+ sighandler(daemon_sig);
Unconditionally calling sighandler(), so we don't have the option
of it
Post by Rob Landley
being null and just getting the default behavior for signals. And
the
Post by Rob Landley
caller can't have its own use of alarm outside this helper.
Yes, for the broader use it would be good to make the signal handling
optional (but that's actually the main point of this function). The
caller
can have its own use of alarm if it doesn't use the one built into
this
function.
There's a timeout built into the blocking function. Possibly you have
to recalculate the timeout before calling the function again if you
care about next scheduled action instead of just "it took too long".
This is a heavyweight operation?
Post by Felix Janda
Post by Rob Landley
Ok, net win. (I need a lib/pending.h. Right now adding stuff to
lib/pending.c and then yanking it out again affects the commit
history
Post by Rob Landley
of permanent files...)
I guess using signal() instead of sigaction() would save some lines.
But I
didn't want to mix the new pselect with the old signal().
I still don't understand why you're doing all this stuff with signals.
Post by Felix Janda
Post by Rob Landley
But this patch is doing a lot more than just introducing the new
library functions and making stuff use them, so I've got to do
_more_
Post by Rob Landley
review before I can come to an actual conclusion about whether or
not
Post by Rob Landley
to apply it...
It is really just moving stuff around. Luckily the local variables in
the
main functions nicely broke up into the three seperate functions each.
Anyway, I still need to improve the restarting of syslogd.
Post by Rob Landley
I really do appreciate this work. Sorry to give you such a hard time
about it. I just want to get it _right_...
No problem. Please feel free to wait with coming back to this until
after
the (impending?) release.
I have an extra day in ohio (the linuxfest sunday content wasn't worth
staying for), so I'm trying to catch up on my email a bit and
downloading the LFS 7.4 files in the background...

Rob
Rob Landley
2013-12-01 19:05:54 UTC
Permalink
Taking the remaining holiday time to close old reply windows and such.
I've ahd this one open a while... :)
Post by Felix Janda
Post by Rob Landley
Post by Felix Janda
Both syslogd and dhcpd wait for sockets, signals or a timeout.
Currently,
they use select() with its timeout to listen to the sockets and a
self-pipe for the signals.
The new library function instead uses pselect() without a timeout
and
Post by Rob Landley
Post by Felix Janda
alarm(). With pselect() signals are only unblocked during this
system
Post by Rob Landley
Post by Felix Janda
call; so the xwrappers don't need to be aware about interrupted
syscalls.
Are we normally blocking a lot of signals in other commands? Using
the
Post by Rob Landley
6 argument version instead of the 5 argument version addresses an
issue
Post by Rob Landley
for which existing command?
If a signal occurs during the block it will still be delivered in the
next
pselect loop. pselect implements exactly the signal unblocking I am
talking
about above. We block all signals. But during the signal call some are
unblocked. When it returns they are again blocked and we can safely
check
what has happened. See the article
https://lwn.net/Articles/176911/
for some discussion on the system call.
My problem with the approach was that the loop really shouldn't mess
with signal handlers at all. If we want to fiddle with something fancy
there's signalfd.

I have an aversion to "infrastructure in search of a user". When common
code goes into the library, I like to be able to point to at least one
existing user, preferably two.
Post by Felix Janda
Post by Rob Landley
Post by Felix Janda
alarm() is used instead of the timeout argument to pselect() so
that
Post by Rob Landley
Post by Felix Janda
the length of the intervals between timeouts is not affected by
being
Post by Rob Landley
Post by Felix Janda
interrupted.
http://blog.alex.org.uk/2013/08/24/changes-to-qemus-timer-system/
I'm not convinced sending signals to ourselves is preferable to
telling
Post by Rob Landley
the system call when to timeout. Of the three options (select
adjusting
Post by Rob Landley
its timespec, us manually reading the clock and feeding pselect a
timeout based on when we next want to wake up, and us sending
ourselves
Post by Rob Landley
an alarm), us sending ourselves an alarm and constantly having to
reset
Post by Rob Landley
it seems like it has the most moving parts and the most things that
can
Post by Rob Landley
go wrong.
select() adjusts its timespecs on linux. But that's only one possible
behavior specified by POSIX.
Some not-linuxes don't work like linux, posix is vague enough to allow
Windows NT to be certified...

I'm pretty happy sticking with a documented Linux semantic if it's a
documented Linux semantic.
Post by Felix Janda
Since we are already doing signals SIGALRM
seemed more elegant then constantly adjusting the timeout to me.
Having a library I/O function modify global signal handler state is
elegant?
Post by Felix Janda
Post by Rob Landley
http://landley.net/notes-2011.html#05-09-2011
Post by Felix Janda
On SIGHUP syslogd restarts. Previously everything relevant was
contained
in syslogd_main() and the restarting was implemented by a goto.
Now a
Post by Rob Landley
Post by Felix Janda
longjmp() would be necessary. I instead used an xexec().
Since commit 955 (and pondering toysh to finally tackle that sucker
within a finite amount of time) I'm a bit worried that restarting a
command without OS involvement might misbehave if the signal mask
gets
Post by Rob Landley
corrupted, or there are funky environment variables, or memory leaks
build up, or we have leftover filehandles, or a couple dozen other
things.
We haven't got "generic reset this process's state" code, longjmp()
leaves a lot of debris, and xexec() callign a new main doesn't clear
the old stuff off the stack; do that in a tight enough loop for long
enough and eventually you will run out of stack even in a normal
linux
Post by Rob Landley
userspace. (Default's 8 megs: ulimit -s says 8192 and the bash man
page
Post by Rob Landley
says the units are kilobytes.)
I added NOFORK because some commands (like cd) _must_ happen in the
current toysh process context, but also because I could very very
carefully audit commands to make sure it was safe to call them in
the
Post by Rob Landley
same process context and continue when they return. But these
commands
Post by Rob Landley
are in pending because I haven't even done an initial audit pass on
them.
Discarding your process on restart and execing a new one covers a
multitude of sins. Possibly syslogd is clean enough it can restart
itself without accumulating trash, but as a long-lived daemon I have
yet to closely read (hence it being in the pending directory), this
does not fill me with confidence.
Probably just the "it's 5am, sun coming up soon" things. I should
step
Post by Rob Landley
away from the keyboard...
You are of course right. I think you have some good ideas below how to
fix it.
I note that "fork but don't exec" is remarkably cheap, and toybox
should probably do more of that. The downside is long chains of xexec()
that call command_main() can potentially eat a lot of stack and heap
and filehandles and such.

Some vague infrastructure I haven't had time to do would say "what are
the resources used by this process", and look at memory allocations not
yet freed, open filehandles, callstack, memory mappings, and so on. So
far the above is just a malloc/free wrapper, checking in /proc, and
backtrace(). Possibly some code under CONFIG_DEBUG that dumps leaked
filehandles and such. I could clean up the stack with a longjmp()...

There are also some bad habits I need to go through the code and clean
up. For example, if you call xexec() it'll try to free toys.optargs if
it's not pointing to the original argv[], and if we've incremented
optargs (which we do in a few places looping through without allocating
a temp variable), that'll break the heap. These are commands that don't
currently xexec(), but I have a commit pending to clean those out
anyway because I don't want to set a bad example that gets copied.

I could also just add an xexec() counter to struct toy_context and
force an actual exec /proc/self/exe if it hits 5 or something.
(Decrement it in the exit path, it's not sequential calls it's
_stacked_ calls.) But adding infrastructure is a poor substitute for
writing code to not do that in the first place...

Which brigns us back to my objection to the signal handling. :)
Post by Felix Janda
Post by Rob Landley
Post by Felix Janda
Since xexec()
doesn't change the pid the pidfile needs to be unlinked() before
terminating so that xpidfile() doesn't complain.
Um... I agree this is a symptom of a problem. I'm not sure that's
the
Post by Rob Landley
right fix. The restart is maintaining state and then getting
confused
Post by Rob Landley
by the state it's maintaining...
(I can just see some horrible systemd thing hitting a race window
where
Post by Rob Landley
the PID file wasn't there and trying to restart the process...)
Does systemd still need PID files?
I have no idea. It's a horrible thing and I'm trying to avoid it. My
point was not assuming behavior on the part of other programs, and
leaving a window where something doing inotify() wakes up because an
indicator file went away when the process it represents didn't and
isn't going to... I try to avoid that sort of halfway state where there
can be conflicting assumptions.

(Of course if the child does an actual exec() you still have the
recycled PID issue. This is why I prefer to "I have a real program and
it's doing X, this causes Y" rather than trying to solve theoretical
future problems.)
Post by Felix Janda
Post by Rob Landley
Post by Felix Janda
Furthermore if not run
in the foreground syslogd will daemonize() again when restarting.
Could
one detect in daemonize() whether daemonizing is necessary?
"The restart is maintaining state and then getting..."
Are these the _only_ instances of this? Is a main loop that exits
and
Post by Rob Landley
restarts itself but avoids initial setup better?
Is there a clean way to do this where we _don't_ have to be
absolutely
Post by Rob Landley
sure we've thought of everything ever? execv("/proc/self/exe",
toys.argc) and then fall back to something else if it returns,
perhaps?
Post by Rob Landley
Or can we get an exhaustive list of all process resources (from the
container suspend/resume guys, presumably) and check for ourselves
that
Post by Rob Landley
we're not leaking anything?
So how about making the two helper functions for daemon_main_loop()
return a value and possibly returning from daemon_main_loop() based on
that value? Then in syslogd_main() add a infinite loop around (or
almost)
everything.
I need to go clean up dhcpd and systemd before figuring out what the
infrastructure should look like. If we're going to do anything fancy
with signals I lean towards teaching this loop about signalfd, but
probably since Linux can adjust the timeout we should just let Linux
adjust the timeout.
Post by Felix Janda
Post by Rob Landley
http://lkml.indiana.edu/hypermail/linux/kernel/0206.2/0835.html
Post by Felix Janda
Felix
# HG changeset patch
# User Felix Janda <felix.janda at posteo.de>
# Date 1378041908 -7200
# Node ID 752d22ece0fe3fa9763fd0b6e231e653cc173890
# Parent 587b7572aac2a64988503458155e1f2546672525
Factor out daemon select() loops into a library function
Ok, the new select() lib function I'd probably be up for reviewing
on
Post by Rob Landley
its own, and the cleanups to a function I haven't even read yet I'd
just pass through and read/cleanup the new baseline when I got
around
Post by Rob Landley
to it.
This patch really just implements the helper function and makes the
two
toys use it. It's just a big function, causes the big main functions
to
be split up and makes the self-pipe stuff unnecessary.
$ hg status toys
M toys/lsb/umount.c
M toys/other/hello.c
M toys/other/pmap.c
M toys/other/pwdx.c
M toys/other/vconfig.c
M toys/other/vmstat.c
M toys/pending/dumpleases.c
M toys/pending/su.c
M toys/posix/du.c
M toys/posix/grep.c

Lemme finish my current set of cleanups, then get back to this. :)
Post by Felix Janda
Post by Rob Landley
But both at once? Not up for that right now. Might try again in the
morning...
I wouldn't consider syslogd to be of high priority.
On the other hand I'd like (or maybe would have liked some days ago)
to continue with cleaning it up. Now that the main function is not
such
a monster anymore one can maybe inline more stuff. OTOH I'm still not
sure about the behavior when something goes wrong when the toy starts.
If we come to an conclusion here, I'll send you a new version of the
patch, right?
I'm finally sort of resurfacing from A) catching up on domestic life
after 6 months out of state, B) starting a new job after the
kickstarter idea got zero replies.

But as you can see: backlog on all fronts. I haven't forgotten about
this, I'm just... behind. :)

Thanks for your patience,

Rob
Felix Janda
2013-12-02 21:20:25 UTC
Permalink
Post by Rob Landley
Taking the remaining holiday time to close old reply windows and such.
I've ahd this one open a while... :)
[...]
Post by Rob Landley
Post by Felix Janda
Post by Rob Landley
Are we normally blocking a lot of signals in other commands? Using
the
Post by Rob Landley
6 argument version instead of the 5 argument version addresses an
issue
Post by Rob Landley
for which existing command?
If a signal occurs during the block it will still be delivered in the next
pselect loop. pselect implements exactly the signal unblocking I am talking
about above. We block all signals. But during the signal call some are
unblocked. When it returns they are again blocked and we can safely check
what has happened. See the article
https://lwn.net/Articles/176911/
for some discussion on the system call.
My problem with the approach was that the loop really shouldn't mess
with signal handlers at all. If we want to fiddle with something fancy
there's signalfd.
Ok, I see that in the case that in the case that no signal handling is
needed it shouldn't touch the sigprocmask at all.

In the case signal handling is needed the blocking tames the
asynchronic signals. signalfd would be another (linux specific) option.
Post by Rob Landley
I have an aversion to "infrastructure in search of a user". When common
code goes into the library, I like to be able to point to at least one
existing user, preferably two.
For me the whole point was factoring out the similar code in dhcpd and
syslogd (now there are more possible users in pending), replacing the
self-pipes by something of similar functionality but better suited to
be put into a function.

[...]
Post by Rob Landley
Post by Felix Janda
select() adjusts its timespecs on linux. But that's only one possible
behavior specified by POSIX.
Some not-linuxes don't work like linux, posix is vague enough to allow
Windows NT to be certified...
I'm pretty happy sticking with a documented Linux semantic if it's a
documented Linux semantic.
Ok. (But it'd be good to note somewhere that a Linux semantic is used.)
Post by Rob Landley
Post by Felix Janda
Since we are already doing signals SIGALRM
seemed more elegant then constantly adjusting the timeout to me.
Having a library I/O function modify global signal handler state is
elegant?
"more elegant". (Oh, s/then/than/) daemon_main_loop() says how much I
thought of it as a library I/O function at the start. It is meant to
take total control of all signals. I can't think of any application
(in toybox) of both signals and a poll/select loop but a
wait-for-fd-or-signal-loop.

[...] (Discussion on fork and exec.)
Post by Rob Landley
Post by Felix Janda
Post by Rob Landley
Post by Felix Janda
Since xexec()
doesn't change the pid the pidfile needs to be unlinked() before
terminating so that xpidfile() doesn't complain.
Um... I agree this is a symptom of a problem. I'm not sure that's
the
Post by Rob Landley
right fix. The restart is maintaining state and then getting
confused
Post by Rob Landley
by the state it's maintaining...
(I can just see some horrible systemd thing hitting a race window
where
Post by Rob Landley
the PID file wasn't there and trying to restart the process...)
Does systemd still need PID files?
I have no idea. It's a horrible thing and I'm trying to avoid it. My
point was not assuming behavior on the part of other programs, and
leaving a window where something doing inotify() wakes up because an
indicator file went away when the process it represents didn't and
isn't going to... I try to avoid that sort of halfway state where there
can be conflicting assumptions.
So build in some extra logic so that the PID file is only deleted if the
daemon actually exits?

[...]
Post by Rob Landley
Post by Felix Janda
So how about making the two helper functions for daemon_main_loop()
return a value and possibly returning from daemon_main_loop() based on
that value? Then in syslogd_main() add a infinite loop around (or almost)
everything.
I need to go clean up dhcpd and systemd before figuring out what the
infrastructure should look like. If we're going to do anything fancy
with signals I lean towards teaching this loop about signalfd, but
probably since Linux can adjust the timeout we should just let Linux
adjust the timeout.
Apart from the portability I'd also be very much happy with signalfd.
Again pselect is a replacement for the __self-pipes__. The timeout
is just a side show.
Post by Rob Landley
Lemme finish my current set of cleanups, then get back to this. :)
Take your time.
Post by Rob Landley
I'm finally sort of resurfacing from A) catching up on domestic life
after 6 months out of state, B) starting a new job after the
kickstarter idea got zero replies.
I have no valuable input on the proposed text for the campaign
(like it as it is) but would really like something like it to happen
(and succeed). The current state is kind of sad. (There is no way
to blame you for it.)

Felix
Felix Janda
2013-12-02 21:20:25 UTC
Permalink
Post by Rob Landley
Taking the remaining holiday time to close old reply windows and such.
I've ahd this one open a while... :)
[...]
Post by Rob Landley
Post by Felix Janda
Post by Rob Landley
Are we normally blocking a lot of signals in other commands? Using
the
Post by Rob Landley
6 argument version instead of the 5 argument version addresses an
issue
Post by Rob Landley
for which existing command?
If a signal occurs during the block it will still be delivered in the next
pselect loop. pselect implements exactly the signal unblocking I am talking
about above. We block all signals. But during the signal call some are
unblocked. When it returns they are again blocked and we can safely check
what has happened. See the article
https://lwn.net/Articles/176911/
for some discussion on the system call.
My problem with the approach was that the loop really shouldn't mess
with signal handlers at all. If we want to fiddle with something fancy
there's signalfd.
Ok, I see that in the case that in the case that no signal handling is
needed it shouldn't touch the sigprocmask at all.

In the case signal handling is needed the blocking tames the
asynchronic signals. signalfd would be another (linux specific) option.
Post by Rob Landley
I have an aversion to "infrastructure in search of a user". When common
code goes into the library, I like to be able to point to at least one
existing user, preferably two.
For me the whole point was factoring out the similar code in dhcpd and
syslogd (now there are more possible users in pending), replacing the
self-pipes by something of similar functionality but better suited to
be put into a function.

[...]
Post by Rob Landley
Post by Felix Janda
select() adjusts its timespecs on linux. But that's only one possible
behavior specified by POSIX.
Some not-linuxes don't work like linux, posix is vague enough to allow
Windows NT to be certified...
I'm pretty happy sticking with a documented Linux semantic if it's a
documented Linux semantic.
Ok. (But it'd be good to note somewhere that a Linux semantic is used.)
Post by Rob Landley
Post by Felix Janda
Since we are already doing signals SIGALRM
seemed more elegant then constantly adjusting the timeout to me.
Having a library I/O function modify global signal handler state is
elegant?
"more elegant". (Oh, s/then/than/) daemon_main_loop() says how much I
thought of it as a library I/O function at the start. It is meant to
take total control of all signals. I can't think of any application
(in toybox) of both signals and a poll/select loop but a
wait-for-fd-or-signal-loop.

[...] (Discussion on fork and exec.)
Post by Rob Landley
Post by Felix Janda
Post by Rob Landley
Post by Felix Janda
Since xexec()
doesn't change the pid the pidfile needs to be unlinked() before
terminating so that xpidfile() doesn't complain.
Um... I agree this is a symptom of a problem. I'm not sure that's
the
Post by Rob Landley
right fix. The restart is maintaining state and then getting
confused
Post by Rob Landley
by the state it's maintaining...
(I can just see some horrible systemd thing hitting a race window
where
Post by Rob Landley
the PID file wasn't there and trying to restart the process...)
Does systemd still need PID files?
I have no idea. It's a horrible thing and I'm trying to avoid it. My
point was not assuming behavior on the part of other programs, and
leaving a window where something doing inotify() wakes up because an
indicator file went away when the process it represents didn't and
isn't going to... I try to avoid that sort of halfway state where there
can be conflicting assumptions.
So build in some extra logic so that the PID file is only deleted if the
daemon actually exits?

[...]
Post by Rob Landley
Post by Felix Janda
So how about making the two helper functions for daemon_main_loop()
return a value and possibly returning from daemon_main_loop() based on
that value? Then in syslogd_main() add a infinite loop around (or almost)
everything.
I need to go clean up dhcpd and systemd before figuring out what the
infrastructure should look like. If we're going to do anything fancy
with signals I lean towards teaching this loop about signalfd, but
probably since Linux can adjust the timeout we should just let Linux
adjust the timeout.
Apart from the portability I'd also be very much happy with signalfd.
Again pselect is a replacement for the __self-pipes__. The timeout
is just a side show.
Post by Rob Landley
Lemme finish my current set of cleanups, then get back to this. :)
Take your time.
Post by Rob Landley
I'm finally sort of resurfacing from A) catching up on domestic life
after 6 months out of state, B) starting a new job after the
kickstarter idea got zero replies.
I have no valuable input on the proposed text for the campaign
(like it as it is) but would really like something like it to happen
(and succeed). The current state is kind of sad. (There is no way
to blame you for it.)

Felix

Rob Landley
2013-12-01 19:05:54 UTC
Permalink
Taking the remaining holiday time to close old reply windows and such.
I've ahd this one open a while... :)
Post by Felix Janda
Post by Rob Landley
Post by Felix Janda
Both syslogd and dhcpd wait for sockets, signals or a timeout.
Currently,
they use select() with its timeout to listen to the sockets and a
self-pipe for the signals.
The new library function instead uses pselect() without a timeout
and
Post by Rob Landley
Post by Felix Janda
alarm(). With pselect() signals are only unblocked during this
system
Post by Rob Landley
Post by Felix Janda
call; so the xwrappers don't need to be aware about interrupted
syscalls.
Are we normally blocking a lot of signals in other commands? Using
the
Post by Rob Landley
6 argument version instead of the 5 argument version addresses an
issue
Post by Rob Landley
for which existing command?
If a signal occurs during the block it will still be delivered in the
next
pselect loop. pselect implements exactly the signal unblocking I am
talking
about above. We block all signals. But during the signal call some are
unblocked. When it returns they are again blocked and we can safely
check
what has happened. See the article
https://lwn.net/Articles/176911/
for some discussion on the system call.
My problem with the approach was that the loop really shouldn't mess
with signal handlers at all. If we want to fiddle with something fancy
there's signalfd.

I have an aversion to "infrastructure in search of a user". When common
code goes into the library, I like to be able to point to at least one
existing user, preferably two.
Post by Felix Janda
Post by Rob Landley
Post by Felix Janda
alarm() is used instead of the timeout argument to pselect() so
that
Post by Rob Landley
Post by Felix Janda
the length of the intervals between timeouts is not affected by
being
Post by Rob Landley
Post by Felix Janda
interrupted.
http://blog.alex.org.uk/2013/08/24/changes-to-qemus-timer-system/
I'm not convinced sending signals to ourselves is preferable to
telling
Post by Rob Landley
the system call when to timeout. Of the three options (select
adjusting
Post by Rob Landley
its timespec, us manually reading the clock and feeding pselect a
timeout based on when we next want to wake up, and us sending
ourselves
Post by Rob Landley
an alarm), us sending ourselves an alarm and constantly having to
reset
Post by Rob Landley
it seems like it has the most moving parts and the most things that
can
Post by Rob Landley
go wrong.
select() adjusts its timespecs on linux. But that's only one possible
behavior specified by POSIX.
Some not-linuxes don't work like linux, posix is vague enough to allow
Windows NT to be certified...

I'm pretty happy sticking with a documented Linux semantic if it's a
documented Linux semantic.
Post by Felix Janda
Since we are already doing signals SIGALRM
seemed more elegant then constantly adjusting the timeout to me.
Having a library I/O function modify global signal handler state is
elegant?
Post by Felix Janda
Post by Rob Landley
http://landley.net/notes-2011.html#05-09-2011
Post by Felix Janda
On SIGHUP syslogd restarts. Previously everything relevant was
contained
in syslogd_main() and the restarting was implemented by a goto.
Now a
Post by Rob Landley
Post by Felix Janda
longjmp() would be necessary. I instead used an xexec().
Since commit 955 (and pondering toysh to finally tackle that sucker
within a finite amount of time) I'm a bit worried that restarting a
command without OS involvement might misbehave if the signal mask
gets
Post by Rob Landley
corrupted, or there are funky environment variables, or memory leaks
build up, or we have leftover filehandles, or a couple dozen other
things.
We haven't got "generic reset this process's state" code, longjmp()
leaves a lot of debris, and xexec() callign a new main doesn't clear
the old stuff off the stack; do that in a tight enough loop for long
enough and eventually you will run out of stack even in a normal
linux
Post by Rob Landley
userspace. (Default's 8 megs: ulimit -s says 8192 and the bash man
page
Post by Rob Landley
says the units are kilobytes.)
I added NOFORK because some commands (like cd) _must_ happen in the
current toysh process context, but also because I could very very
carefully audit commands to make sure it was safe to call them in
the
Post by Rob Landley
same process context and continue when they return. But these
commands
Post by Rob Landley
are in pending because I haven't even done an initial audit pass on
them.
Discarding your process on restart and execing a new one covers a
multitude of sins. Possibly syslogd is clean enough it can restart
itself without accumulating trash, but as a long-lived daemon I have
yet to closely read (hence it being in the pending directory), this
does not fill me with confidence.
Probably just the "it's 5am, sun coming up soon" things. I should
step
Post by Rob Landley
away from the keyboard...
You are of course right. I think you have some good ideas below how to
fix it.
I note that "fork but don't exec" is remarkably cheap, and toybox
should probably do more of that. The downside is long chains of xexec()
that call command_main() can potentially eat a lot of stack and heap
and filehandles and such.

Some vague infrastructure I haven't had time to do would say "what are
the resources used by this process", and look at memory allocations not
yet freed, open filehandles, callstack, memory mappings, and so on. So
far the above is just a malloc/free wrapper, checking in /proc, and
backtrace(). Possibly some code under CONFIG_DEBUG that dumps leaked
filehandles and such. I could clean up the stack with a longjmp()...

There are also some bad habits I need to go through the code and clean
up. For example, if you call xexec() it'll try to free toys.optargs if
it's not pointing to the original argv[], and if we've incremented
optargs (which we do in a few places looping through without allocating
a temp variable), that'll break the heap. These are commands that don't
currently xexec(), but I have a commit pending to clean those out
anyway because I don't want to set a bad example that gets copied.

I could also just add an xexec() counter to struct toy_context and
force an actual exec /proc/self/exe if it hits 5 or something.
(Decrement it in the exit path, it's not sequential calls it's
_stacked_ calls.) But adding infrastructure is a poor substitute for
writing code to not do that in the first place...

Which brigns us back to my objection to the signal handling. :)
Post by Felix Janda
Post by Rob Landley
Post by Felix Janda
Since xexec()
doesn't change the pid the pidfile needs to be unlinked() before
terminating so that xpidfile() doesn't complain.
Um... I agree this is a symptom of a problem. I'm not sure that's
the
Post by Rob Landley
right fix. The restart is maintaining state and then getting
confused
Post by Rob Landley
by the state it's maintaining...
(I can just see some horrible systemd thing hitting a race window
where
Post by Rob Landley
the PID file wasn't there and trying to restart the process...)
Does systemd still need PID files?
I have no idea. It's a horrible thing and I'm trying to avoid it. My
point was not assuming behavior on the part of other programs, and
leaving a window where something doing inotify() wakes up because an
indicator file went away when the process it represents didn't and
isn't going to... I try to avoid that sort of halfway state where there
can be conflicting assumptions.

(Of course if the child does an actual exec() you still have the
recycled PID issue. This is why I prefer to "I have a real program and
it's doing X, this causes Y" rather than trying to solve theoretical
future problems.)
Post by Felix Janda
Post by Rob Landley
Post by Felix Janda
Furthermore if not run
in the foreground syslogd will daemonize() again when restarting.
Could
one detect in daemonize() whether daemonizing is necessary?
"The restart is maintaining state and then getting..."
Are these the _only_ instances of this? Is a main loop that exits
and
Post by Rob Landley
restarts itself but avoids initial setup better?
Is there a clean way to do this where we _don't_ have to be
absolutely
Post by Rob Landley
sure we've thought of everything ever? execv("/proc/self/exe",
toys.argc) and then fall back to something else if it returns,
perhaps?
Post by Rob Landley
Or can we get an exhaustive list of all process resources (from the
container suspend/resume guys, presumably) and check for ourselves
that
Post by Rob Landley
we're not leaking anything?
So how about making the two helper functions for daemon_main_loop()
return a value and possibly returning from daemon_main_loop() based on
that value? Then in syslogd_main() add a infinite loop around (or
almost)
everything.
I need to go clean up dhcpd and systemd before figuring out what the
infrastructure should look like. If we're going to do anything fancy
with signals I lean towards teaching this loop about signalfd, but
probably since Linux can adjust the timeout we should just let Linux
adjust the timeout.
Post by Felix Janda
Post by Rob Landley
http://lkml.indiana.edu/hypermail/linux/kernel/0206.2/0835.html
Post by Felix Janda
Felix
# HG changeset patch
# User Felix Janda <felix.janda at posteo.de>
# Date 1378041908 -7200
# Node ID 752d22ece0fe3fa9763fd0b6e231e653cc173890
# Parent 587b7572aac2a64988503458155e1f2546672525
Factor out daemon select() loops into a library function
Ok, the new select() lib function I'd probably be up for reviewing
on
Post by Rob Landley
its own, and the cleanups to a function I haven't even read yet I'd
just pass through and read/cleanup the new baseline when I got
around
Post by Rob Landley
to it.
This patch really just implements the helper function and makes the
two
toys use it. It's just a big function, causes the big main functions
to
be split up and makes the self-pipe stuff unnecessary.
$ hg status toys
M toys/lsb/umount.c
M toys/other/hello.c
M toys/other/pmap.c
M toys/other/pwdx.c
M toys/other/vconfig.c
M toys/other/vmstat.c
M toys/pending/dumpleases.c
M toys/pending/su.c
M toys/posix/du.c
M toys/posix/grep.c

Lemme finish my current set of cleanups, then get back to this. :)
Post by Felix Janda
Post by Rob Landley
But both at once? Not up for that right now. Might try again in the
morning...
I wouldn't consider syslogd to be of high priority.
On the other hand I'd like (or maybe would have liked some days ago)
to continue with cleaning it up. Now that the main function is not
such
a monster anymore one can maybe inline more stuff. OTOH I'm still not
sure about the behavior when something goes wrong when the toy starts.
If we come to an conclusion here, I'll send you a new version of the
patch, right?
I'm finally sort of resurfacing from A) catching up on domestic life
after 6 months out of state, B) starting a new job after the
kickstarter idea got zero replies.

But as you can see: backlog on all fronts. I haven't forgotten about
this, I'm just... behind. :)

Thanks for your patience,

Rob
Loading...