[Toybox] [PATCH] Factor out dhcpd's and syslogd's main loops into a library function

[Felix Janda]

Rob Landley wrote:
> On 09/10/2013 04:22:41 PM, Felix Janda wrote:
> > Rob Landley wrote:
> > > 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.

> > 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.

> > 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().

> > > > +  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.
> 
> 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.)

> > > 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().
> 
> I still don't understand why you're doing all this stuff with signals.
> 
> > > 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.
> 
> 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

 1379440766.0