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

[Jan Cholasta]

On 27.8.2015 16:00, Jan Cholasta wrote:
> 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.
>

I have heard no complaints, so I'm pushing the patches. Should you have 
any further concerns, they can be sorted out in additional patches.

Pushed to master: 70d1c71e46811bd3d44249d5361f99613d4ce5af

Honza

-- 
Jan Cholasta