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

From 474dde8e4597cd4bb64d1a4c57db5028eec5f0a5 Mon Sep 17 00:00:00 2001
From: Petr Viktorin <pvikt...@redhat.com>
Date: Wed, 12 Aug 2015 15:23:56 +0200
Subject: [PATCH] Modernize use of range()

In Python 3, range() behaves like the old xrange().
The difference between range() and xrange() is usually not significant,
especially if the whole result is iterated over.

Convert xrange() usage to range() for small ranges.
Use modern idioms in a few other uses of range().
---
 ipalib/parameters.py                        |  6 +++---
 ipalib/plugins/aci.py                       |  6 ++----
 ipalib/plugins/baseuser.py                  | 14 +++++++-------
 ipalib/plugins/dns.py                       |  4 ++--
 ipalib/plugins/host.py                      |  8 ++++----
 ipaserver/install/cainstance.py             |  4 ++--
 ipaserver/install/dnskeysyncinstance.py     |  2 +-
 ipaserver/install/ipa_otptoken_import.py    |  5 +++--
 ipaserver/install/plugins/rename_managed.py |  4 ++--
 ipatests/data.py                            |  2 +-
 ipatests/test_ipalib/test_base.py           |  6 +++---
 ipatests/test_ipalib/test_cli.py            |  2 +-
 ipatests/test_ipalib/test_config.py         |  4 ++--
 ipatests/test_ipalib/test_errors.py         |  2 +-
 ipatests/test_ipalib/test_frontend.py       |  4 ++--
 ipatests/test_ipalib/test_messages.py       |  2 +-
 ipatests/test_ipalib/test_plugable.py       |  4 ++--
 ipatests/test_ipapython/test_keyring.py     |  2 +-
 ipatests/test_xmlrpc/test_vault_plugin.py   |  2 +-
 19 files changed, 41 insertions(+), 42 deletions(-)

diff --git a/ipalib/parameters.py b/ipalib/parameters.py
index 1cd26b9a3a6ad561f9f65f64db5603e3624018bf..534fa886e613f77c509f2f6707d5415b1dc34063 100644
--- a/ipalib/parameters.py
+++ b/ipalib/parameters.py
@@ -1464,12 +1464,12 @@ def __init__(self, name, *rules, **kw):
 
     def _convert_scalar(self, value, index=None):
         if isinstance(value, six.string_types):
-            for i in xrange(len(value)):
-                if ord(value[i]) > 127:
+            for char in value:
+                if ord(char) > 127:
                     raise ConversionError(name=self.get_param_name(),
                         index=index,
                         error=_('The character %(char)r is not allowed.') %
-                            dict(char=value[i],)
+                            dict(char=char,)
                     )
         return super(IA5Str, self)._convert_scalar(value, index)
 
diff --git a/ipalib/plugins/aci.py b/ipalib/plugins/aci.py
index cdc8f70928fe82ff0a3e7bc4b18073c2137984ad..5aa486db895b30b7971b95bd82abcd2fa8044281 100644
--- a/ipalib/plugins/aci.py
+++ b/ipalib/plugins/aci.py
@@ -322,10 +322,8 @@ def _aci_to_kw(ldap, a, test=False, pkey_only=False):
         return kw
     kw['permissions'] = tuple(a.permissions)
     if 'targetattr' in a.target:
-        kw['attrs'] = list(a.target['targetattr']['expression'])
-        for i in xrange(len(kw['attrs'])):
-            kw['attrs'][i] = unicode(kw['attrs'][i])
-        kw['attrs'] = tuple(kw['attrs'])
+        kw['attrs'] = tuple(unicode(e)
+                            for e in a.target['targetattr']['expression'])
     if 'targetfilter' in a.target:
         target = a.target['targetfilter']['expression']
         if target.startswith('(memberOf=') or target.startswith('memberOf='):
diff --git a/ipalib/plugins/baseuser.py b/ipalib/plugins/baseuser.py
index c3e2903710649e2eda395f49fe4d26d03285bff8..ed7c1a9d360a89ce0640dd63e748596993bb8b6c 100644
--- a/ipalib/plugins/baseuser.py
+++ b/ipalib/plugins/baseuser.py
@@ -427,16 +427,16 @@ def normalize_manager(self, manager, container):
             manager = [manager]
         try:
             container_dn = DN(container, api.env.basedn)
