[Dovecot] Patch: event port-based ioloop and notify

[Emanuele Pucciarelli]

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/networking-discuss@opensolaris.org/msg12330.html

-- 
Emanuele