[Dovecot] MANAGESIEVE patch v3 for dovecot 1.0.rc28

[Stephan Bosch]

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 at rename-it.nl
IRC: Freenode, #dovecot, S[r]us