-            for m in xrange(len(manager)):
-                if isinstance(manager[m], DN) and manager[m].endswith(container_dn):
+            for i, mgr in enumerate(manager):
+                if isinstance(mgr, DN) and mgr.endswith(container_dn):
                     continue
                 entry_attrs = self.backend.find_entry_by_attr(
-                        self.primary_key.name, manager[m], self.object_class, [''],
+                        self.primary_key.name, mgr, self.object_class, [''],
                         container_dn
                     )
-                manager[m] = entry_attrs.dn
+                manager[i] = entry_attrs.dn
         except errors.NotFound:
-            raise errors.NotFound(reason=_('manager %(manager)s not found') % dict(manager=manager[m]))
+            raise errors.NotFound(reason=_('manager %(manager)s not found') % dict(manager=mgr))
 
         return manager
 
@@ -448,8 +448,8 @@ def convert_manager(self, entry_attrs, **options):
              return
 
         if 'manager' in entry_attrs:
-            for m in xrange(len(entry_attrs['manager'])):
-                entry_attrs['manager'][m] = self.get_primary_key_from_dn(entry_attrs['manager'][m])
+            for i, mgr in enumerate(entry_attrs['manager']):
+                entry_attrs['manager'][i] = self.get_primary_key_from_dn(mgr)
 
     def _user_status(self, user, container):
         assert isinstance(user, DN)
diff --git a/ipalib/plugins/dns.py b/ipalib/plugins/dns.py
index 335b58afb71e20e10439ccaa620750a720750690..81c7e06532f4bb1fa73831c365d61247ea063b26 100644
--- a/ipalib/plugins/dns.py
+++ b/ipalib/plugins/dns.py
@@ -1751,7 +1751,7 @@ def _get_auth_zone_ldap(name):
     # Create all possible parent zone names
     search_name = name.make_absolute()
     zone_names = []
-    for i in xrange(len(search_name)):
+    for i, name in enumerate(search_name):
         zone_name_abs = DNSName(search_name[i:]).ToASCII()
         zone_names.append(zone_name_abs)
         # compatibility with IPA < 4.0, zone name can be relative
@@ -1826,7 +1826,7 @@ def _get_longest_match_ns_delegation_ldap(zone, name):
 
     # create list of possible record names
     possible_record_names = [DNSName(relative_record_name[i:]).ToASCII()
-                             for i in xrange(len(relative_record_name))]
+                             for i in range(len(relative_record_name))]
 
     # search filters
     name_filter = ldap.make_filter({'idnsname': [possible_record_names]})
diff --git a/ipalib/plugins/host.py b/ipalib/plugins/host.py
index 6380962e7839f96d8868b488ca68dd0f85da12ae..532ff66607911cfd8e1a3407b0f361641bb7992b 100644
--- a/ipalib/plugins/host.py
+++ b/ipalib/plugins/host.py
@@ -774,10 +774,10 @@ def pre_callback(self, ldap, dn, *keys, **options):
                                         for t in _record_types]
                     for attr in _attribute_types:
                         if attr not in ['arecord', 'aaaarecord'] and attr in record:
-                            for i in xrange(len(record[attr])):
-                                if (record[attr][i].endswith(parts[0]) or
-                                    record[attr][i].endswith(fqdn+'.')):
-                                    delkw = { unicode(attr) : record[attr][i] }
+                            for val in record[attr]:
+                                if (val.endswith(parts[0]) or
+                                        val.endswith(fqdn + '.')):
+                                    delkw = {unicode(attr): val}
                                     api.Command['dnsrecord_del'](domain,
                                             record['idnsname'][0],
                                             **delkw)
diff --git a/ipaserver/install/cainstance.py b/ipaserver/install/cainstance.py
index 55c02224db8454856bf9fd9bb3977cb75c62f282..6ca4c0ad74f11051ddfeb0a705c6c4d0fb3ae11b 100644
--- a/ipaserver/install/cainstance.py
+++ b/ipaserver/install/cainstance.py
@@ -806,8 +806,8 @@ def __issue_ra_cert(self):
         chain = chain.split("\n")
 
         root_nickname=[]
-        for i in xrange(len(chain)):
-            m = re.match('\ *"(.*)" \[.*', chain[i])
+        for part in chain:
+            m = re.match('\ *"(.*)" \[.*', part)
             if m:
                 nick = m.groups(0)[0]
                 if nick != "ipa-ca-agent" and nick[:7] != "Builtin":
diff --git a/ipaserver/install/dnskeysyncinstance.py b/ipaserver/install/dnskeysyncinstance.py
index 482250feec5921bb629f3b60f94e288819fcdfa2..68130c92558a4feb8d08fa826dbf6333d4461d1f 100644
--- a/ipaserver/install/dnskeysyncinstance.py
+++ b/ipaserver/install/dnskeysyncinstance.py
@@ -310,7 +310,7 @@ def __setup_replica_keys(self):
                 # check if key with this ID exist in softHSM
                 # id is 16 Bytes long
                 key_id = "".join(chr(random.randint(0, 255))
-                                 for _ in xrange(0, 16))
+                                 for _ in range(0, 16))
                 replica_pubkey_dn = DN(('ipk11UniqueId', 'autogenerate'), dn_base)
 
 
diff --git a/ipaserver/install/ipa_otptoken_import.py b/ipaserver/install/ipa_otptoken_import.py
index 009ecf5d4397fe7411483988fb7e72f470aacb89..fceb8b14d6e8ed032cd255f86bd1d99a7999061d 100644
--- a/ipaserver/install/ipa_otptoken_import.py
+++ b/ipaserver/install/ipa_otptoken_import.py
@@ -31,6 +31,7 @@
 import dateutil.tz
 import nss.nss as nss
 import krbV
+from six.moves import xrange
 
 from ipapython import admintool
 from ipalib import api, errors
@@ -190,7 +191,7 @@ def derive(self, masterkey):
 
         # Loop through each block adding it to the derived key.
         dk = []
-        for i in xrange(1, blocks + 1):
+        for i in range(1, blocks + 1):
             # Set initial values.
             last = self.salt + struct.pack('>I', i)
             hash = [0] * mac.digest_size
@@ -202,7 +203,7 @@ def derive(self, masterkey):
                 last = tmp.digest()
 
                 # XOR the previous hash with the new hash.
-                for k in xrange(mac.digest_size):
+                for k in range(mac.digest_size):
                     hash[k] ^= ord(last[k])
 
             # Add block to derived key.
diff --git a/ipaserver/install/plugins/rename_managed.py b/ipaserver/install/plugins/rename_managed.py
index 9cff18b76a649db0018ffe51bf43f83a8043ee61..4b42d48dc55b877fd1408ca422d6fbd2351605fe 100644
--- a/ipaserver/install/plugins/rename_managed.py
+++ b/ipaserver/install/plugins/rename_managed.py
@@ -33,8 +33,8 @@ def entry_to_update(entry):
     update = []
     for attr in entry.keys():
         if isinstance(entry[attr], list):
-            for i in xrange(len(entry[attr])):
-                update.append(dict(attr=str(attr), value=str(entry[attr][i])))
+            for item in entry[attr]:
+                update.append(dict(attr=str(attr), value=str(item)))
         else:
             update.append(dict(attr=str(attr), value=str(entry[attr])))
 
diff --git a/ipatests/data.py b/ipatests/data.py
index 9332a53a52c579cb9469740394fcc3fb3cc9bc81..5abfffcfdb0665d06afdc26dcbda8408936ddc97 100644
--- a/ipatests/data.py
+++ b/ipatests/data.py
@@ -25,7 +25,7 @@
 
 
 # A string that should have bytes 'x\00' through '\xff':
-binary_bytes = ''.join(struct.pack('B', d) for d in xrange(256))
+binary_bytes = ''.join(struct.pack('B', d) for d in range(256))
 assert '\x00' in binary_bytes and '\xff' in binary_bytes
 assert type(binary_bytes) is str and len(binary_bytes) == 256
 
diff --git a/ipatests/test_ipalib/test_base.py b/ipatests/test_ipalib/test_base.py
index 0117946bc2de8cb2aa014e42e6c6569b33f0900b..265e118585862a5f3c77ba8092943d9d52dccecf 100644
--- a/ipatests/test_ipalib/test_base.py
+++ b/ipatests/test_ipalib/test_base.py
@@ -209,7 +209,7 @@ class test_NameSpace(ClassChecker):
     _cls = base.NameSpace
 
     def new(self, count, sort=True):
-        members = tuple(DummyMember(i) for i in xrange(count, 0, -1))
+        members = tuple(DummyMember(i) for i in range(count, 0, -1))
         assert len(members) == count
         o = self.cls(members, sort=sort)
         return (o, members)
@@ -305,12 +305,12 @@ def test_getitem(self):
             e = raises(KeyError, o.__getitem__, 'nope')
 
             # Test int indexes:
-            for i in xrange(cnt):
+            for i in range(cnt):
                 assert o[i] is members[i]
             e = raises(IndexError, o.__getitem__, cnt)
 
             # Test negative int indexes:
-            for i in xrange(1, cnt + 1):
+            for i in range(1, cnt + 1):
                 assert o[-i] is members[-i]
             e = raises(IndexError, o.__getitem__, -(cnt + 1))
 
diff --git a/ipatests/test_ipalib/test_cli.py b/ipatests/test_ipalib/test_cli.py
index 4c9ae61c4d959b4fec1c7b59bd83c46dc2b3c9fd..0c3affcd34c72e431b3739b72598c93d88985522 100644
--- a/ipatests/test_ipalib/test_cli.py
+++ b/ipatests/test_ipalib/test_cli.py
@@ -91,7 +91,7 @@ def __get_cmd(self):
     Command = property(__get_cmd)
 
     def __cmd_iter(self, cnt):
-        for i in xrange(cnt):
+        for i in range(cnt):
             yield DummyCommand(get_cmd_name(i))
 
     def finalize(self):
diff --git a/ipatests/test_ipalib/test_config.py b/ipatests/test_ipalib/test_config.py
index f414d55e48f08995363f9cc49597f8c7d760b039..b8cba516da21d88448f35ffb9a3e624dc7d8c147 100644
--- a/ipatests/test_ipalib/test_config.py
+++ b/ipatests/test_ipalib/test_config.py
@@ -298,7 +298,7 @@ def test_len(self):
         """
         o = self.cls()
         assert len(o) == 0
-        for i in xrange(1, 11):
+        for i in range(1, 11):
             key = 'key%d' % i
             value = u'value %d' % i
             o[key] = value
@@ -390,7 +390,7 @@ def test_merge_from_file(self):
         for (k, v) in orig.items():
             assert o[k] is v
         assert list(o) == sorted(keys + ('key0', 'key1', 'key2', 'key3', 'config_loaded'))
-        for i in xrange(4):
+        for i in range(4):
             assert o['key%d' % i] == ('var%d' % i)
         keys = tuple(o)
 
diff --git a/ipatests/test_ipalib/test_errors.py b/ipatests/test_ipalib/test_errors.py
index dfe60f2830bc8401beca027a8000ec4e76fd2574..83180060667e4fc36924e3950ff6e5c8617bf19f 100644
--- a/ipatests/test_ipalib/test_errors.py
+++ b/ipatests/test_ipalib/test_errors.py
@@ -369,7 +369,7 @@ def extratest(self, cls):
 
 class test_PublicErrors(object):
     message_list = errors.public_errors
-    errno_range = xrange(900, 5999)
+    errno_range = list(range(900, 5999))
     required_classes = (StandardError, errors.PublicError)
     texts = errors._texts
 
diff --git a/ipatests/test_ipalib/test_frontend.py b/ipatests/test_ipalib/test_frontend.py
index 3798c4ed838e78ae368bed815ff8c279bb09e972..c74646dc10438c8c8e5ee8968fdfea1812cf9abb 100644
--- a/ipatests/test_ipalib/test_frontend.py
+++ b/ipatests/test_ipalib/test_frontend.py
@@ -940,7 +940,7 @@ def __clone__(self, attr_name):
 
         def get_attributes(cnt, format):
             for name in ['other', 'user', 'another']:
-                for i in xrange(cnt):
+                for i in range(cnt):
                     yield DummyAttribute(name, format % i)
 
         cnt = 10
@@ -970,7 +970,7 @@ class user(self.cls):
         assert isinstance(namespace, plugable.NameSpace)
         assert len(namespace) == cnt
         f = methods_format
-        for i in xrange(cnt):
+        for i in range(cnt):
             attr_name = f % i
             attr = namespace[attr_name]
             assert isinstance(attr, DummyAttribute)
diff --git a/ipatests/test_ipalib/test_messages.py b/ipatests/test_ipalib/test_messages.py
index 686bf8dd53b61d3c5074a90e2eead1f844f3fbdb..966a6c133176381be9c921c77073d760f81278e6 100644
--- a/ipatests/test_ipalib/test_messages.py
+++ b/ipatests/test_ipalib/test_messages.py
@@ -41,7 +41,7 @@ class test_PublicMessage(test_errors.test_PublicError):
 
 class test_PublicMessages(test_errors.BaseMessagesTest):
     message_list = messages.public_messages
-    errno_range = xrange(10000, 19999)
+    errno_range = list(range(10000, 19999))
     required_classes = (UserWarning, messages.PublicMessage)
     texts = messages._texts
 
diff --git a/ipatests/test_ipalib/test_plugable.py b/ipatests/test_ipalib/test_plugable.py
index c0b88d1f97bf3ddd6a8b68c920a5b73b1e4be41d..1481394648d191e20d3605ff49d405725421e8ea 100644
--- a/ipatests/test_ipalib/test_plugable.py
+++ b/ipatests/test_ipalib/test_plugable.py
@@ -239,14 +239,14 @@ def get_base_name(b):
         def get_plugin_name(b, p):
             return 'base%d_plugin%d' % (b, p)
 
-        for b in xrange(2):
+        for b in range(2):
             base_name = get_base_name(b)
             base = locals()[base_name]
             ns = getattr(api, base_name)
             assert isinstance(ns, plugable.NameSpace)
             assert read_only(api, base_name) is ns
             assert len(ns) == 3
-            for p in xrange(3):
+            for p in range(3):
                 plugin_name = get_plugin_name(b, p)
                 plugin = locals()[plugin_name]
                 inst = ns[plugin_name]
diff --git a/ipatests/test_ipapython/test_keyring.py b/ipatests/test_ipapython/test_keyring.py
index bd876ea77007137fbad7b078eeee9a8a6bab94ab..9a5fc98ea9fe731c50b6708b547d06c80cb823d5 100644
--- a/ipatests/test_ipapython/test_keyring.py
+++ b/ipatests/test_ipapython/test_keyring.py
@@ -89,7 +89,7 @@ def test_04(self):
         assert(result == UPDATE_VALUE)
 
         # Now update it 10 times
-        for i in xrange(10):
+        for i in range(10):
             kernel_keyring.update_key(TEST_KEY, 'test %d' %  i)
             result = kernel_keyring.read_key(TEST_KEY)
             assert(result == 'test %d' % i)
diff --git a/ipatests/test_xmlrpc/test_vault_plugin.py b/ipatests/test_xmlrpc/test_vault_plugin.py
index 816b6bd2f2f621e1e3a60e1d762cf108ba147459..c97fc761c598776c71d18f03915c138eee0d8a94 100644
--- a/ipatests/test_xmlrpc/test_vault_plugin.py
+++ b/ipatests/test_xmlrpc/test_vault_plugin.py
@@ -34,7 +34,7 @@
 asymmetric_vault_name = u'asymmetric_test_vault'
 
 # binary data from \x00 to \xff
-secret = ''.join(chr(c) for c in xrange(0, 256))
+secret = ''.join(chr(c) for c in range(0, 256))
 
 password = u'password'
 
-- 
2.1.0

From 4ac0d9bf99455c1c101075d8cb2f2c003bcb3189 Mon Sep 17 00:00:00 2001
From: Petr Viktorin <pvikt...@redhat.com>
Date: Wed, 12 Aug 2015 15:39:46 +0200
Subject: [PATCH] Convert zip() result to list()

In Python 3, zip() returns an iterator. To get a list, it must
be explicitly converted.
In most cases, zip() result is iterated over so this is not
necessary.
---
 ipatests/test_ipapython/test_ipautil.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ipatests/test_ipapython/test_ipautil.py b/ipatests/test_ipapython/test_ipautil.py
index f2e032ed420acfda89387a3179e57305fff29530..c5a509835d9a12a9d3eaabdafbc4e348b8c6c180 100644
--- a/ipatests/test_ipapython/test_ipautil.py
+++ b/ipatests/test_ipapython/test_ipautil.py
@@ -172,8 +172,8 @@ def test_items(self):
         assert ("key2", "val2") in items_set
         assert ("KEY3", "VAL3") in items_set
 
-        assert list(self.cidict.items()) == list(self.cidict.iteritems()) == zip(
-            self.cidict.keys(), self.cidict.values())
+        assert list(self.cidict.items()) == list(self.cidict.iteritems()) == list(zip(
+            self.cidict.keys(), self.cidict.values()))
 
     def test_iter(self):
         items = []
-- 
2.1.0

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to