head/libexec/ftpd/blacklist.c
50–51 ↗	(On Diff #17259)	Isn't this redundant? The only `blacklist_notify()` calls are clearly in the client session, which always initializes blacklist (right before the `yyparse()`).

head/libexec/ftpd/blacklist.c
50–51 ↗	(On Diff #17259)	Not exactly. If the blacklist_init() fails for some reason, blstate will be NULL. It will try again to blacklist_init(), and if it fails a second time, blstate will still be NULL. In which case, it just gives up and continues regular processing, forgoing any notification to blacklistd. Given how ftpd in FreeBSD is structured, it's not necessary to explicitly call blacklist_init() just before the yyparse(). I put that in so there's a definitive point where it gets calls, no matter the code paths that are taken afterwards... It just means in the common case, where blacklist_init() succeeds the first time, we do a couple of other pointer compares to NULL, which is pretty cheap.

cem added inline comments.Jun 3 2016, 5:13 PM

head/libexec/ftpd/blacklist.c
50–51 ↗	(On Diff #17259)	If the blacklist_init() fails for some reason, blstate will be NULL. It will try again to blacklist_init(), and if it fails a second time, blstate will still be NULL. In which case, it just gives up and continues regular processing, forgoing any notification to blacklistd. Right—is there any reason to expect that `blacklist_init` fails the first time and doesn't fail again on the second try? Given how ftpd in FreeBSD is structured, it's not necessary to explicitly call blacklist_init() just before the yyparse(). I put that in so there's a definitive point where it gets calls, no matter the code paths that are taken afterwards... Only because every notify call is checking for blstate, right? I don't understand why this daemon does it differently from all of the others. Either initialize it once where needed (like everything else does?), or drop the explicit initialization, no? It just means in the common case, where blacklist_init() succeeds the first time, we do a couple of other pointer compares to NULL, which is pretty cheap. I am not concerned about the performance cost, just the style / code smell. Thanks for doing this work!

head/libexec/ftpd/blacklist.c
50–51 ↗	(On Diff #17259)	I don't think it's particularly more likely to succeed calling the same initialization routine a second time. The sshd changes (D5915) are similar to this one. (Feel free to comment on that review if you see anything.) If you have a daemon that is going to chroot() someplace, it obviously needs to call the library routine blacklist_open() before it does the chroot. (Or you need to arrange to have a socket endpoint in the chrooted area of the filesystem. Which blacklistd can do, but requires additional configuration.) Anyway - the FreeBSD ftpd does do a chroot, but only after it has already verified that username/password (anonymous is handled a little differently). The sendmail changes (which I need to move to a port) are also similar to this one. Most of the other daemons that I've converted are just spawned out of inetd, so there's no persistent daemon hanging around, so the blstate is just initialized just before use, and there will never be an attempt to reuse the blstate to communicate with the blacklistd process. I will change this to have the explicit initialization, and take out the secondary attempt in blacklist_notify. I will also change it to use the symbolic name of STDIN_FILENO, rather than just '0' as it does now - someone else commented on that in some email.