Timo Sirainen <[EMAIL PROTECTED]> writes: > On Sun, 2008-10-26 at 18:33 +0100, Sascha Wilde wrote: >> Timo Sirainen <[EMAIL PROTECTED]> writes: >> > On Fri, 2008-10-24 at 16:19 +0200, Sascha Wilde wrote: >> >> Ok, I used auth-master.* -- the new code is in changeset f5ce17153a3d in >> >> my kolab-branch at >> >> http://hg.intevation.org/kolab/dovecot-1.2_kolab-branch/ and I made >> >> deliver use it in 94b00e377a25. >> >> >> >> I had no time for thorough testing, but in my test-setup it seems to >> >> work like before, so at least I didn't break it completely... ;-) >> > >> > A couple of things: >> > >> > 1. It disconnects after each lookup. Not good if multiple lookups are >> > done. Then again it probably shouldn't keep the connection alive forever >> > since the imap connections can run for ages and most of the necessary >> > lookups are probably done close to each others. Maybe timeout after 1 >> > minute of idling? >> >> I agree that this is something that should be optimized, but I was under >> the impression, that the current behavior of deliver was just like that >> -- maybe I'm mistaken, I haven't double-checked that... > > deliver does always only one lookup, so it doesn't matter. But for IMAP > if you have shared mailboxes from multiple users it'll do multiple > lookups.
Ack. >> > 2. conn->to is for auth request timeout. It should be removed after >> > io_loop_run() so if 1. is fixed it won't leak timeouts. >> > (The same conn->to could actually be used for the two timeouts - one >> > value when looking up, another value when idling.) >> >> Ack. Unfortunately I'll have to put a working prototype of the >> "%%h"-feature together before I'll have time to look into that... > > Well, I could probably get these missing things done too. This would be really great and highly appreciated! I just didn't dare to ask... :-) >> > Cleanest fix would be to add pool_t pool parameter to >> > auth_master_user_lookup() and allocate memory only from it >> >> I think a free_auth_user_reply function might be preferable. >> >> But I have to admit, that I didn't look deeply enough into the memory >> pool management in dovecot to really know whats The Right Thing To >> Do[tm]. > > The idea behind Dovecot's memory allocations is that you shouldn't have > to go through all the trouble of doing lots of memory frees. Because 1) > it's easy to cause memory leaks then, 2) it requires more code and makes > it uglier, 3) possibly increases memory fragmentation. > > So with memory pools you just allocate all the memory from the pool and > finally simply free the pool. That takes care of all these 1-3) issues. > It could use slightly more memory, but especially for these kind of > short living allocations it really doesn't matter. Than I don't really see the problem with the current code -- I understand that all the memory it uses (with i_strdup and friends) is allocated from the default pool, which I assume will be freed eventually. If the goal of an dedicated pool is to free the memory early the code using the auth-master API will have to allocate and free this pool, I don't see the advantage here... But then, on a second thought I _do_ see the advantage of a consistent way to do things like this. ;-) >> Btw, on dedicated vs. default resources, I wasn't quite sure if it was a >> good idea to use the default ioloop. Any thoughts on that? > > For deliver it doesn't matter, but for imap you really should create a > new ioloop or things will probably break. Yes, I know (already made this mistake)... ;-) The question is, should the ioloop be an extra argument to auth_master_init? >> > (also p_array_init(&reply->extra_fields) would be cleaner to do inside >> > the lookup code than require it to be done externally). >> >> Hmm, the idea was to only fill the extra_fields array when it was >> initialized, but maybe it isn't worth the trouble... > > See above - it's only a short living lookup and this makes code slightly > cleaner since the allocation is done only in one place. :) Ok, I'll make this change. cheers sascha -- Sascha Wilde OpenPGP key: 4BB86568 http://www.intevation.de/~wilde/ http://www.intevation.de/ Intevation GmbH, Neuer Graben 17, 49074 Osnabrück; AG Osnabrück, HR B 18998 Geschäftsführer: Frank Koormann, Bernhard Reiter, Dr. Jan-Oliver Wagner
pgpBlrsyToB5G.pgp
Description: PGP signature