Re: [Dovecot] New generic userdb lookup api
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
Re: [Dovecot] New generic userdb lookup api
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. Well, default pool isn't really a proper memory pool. p_malloc(default_pool) = p_malloc(system_pool) = i_malloc() = calloc(). It will only get freed when the process is killed. 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. ;-) I was thinking something like: pool = pool_alloconly_create(userdb lookup, 512); auth_master_lookup(auth, pool, reply); // do stuff with reply pool_unref(pool); Or I suppose auth_master_init() could allocate the pool internally and call p_clear() at the beginning of each lookup. Hmm. 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? I don't think there's any benefit in doing that. PGP.sig Description: This is a digitally signed message part
Re: [Dovecot] New generic userdb lookup api
Timo Sirainen [EMAIL PROTECTED] writes: 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. Well, default pool isn't really a proper memory pool. p_malloc(default_pool) = p_malloc(system_pool) = i_malloc() = calloc(). It will only get freed when the process is killed. Hmm, that would be after the end of the imap connection (deliver should be no problem anyway) -- yes this could indeed take some time... 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. ;-) I was thinking something like: pool = pool_alloconly_create(userdb lookup, 512); auth_master_lookup(auth, pool, reply); // do stuff with reply pool_unref(pool); Or I suppose auth_master_init() could allocate the pool internally and call p_clear() at the beginning of each lookup. Hmm. I think I prefer the first version, as in the second proposal replies From older lookups would become destroyed with every new lookup, which IMO would not be really evident to the user of the API. 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? I don't think there's any benefit in doing that. Ok, thanks for your input. :) I'll put together an a little improved version this afternoon. 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 pgpyFcOOmeNVC.pgp Description: PGP signature
Re: [Dovecot] New generic userdb lookup api
Ok, as discussed I have made some changes (hopefully improvements) in the new auth-master API for userdb requests... Sascha Wilde [EMAIL PROTECTED] writes: Timo Sirainen [EMAIL PROTECTED] writes: [...] 3. 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. Done, the last getenv()s have been moved to the deliver code. 4. 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 Done. I thought for a moment of putting the pool (de)allocation into auth_master_init and auth_master_deinit -- but that turned out to be to quirky, especially with the existing deliver code... (also p_array_init(reply-extra_fields) would be cleaner to do inside the lookup code than require it to be done externally). Done, too. 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 pgpvwa2nMJGw7.pgp Description: PGP signature
Re: [Dovecot] New generic userdb lookup api
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... 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... 3. 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. 4. 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 pgpIkkYnThP9L.pgp Description: PGP signature
Re: [Dovecot] New generic userdb lookup api
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. 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. 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. 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. (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. :) signature.asc Description: This is a digitally signed message part
Re: [Dovecot] New generic userdb lookup api (was: New userdb backend for checkpassword like programs)
Timo Sirainen [EMAIL PROTECTED] writes: Hmm. auth-client.c is about performing authentication as a client. What you're doing is about doing a userdb lookup and connecting to dovecot-auth as a master. So different file, but I'm not really sure about the name. Perhaps auth-master.c and auth_master_init/deinit() auth_master_user_lookup() function? 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... ;-) Now I have to go back and finally implement the %%h feature for shared name spaces. 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 pgp2KZnfTlYwn.pgp Description: PGP signature
Re: [Dovecot] New generic userdb lookup api
On Fri, 2008-10-24 at 16:19 +0200, Sascha Wilde wrote: Timo Sirainen [EMAIL PROTECTED] writes: Hmm. auth-client.c is about performing authentication as a client. What you're doing is about doing a userdb lookup and connecting to dovecot-auth as a master. So different file, but I'm not really sure about the name. Perhaps auth-master.c and auth_master_init/deinit() auth_master_user_lookup() function? 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? 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.) 3. 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(). 4. You're leaking memory. Cleanest fix would be to add pool_t pool parameter to auth_master_user_lookup() and allocate memory only from it (also p_array_init(reply-extra_fields) would be cleaner to do inside the lookup code than require it to be done externally). signature.asc Description: This is a digitally signed message part
