[Toybox] [PATCH] Factor out dhcpd's and syslogd's main loops into a library function
Felix Janda
felix.janda at posteo.de
Tue Sep 10 14:22:41 PDT 2013
Rob Landley wrote:
> On 09/01/2013 08:54:27 AM, Felix Janda wrote:
> > 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.
> 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...)
>
> > 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.
> > +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.
> > +/*
> > + * 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?
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...
> > + for (;;) {
> > + memcpy(©, &readfds, sizeof(copy));
> > + ret = pselect(nfds, ©, 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.
> > + } else selecthandler(©);
> > + }
> > +}
> > 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.
> > // 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...
>
> > // 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;
> > + }
> > +}
> > +
>
> 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...)
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().
> 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.
> 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
1378848161.0
More information about the Toybox
mailing list