[Freeipa-devel] [PATCH 0184] Always return absolute idnsname in dnszone commands

[Martin Basti]

On 23/01/15 08:22, Jan Cholasta wrote:
> Dne 20.1.2015 v 12:49 Martin Basti napsal(a):
>> On 15/01/15 16:07, Jan Cholasta wrote:
>>> Dne 15.1.2015 v 15:39 Martin Basti napsal(a):
>>>> On 15/01/15 15:07, Jan Cholasta wrote:
>>>>> Dne 15.1.2015 v 14:58 Martin Basti napsal(a):
>>>>>> On 15/01/15 14:25, Jan Cholasta wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> Dne 15.1.2015 v 13:27 Martin Basti napsal(a):
>>>>>>>> On 15/01/15 13:17, Martin Basti wrote:
>>>>>>>>> https://fedorahosted.org/freeipa/ticket/4722
>>>>>>>>>
>>>>>>>>> Patch attached.
>>>>>>>>>
>>>>>>>> Fast fix.
>>>>>>>>
>>>>>>>> Updated patch attached.
>>>>>>>
>>>>>>> 1) Forward zone commands are not fixed.
>>>>>> FWzones are new and always normalized to absolute name in ldap
>>>>>
>>>>> Would you bet your money on that? Better be safe than sorry,
>>>>> especially when it's just a matter of moving the code around (right?)
>>>>>
>>>>>>>
>>>>>>> 2) It seems that the primary key returned by -mod, -del and -show
>>>>>>> (.result.value) is made absolute somewhere else in the code. 
>>>>>>> Would it
>>>>>>> be possible to do it in one place?
>>>>>> IMO it is not possible.
>>>>>>
>>>>>> Value is generated from key, and key is normalized to absolute zone
>>>>>> before calling execute.
>>>>>>
>>>>>> LDAPUpdate:
>>>>>> ...
>>>>>>          if self.obj.primary_key:
>>>>>>              pkey = keys[-1]
>>>>>>          else:
>>>>>>              pkey = None
>>>>>>
>>>>>>          return dict(result=entry_attrs, value=pkey_to_value(pkey,
>>>>>> options))
>>>>>>
>>>>>> The idnsname attribute is just taken from LDAP without any
>>>>>> normalization
>>>>>
>>>>> Right.
>>>>>
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> 3) Attribute values returned from LDAP are never None, so the if
>>>>>>> should be "if 'idnsname' in entry_attrs:".
>>>>>> Ok I will revert the change I made.
>>>>>>>
>>>>>>> 4) If idnsname always has only single value, use
>>>>>>> "entry_attrs.single_value['idnsname'] =
>>>>>>> entry_attrs.single_value['idnsname'].make_absolute()"
>>>>>> Thanks
>>>>>>>
>>>>>>> Honza
>>>>>>>
>>>>>>
>>>>>> Updated patch attached.
>>>>>>
>>>>>
>>>>>
>>>> Updated patch attached.
>>>>
>>>
>>> Thanks.
>>>
>>> Is there a reason why you put the _make_zone_absolute calls in
>>> dnszone_* and dnsforwardzone_* instead of DNSZoneBase_*?
>>>
>> I moved callback into Base classes.
>> Patch attached.
>>
>
> 1) Multi-line statements should generally use parentheses for implicit 
> line continuation rather than backslashes:
>
> +            entry_attrs.single_value['idnsname'] = (
> + entry_attrs.single_value['idnsname'].make_absolute())
>
> 2) You can remove the DN asserts from dnszone_*, they are already 
> asserted in DNSZoneBase_*.
>
> 3) Do not just ignore return values of super().post_callback():
>
>     def post_callback(self, something, ...):
>         something = super(...).post_callback(something, ...)
>         ...
>         return something
>
Updated patch attached.


-- 
Martin Basti

-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mbasti-0184.6-Always-return-absolute-idnsname-in-dnszone-commands.patch
Type: text/x-patch
Size: 3968 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150123/6d22a335/attachment.bin>