Hello Timo,
Thanks for reviewing the managesieve patch.
Timo Sirainen schreef:
On Mon, 2007-03-26 at 18:34 +0200, Stephan Bosch wrote:
http://sinas.rename-it.nl/~sirius/dovecot-1.0.rc28-MANAGESIEVE-v3.diff.gz
I finally took a look at this. The biggest problem is that there's way too much duplicated code. I agree. This would make updating the various protocol implementations tedious and error prone (to say the least). My initial goal was to write a working non-intrusive managesieve patch for dovecot, to be fully merged with dovecot later.
- lib-managesieve: Is the only thing different from lib-imap that it does UTF8 validation? Then rather add some utf8-flag to the imap-parser. Or maybe add some string validation callback which does it in lib-managesieve. It has been 6 months since I adapted this code for managesieve. I'll do a quick review and check whether these two libraries can be merged easily, but I believe they can.
managesieve: I guess the commands-common.c could be put somewhere.. Hmm. Also client.c has a lot of copy&pasted code. I'm not sure what's the right place for these though.
login: There is still too much duplication in the login code (also for imap/pop3). I'm thinking about cleaning this up after v1.0. Wonder if there should exist just one login binary which can handle multiple protocols (and support plugins).. That's a nice idea.
- Sieve plugin would need to be merged somehow to this to avoid adding lib-sieve/cmu.
I coined this in my previous e-mail: if the master process could provide some means of dynamically registering new process types (protocols), e.g. through plugins, it should not be difficult to incorporate this managesieve implementation in dovecot-sieve entirely. Also, this plugin solution could be implemented/complemented by your single-login-process idea.
Within the dovecot-sieve plugin package, a common sieve library would then solve the lib-sieve/cmu duplication problem.
There's also one bug:
/* FIXME: Is this buffer on the stack method allowable for
- dovecot at all or do we need to define some t_buffer? */ static const int bufsize = 255; char linkbuf[bufsize];
ret = readlink(storage->active_path, linkbuf, bufsize-1); linkbuf[ret] = '\0'; //// ret can be -1 .. return t_strdup(linkbuf);
Change rather to something like:
char linkbuf[PATH_MAX]; ret = readlink(storage->active_path, linkbuf, sizeof(linkbuf)); .. return t_strndup(linkbuf, ret);
Nice one, also with respect to the buffer size. Apart from this real bug, there are currently still many more FIXMEs out there.
I'll fix the issues that I can fix right now. Most duplicate code problems will have to wait until you have the time to juggle around a bit with the dovecot(-sieve) code. Please notify my when you have changed anything with this respect.
Regards,
-- Stephan Bosch stephan@rename-it.nl IRC: Freenode, #dovecot, S[r]us