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

[Jan Cholasta]

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?

Patch 701: Since we are using python-six, shouldn't "six.PY2" be used 
instead of "sys.version_info < (3, 0)"?

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.

Patch 711: ACK

-- 
Jan Cholasta