On Thu, 2011-01-06 at 10:35 +0100, Jan Zelený wrote:
> Simo Sorce <sso...@redhat.com> wrote:
> > This patch makes it possible to run ipa-dns-install and use the admin
> > kerberos credentials.
> > 
> > Fixes #686.
> > 
> > Simo.
> 
> Nack, I have some comments:
> 
> Exception handling (chunk #4):
> Those prints should go away. But the main thing: that particular part of code 
> doesn't seem to produce any exceptions, which should be handled

Ok I will remove that part, it was half debugging code and half to
handle code that has been later changed.

> Function ldap_disconnect isn't used anywhere. That makes me wonder - is it 
> redundant or should it be somewhere in the code. I guess this is a policy 
> issue - either we want the connection to stay as long as possible or we want 
> to use it only for a certain set of commands and then disconnect it.

I initially used it to do connect,op,disconnect, but later decided it
was better to let connection live as long as the instance was around.

In a future patch we may even move admin_conn to be a global handler so
that multiple instances will use just one connection instead of having
one pending per-instance type, but I didn't want to go that far.

However I didn't remove ldap_disconnect because it will be useful if
later on someone needs to change the code to have a temporary
connection. I think I may want to use it in the next patch I am working
on. I can remove it though and re-add it later if needed, I am ok either
way.

Simo.

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to