[Dovecot] New generic userdb lookup api

[Sascha Wilde]

Timo Sirainen <tss at iki.fi> writes:
> On Sun, 2008-10-26 at 18:33 +0100, Sascha Wilde wrote:
>> Timo Sirainen <tss at 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:
>> >
>> > 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 188 bytes
Desc: not available
Url : http://dovecot.org/pipermail/dovecot/attachments/20081027/ef94aa03/attachment.bin