[Freeipa-devel] [PATCHES] 0696-0710 More modernization

[Jan Cholasta]

On 27.8.2015 12:17, Petr Viktorin wrote:
> On 08/27/2015 09:07 AM, Jan Cholasta wrote:
>> On 26.8.2015 12:15, Petr Viktorin wrote:
>>> On 08/26/2015 09:47 AM, Jan Cholasta wrote:
>>>> On 25.8.2015 15:05, Petr Viktorin wrote:
>>>>> On 08/25/2015 12:39 PM, Christian Heimes wrote:
>>>>>> On 2015-08-24 17:31, Petr Viktorin wrote:
>>>>>>>>>> 0701.2-Use-Python3-compatible-dict-method-names
>>>>>>>>>> NACK
>>>>>>>>>> Why are you replacing iteritems() with items() instead of using
>>>>>>>>>> six.iteritems()?
>>>>>>>
>>>>>>> It looks cleaner, and it will be easier to clean up after six is
>>>>>>> dropped.
>>>>>>> Also, the performance difference is negligible if the whole thing is
>>>>>>> iterated over. (On small dicts, which are the majority of what
>>>>>>> iteritems
>>>>>>> was used on, items() is actually a bit faster on my machine.)
>>>>>>
>>>>>> Right, for small dicts the speed difference is negligible and
>>>>>> favors the
>>>>>> items() over iteritems(). For medium sized and large dicts the
>>>>>> iterators
>>>>>> are faster and consume less memory.
>>>>>>
>>>>>> I'm preferring iterator methods everywhere because I don't have to
>>>>>> worry
>>>>>> about dict sizes.
>>>>>>
>>>>>>>>>> 0710.2-Modernize-use-of-range
>>>>>>>>>> NACK
>>>>>>>>>> Please use six.moves.range. It defaults to xrange() in Python 2.
>>>>>>>
>>>>>>> Why? What is the benefit of xrange in these situations?
>>>>>>>
>>>>>>> Like with iteritems in 0701, when iterating over the whole thing, the
>>>>>>> performance difference is negligible. I don't think a few
>>>>>>> microseconds
>>>>>>> outside of tight loops are worth the verbosity.
>>>>>>
>>>>>> It's the same reasoning as in 0701. As long as you have a small range,
>>>>>> it doesn't make a difference. For large ranges the additional memory
>>>>>> usage can be problematic.
>>>>>>
>>>>>> In all above cases the iterator methods and generator functions are a
>>>>>> safer choice. A malicious user can abuse the non-iterative methods for
>>>>>> DoS attacks. As long as the input can't be controlled by a user and
>>>>>> the
>>>>>> range/dict/set/list is small, the non-iterative methods are fine. You
>>>>>> have to verify every location, though.
>>>>>
>>>>> Keep in mind that for dicts, all the data is in memory already (in the
>>>>> dict). Are you worried about DoS from an extra list of references to
>>>>> existing objects?
>>>>>
>>>>>> I'm usually too busy with other stuff (aka lazy) to verify these
>>>>>> preconditions...
>>>>>
>>>>> I changed one questionable use of range. The other ones are:
>>>>>
>>>>> ipalib/plugins/dns.py: for i in range(len(relative_record_name)
>>>>> (max. ~255, verified by DNSName)
>>>>>
>>>>> ipaserver/install/dnskeysyncinstance.py: for _ in range(0, 16))
>>>>> (16)
>>>>>
>>>>> ipaserver/install/ipa_otptoken_import.py: for i in range(1, blocks +
>>>>> 1):
>>>>> (about 7)
>>>>>
>>>>> ipaserver/install/ipa_otptoken_import.py: for k in
>>>>> range(mac.digest_size):
>>>>> (16)
>>>>>
>>>>> ipatests/data.py: for d in range(256))
>>>>> (256)
>>>>>
>>>>> Plus tests, where it's rarely above 10.
>>>>>
>>>>
>>>> I have just pushed Michael's python-krbV removal patch, which conflicts
>>>> with your patches. Could you please rebase them on top of current
>>>> master?
>>>>
>>>
>>> Sure, here they are.
>>>
>>>
>>
>> Patches 696-699: ACK
>>
>> Patch 700: There are some error messages which contain basestring. Do we
>> want to fix these as well?
>
> No, I think it's okay as a term. When Python 2 is dropped it can be
> shortened to str.
>
>> Patch 701: Since we are using python-six, shouldn't "six.PY2" be used
>> instead of "sys.version_info < (3, 0)"?
>
> Right, it looks a bit better.
>
>> Patches 702-709: ACK
>>
>> Patch 710: There are some "xrange"s in ipapython/p11helper.py and
>> ipatests/test_xmlrpc/test_add_remove_cert_cmd.py.
>
> Thanks, fixed.
>
>> Patch 711: ACK
>>
>

Thanks, ACK.

Christian, if you don't have any objections, I will push the patches.

-- 
Jan Cholasta