[Dovecot] Patch: event port-based ioloop and notify
puccia+ml-dovecot at gmail.com
Thu Dec 10 01:28:58 EET 2009
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" .
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 ) 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.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.
More information about the dovecot