[Freeipa-devel] [PATCH] 241 Fix precallback validators in DNS plugin

[Petr Viktorin]

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

-- 
Petr³