On Thu, 2012-03-22 at 17:19 +0100, Petr Viktorin wrote: > On 03/22/2012 04:53 PM, Martin Kosek wrote: > > On Thu, 2012-03-22 at 16:30 +0100, Petr Viktorin wrote: > >> On 03/21/2012 01:51 PM, Martin Kosek wrote: > >>> DNS plugin contains several RR type record validators run in > >>> pre_callback which cannot be used as standard param validator > >>> as it needs more data and resources that standard validators > >>> provide. However, the precallback validators are not run for > >>> DNS records created by new structured options and thus an invalid > >>> value may slip in. > >>> > >>> This patch moves the execution of these precallback validators > >>> _after_ the processing of structured DNS options. It also cleans > >>> them up a little and makes them more robust. > >>> > >>> https://fedorahosted.org/freeipa/ticket/2550 > >>> > >> > >> Functionally, the patch works fine. > >> Since you're cleaning up, I have some nitpicks, but ACK if you disagree. > >> > >> > >> Consider if instead of: > >> > >> rtype_cb = '_%s_pre_callback' % rtype > >> if hasattr(self.obj, rtype_cb): > >> getattr(self.obj, rtype_cb)(ldap, dn, entry_attrs, *keys, **options) > >> > >> this wouldn't be more readable: > >> > >> rtype_cb = getattr(self.obj, '_%s_pre_callback' % rtype, None) > >> if rtype_cb: > >> rtype_cb(ldap, dn, entry_attrs, *keys, **options) > >> > >> and since the block appears twice now, make it a method. > >> > >> Also, since is_ns_rec_resolvable raises an exception rather than > >> returning a bool, it should probably be renamed to something like > >> check_ns_rec_resolvable. > >> > > > > These are good ideas, please check the attached patch. > > > > Martin > > ACK >
Pushed to master, ipa-2-2. Martin _______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel