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

Thu Aug 27 10:17:02 UTC 2015

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
> 

-- 
Petr Viktorin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0696.6-Remove-use-of-sys.exc_value.patch
Type: text/x-patch
Size: 2529 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150827/a3b6e130/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0697.6-Don-t-use-a-tuple-in-function-arguments.patch
Type: text/x-patch
Size: 907 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150827/a3b6e130/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0698.6-Add-python-six-to-dependencies.patch
Type: text/x-patch
Size: 984 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150827/a3b6e130/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0699.6-Remove-the-unused-pygettext-script.patch
Type: text/x-patch
Size: 30062 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150827/a3b6e130/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0700.6-Use-six.string_types-instead-of-basestring.patch
Type: text/x-patch
Size: 35449 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150827/a3b6e130/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0701.6-Use-Python3-compatible-dict-method-names.patch
Type: text/x-patch
Size: 74109 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150827/a3b6e130/attachment-0005.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0702.6-Replace-filter-calls-with-list-comprehensions.patch
Type: text/x-patch
Size: 6514 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150827/a3b6e130/attachment-0006.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0703.6-Use-six.moves.input-instead-of-raw_input.patch
Type: text/x-patch
Size: 3100 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150827/a3b6e130/attachment-0007.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0704.6-Use-six.integer_types-instead-of-long-int.patch
Type: text/x-patch
Size: 8301 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150827/a3b6e130/attachment-0008.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0705.6-Replace-uses-of-map.patch
Type: text/x-patch
Size: 18773 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150827/a3b6e130/attachment-0009.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0706.6-Use-next-function-on-iterators.patch
Type: text/x-patch
Size: 3481 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150827/a3b6e130/attachment-0010.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0707.6-Use-the-print-function.patch
Type: text/x-patch
Size: 229158 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150827/a3b6e130/attachment-0011.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0708.6-Use-new-style-raise-syntax.patch
Type: text/x-patch
Size: 6956 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150827/a3b6e130/attachment-0012.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0709.6-Use-six.reraise.patch
Type: text/x-patch
Size: 1892 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150827/a3b6e130/attachment-0013.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0710.6-Modernize-use-of-range.patch
Type: text/x-patch
Size: 20463 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150827/a3b6e130/attachment-0014.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0711.6-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/20150827/a3b6e130/attachment-0015.bin>