[Freeipa-devel] [PATCH] 300-301 Fix DNS SOA serial parameters boundaries

[Petr Viktorin]

On 09/05/2012 01:31 PM, Martin Kosek wrote:
> On 09/05/2012 12:26 PM, Petr Viktorin wrote:
>> On 09/05/2012 12:14 PM, Petr Viktorin wrote:
>>> This works well, but please see some comments below.
>>>
>>> On 09/04/2012 04:22 PM, Martin Kosek wrote:
>>>> To test, simply run the following command:
>>>>
>>>>    ipa dnszone-mod example.com --serial=4294967295
>>>>
>>>> This should work well on patched server&client. Web UI should work too
>>>> as it
>>>> reads the max limit dynamically.
>>>
>>> Please put this in the test suite.
>
> Done.
>
>>>
>>>> ---
>>>> [PATCH 2/2] Fix DNS SOA serial parameters boundaries:
>>>>
>>>> Set correct boundaries for DNS SOA serial parameters (see RFC 1035,
>>>> 2181).
>>>>
>>>>
>>>> [PATCH 1/2] Transfer long numbers over XMLRPC
>>>>
>>>> Numeric parameters in ipalib were limited by XMLRPC boundaries for
>>>> integer (2^31-1) which is too low for some LDAP attributes like DNS
>>>> SOA serial field.
>>>>
>>>> Transfer numbers which are not in XMLRPC boundary as a string and not
>>>> as a number to workaround this limitation. Int parameter had to be
>>>> updated to also accept Python's long type as valid int type.
>>>>
>>>>
>>>> freeipa-mkosek-300-transfer-long-numbers-over-xmlrpc.patch
>>>>
>>>>
>>>>>  From 8782015a17b130c5ebae8b014a7241810b10dedd Mon Sep 17 00:00:00 2001
>>>> From: Martin Kosek<mkosek at redhat.com>
>>>> Date: Tue, 4 Sep 2012 15:49:26 +0200
>>>> Subject: [PATCH 1/2] Transfer long numbers over XMLRPC
>>>>
>>>> Numeric parameters in ipalib were limited by XMLRPC boundaries for
>>>> integer (2^31-1) which is too low for some LDAP attributes like DNS
>>>> SOA serial field.
>>>>
>>>> Transfer numbers which are not in XMLRPC boundary as a string and not
>>>> as a number to workaround this limitation. Int parameter had to be
>>>> updated to also accept Python's long type as valid int type.
>>>>
>>>> https://fedorahosted.org/freeipa/ticket/2568
>>>> ---
>>>>    ipalib/parameters.py | 12 ++++++------
>>>>    ipalib/rpc.py        |  5 ++++-
>>>>    2 files changed, 10 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/ipalib/parameters.py b/ipalib/parameters.py
>>>> index
>>>> de0d14faf08d1ab79c99e65dab9cc08f406e3a1d..21e30356b2a351bf7a3be7d47d7fabf0130cf6d4
>>>>
>>>> 100644
>>>> --- a/ipalib/parameters.py
>>>> +++ b/ipalib/parameters.py
>>>> @@ -1077,7 +1077,7 @@ class Number(Param):
>>>>            """
>>>>            if type(value) is self.type:
>>>>                return value
>>>> -        if type(value) in (unicode, int, float):
>>>> +        if type(value) in (unicode, int, long, float):
>>>
>>>
>>> PEP8 says that "Object type comparisons should always use isinstance()
>>> instead of comparing types directly".
>>> It would be nice to change the old code whenever it is touched. It's
>>> also in a few more places in the patch.
>>>
>
> I considered doing this when I was developing the patch, but I did not want to
> mix type/isinstance in the same code. Now, when I experimented and tried to
> replace it in a larger scope, there were unexpected issues. Like bool type
> suddenly passing an isinstance(value, (int, long)) test and causing issues later.
>
> So I skipped this part (as we discussed off-list).
>
>>>> diff --git a/ipalib/rpc.py b/ipalib/rpc.py
>>>> index
>>>> d1764e3e30492d5855450398e86689bfcad7aa39..85239ac65903acf447a4d971cce70f819979ce8d
>>>>
>>>> 100644
>>>> --- a/ipalib/rpc.py
>>>> +++ b/ipalib/rpc.py
>>>> @@ -94,6 +95,8 @@ def xml_wrap(value):
>>>>        if type(value) is Decimal:
>>>>            # transfer Decimal as a string
>>>>            return unicode(value)
>>>> +    if isinstance(value, (int, long)) and (value < MININT or value >
>>>> MAXINT):
>>>> +        return unicode(value)
>>>>        if isinstance(value, DN):
>>>>            return str(value)
>>>>        assert type(value) in (unicode, int, float, bool, NoneType)
>>>
>>> `long` should also be included here.
>>>
>>>   >>> api.Command.user_find(uidnumber=1511000000)
>>> {'count': 1, 'truncated': False, 'result': ({...},), 'summary': u'1 user
>>> matched'}
>>>   >>> api.Command.user_find(uidnumber=1511000000 + 0L)
>>> Traceback (most recent call last):
>>>     File "<console>", line 1, in <module>
>>> ...
>>>     File "/usr/lib/python2.7/site-packages/ipalib/rpc.py", line 102, in
>>> xml_wrap
>>>       assert type(value) in (unicode, int, float, bool, NoneType)
>>> AssertionError
>>>
>>>
>>>
>>>
>>
>> One more thing: please update the VERSION file.
>>
>
> I did not want to do this because I just messed with validation, but yeah, I
> can do that.
>
> Fixed patches attached.
>

Both are good, ACK.


-- 
Petr³