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

[Jan Cholasta]

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?

-- 
Jan Cholasta