Timo Sirainen tss@iki.fi 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:
- 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...
- 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...
- Would be nice to get rid of the getenv()s :) The MAIL_CHROOT handling could be moved to deliver (use it if reply->chroot == NULL). The debug could be a parameter to auth_master_init().
You are right, and as I moved/left most of the env stuff in deliver/auth-client anyway it is only consequent to handle those two the same -- I'll make this change.
- You're leaking memory.
Um, yes. *blush* -- at least I added the free for the connection shortly after my announcement... ;-)
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].
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?
(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...
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