ctld: Use kevent(2) for socket events rather than select(2)

[jhb]

usr.sbin/ctld/ctld.c

2429–2430

I considered replacing the signal handlers with EVFILT_SIGNAL. I might still do a later commit to do that for most cases, but the SIGALARM / setitimer case is used to interrupt a connect() call if it takes too long and EVFILT_TIMER wouldn't raise an interruption, so that one needs to stay as a signal at least. SIGHUP, SIGTERM, etc. would probably be fine though to be ignored but raise EVFILT_SIGNAL events that popped out here.

[asomers]

usr.sbin/ctld/ctld.c

2429–2430

BTW, there's a problem with the way that the current logic handles SIGCHLD: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=284282 . I'll fix that one myself if I find the time.

2432–2433

Why are you changing the "return" to "continue" here?

[jhb]

usr.sbin/ctld/ctld.c

2429–2430

Huh, yeah, I noticed that the code looked really weird to me with how it handled pidfile. I guess switching to EVFILT_SIGNAL and handling the signal events in main_loop would "fix" that bug since main_loop wouldn't get reentered?

2432–2433

If the signal is one of the caught signals it will notice at the top of the loop anyway? I think it really doesn't matter. Eventually if this used EVFILT_SIGNAL the outer loop logic would move into here anyway but it could switch to continue at that time? Either way gives the same result. (I did start on the EVFILT_SIGNAL thing before backing off, so maybe that was in my mind?)

[jhb]

usr.sbin/ctld/ctld.c

2405

I forgot to mention, this call is the main reason we can't easily use EVFILT_SIGNAL for signals other than SIGALRM. We would need a kevent we could register that would return when there is a pending connection to accept via the in-kernel proxy. There is some precedent for having the kernel create new events that aren't registered by userland directly, e.g. EVFILT_AIO is generated by the aio routines without having been added by a call to kevent(2) from userland. We could also just use a d_kqfilter method for /dev/ctl though as I think we'd have to have a new EVFILT_CTL regardless and perhaps use fflags (NOTE_*) to choose which CTL events we care about. For now we would just define a single flag for this event.

[mav]

usr.sbin/ctld/ctld.c

2758–2761

Won't moving it after daemonisation hide possible error output?

[asomers]

usr.sbin/ctld/ctld.c

2758–2761

Yes it will. But I don't see a good alternative, given that a kq is not inherited after fork.

[mav]

usr.sbin/ctld/ctld.c

2758–2761

About that I can't comment, never used it, so I'll resign here.

[jhb]

usr.sbin/ctld/ctld.c

2758–2761

Yes, this was due to the requirement of kqueue. The errors do still get logged to syslog, so they aren't entirely invisible.

[asomers]

I think this is ok. Losing the ability to apply configuration once before daemonizing is unfortunate but understandable. If it's a problem, we can fix it later by using rfork. I also see that you're only passing a single-element eventlist to kevent, which isn't the most efficient. But ctld isn't supposed to get a high rate of connections, so that's fine.

usr.sbin/ctld/ctld.c

2429–2430

Yeah, that would be one way to fix it.

2758–2761

We can fix this by replacing daemon with a deamon-like function that does rfork(RFPROC | RFNOWAIT) instead of fork. I just tested it, and it works.

[jhb]

usr.sbin/ctld/ctld.c

2429–2430

I might try to do what you suggest though and just move it out into main() anyway.

2758–2761

If it is really pressing I could also defer kqueue registration the first time until after fork(). However, keep in mind that the conf files are still parsed before forking, so things like syntax errors are still reported on stderr. The types of errors that would no longer be on stderr and only in /var/log/messages are things like failing to bind to a socket or failure to open backing store for a LUN. That said, how many sysadmins are watching ctld for errors after starting it vs checking logs? Certainly in a production environment where it starts on boot machines are mostly likely booting unattended and errors are only noticed via logs (or a nagios check watching logs, etc.). Developers also have the option of starting ctld without forking.

BTW, it does seem a bit odd to me that ctld uses fork() for each connection rather than threads. The best explanation I can come up with though is that it avoids having to deal with locking, etc. for configuration changes so that child connections get a consistent snapshot of the config while handling a child connection (in particular for discovery services).