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

[Petr Viktorin]

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.

-- 
Petr Viktorin

-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0710.4-Modernize-use-of-range.patch
Type: text/x-patch
Size: 18052 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150825/9d788565/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0711.4-Convert-zip-result-to-list.patch
Type: text/x-patch
Size: 1223 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150825/9d788565/attachment-0001.bin>