Timo, thanks for the detailed review! I'm going to work on the patch and make it follow your comments.
> Does the configure check actually have to test that all the port_* > functions work? If they exist in libc, aren't they guaranteed to work? > If just their existence is enough, I'd just change the check to test to > AC_CHECK_FUNC(port_associate, ..). I don't know of any configurations where they exist and don't work. I'll cut the tests, then. > Why do you call it fen / file event notification? It seems to be called > just "port" in the man pages and everywhere else, so I'd call it that. Yes, I mixed that up. The general port_* framework is the "event ports framework", while the part that specifically watches for changes in files and directories is the newer "file event notification API" [1]. The presence of port_* is enough for the ioloop to work, but the FEN API, which introduces PORT_SOURCE_FILE in the headers, is needed for the ioloop-notify. The file event notification is only available since version 72 of Nevada, so all recent versions (since before 2008) of OpenSolaris have it, but I don't know which, if any, versions of Solaris 10 have it. I would be going to separate the two checks and take a less tortuous path to check for PORT_SOURCE_FILE (AC_CHECK_DECL, I'd say). > Why do you silently handle EBADFs and ENOENTs? Are those actually > happening in your system? Dovecot should never close fds before removing > them from ioloop, other ioloops also log an error if that happens. I'll check that again and report back. I'm not sure if I saw an ENOENT. Does this apply to ioloop-notify as well? > For port_getn() error handling I'd probably do the same as all other > ioloops: Ignore EINTR/ETIME and treat everything else as fatal. What's > the idea behind BAD_PORTEV_USER? Unfortunately, it's possible (at least in some versions, see [2]) that port_getn() returns EINTR before updating nget to anything useful. In this case, the code would see it set to 1 and try to process the event: it would probably crash immediately, as soon as it tries to dereference events[0].portev_user to update the refcount. It seems to be a rare race condition and I haven't witnessed it personally, but (claimed to be) entirely possible. > In general when an error that really shouldn't happen does happen I > prefer to fatal/panic than trying to limp along. Then I'll let EBADFD be fatal too, instead of trying to recreate the port and proceed, and I'll clean up the two switch statements. >> + io->path = i_new(char, strlen(path)+1); >> + strcpy(io->path, path); > > io->path = i_strdup(path); Thanks. It kind of shows that I haven't taken the time to read the library – sorry about that. I'll re-post as soon as I manage to do the clean-up. [1] http://arc.opensolaris.org/caselog/PSARC/2007/027/mail [2] http://www.mail-archive.com/[email protected]/msg12330.html -- Emanuele
