[Toybox] [PATCH] Factor out dhcpd's and syslogd's main loops into a library function
Rob Landley
rob at landley.net
Fri Sep 6 09:46:26 PDT 2013
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()".
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?)
> +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...)
> +/*
> + * 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?
> + 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.
> + } 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.
> // 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...)
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
1378485986.0
More information about the Toybox
mailing list