On Wed, Nov 25, 2015 at 09:28:27AM +0100, Martin Babinsky wrote: > On 11/25/2015 07:21 AM, Jan Cholasta wrote: > >On 25.11.2015 05:56, Fraser Tweedale wrote: > >>On Tue, Nov 24, 2015 at 05:38:45PM +0100, Jan Cholasta wrote: > >>>On 24.11.2015 17:17, Martin Babinsky wrote: > >>>>On 11/24/2015 05:10 PM, Martin Babinsky wrote: > >>>>>On 11/24/2015 05:01 PM, Martin Babinsky wrote: > >>>>>>On 11/24/2015 04:58 PM, Jan Cholasta wrote: > >>>>>>>On 24.11.2015 16:48, Martin Babinsky wrote: > >>>>>>>>On 11/24/2015 04:44 PM, Martin Babinsky wrote: > >>>>>>>>>https://fedorahosted.org/freeipa/ticket/5459 > >>>>>>>>> > >>>>>>>>forgot to attach the actual file *slaps himself* > >>>>>>> > >>>>>>>ipaserver/install/cainstance.py:1849: [E1101(no-member), > >>>>>>>ensure_default_caacl] Instance of 'API' has no 'Backed' member) > >>>>>>> > >>>>>> > >>>>>>Fixed > >>>>>> > >>>>>> > >>>>>> > >>>>>Also attaching rebased patch for ipa-4-2 > >>>>> > >>>>> > >>>>> > >>>>Ignore the rebased patch, the original 104.1 applies on 4-2 just fine. > >>> > >>>Thanks, ACK. > >>> > >>>Pushed to: > >>>master: ed830af693c596b286b30959eb3166b59cc030c6 > >>>ipa-4-2: c5faaede276f3052517ddf86e64cb228e95dca2a > >>> > >>Thanks for anaysing and fixing these connection issues which were > >>presumably introduced by my patches. I did not hit these in my > >>testing. Admittedly I was focused on the ipa-4-2 branch and my > >>testing was mainly done there. > >> > >>Brittle LDAP connection logic in install and upgrade is an ongoing > >>problem. What shall we do to improve the situation? Push > >>connection details into the Backend and let it connect if/when > >>needed, rather than managing connection state from the outside? > >> > >>I filed a ticket: https://fedorahosted.org/freeipa/ticket/5491 > > > >I don't think we need to be smart about it, everyone just needs to make > >sure that when an ad-hoc connection is opened somewhere, it is also > >closed in the same place. The same applies for any other resource. > > > >> > >>Thanks, > >>Fraser > >> > > > > > > Maybe it would be a good idea to implement some decorator/context manager to > connect/disconnect from specified connection and wrap the CRUD code. Example > usage: > > """ > decorator: > @connected(api.Backend.ldap2, ldapi=True) > def do_magic(api, *args, **kwargs): > # do stuff > > context manager: > > with connected(api.Backend.ldap2, ccache=example_ccache): > do_some_other_magic() > """ > > I remember Jan having some objections against this so it would be nice to > hear why this is not a good idea. > This would not handle a connection loss (e.g. due to DS restart) but would still a useful and relatively non-intrusive measure. Furthermore it will make it easier for developer to do the Right Thing and reviewer to verify (i.e. it's easy to see when patch does LDAP things but is not using the context manager).
Jan, it is well and good to say "developer just has to remember to open and close the connection" - and I accept that I have been the main culprit recently - but let us make it as easy as possible to get right, and hard for a patch that does not do the proper resource management to slip through. Cheers, Fraser -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
