[PATCH] even more python patches
Douglas Bagnall
douglas.bagnall at catalyst.net.nz
Wed Oct 31 23:50:31 UTC 2018
On 1/11/18 1:22 AM, Noel Power wrote:
> Hi Douglas
>
> On 28/10/2018 21:56, Douglas Bagnall via samba-technical wrote:
>> I know you don't like to miss out on reviewing trivial python patches,
>> so I prepared another batch.
>>
>> *Most* of these are easy.
>>
>> They passed CI with the others, and are running on their own at
>> https://gitlab.com/samba-team/devel/samba/pipelines/34610880
>>
>> cheers,
>> Douglas
>
>> [PATCH 01/17] python dbcheck: don't use mutable default args
>
> I think already this is handled, 'attrs' is not modified it is passed to
> check_object which makes a copy of it (there is also an accompanying
> comment about the pass-by-reference 'attrs' variable). I am not against
> the change per-se, maybe it will prevent accidental modification of
> attrs if 'check_database' is changed in the future.
That is the reason for the change -- mutable default arguments are just
waiting to cause you grief. Just as in C where we initialise to NULL for
provably no benefit at most particular moments in time, in python there
are things we should do to reduce the risk of inadvertent disaster.
But I do see your point here:
> OTOH I don't like the fact the default is expressed as a tuple, to me it
> sortof documents that the expected param type (which is list) now looks
> like it needs to be a tuple.
I have reworked and squashed patches 1 and 2 together. In the controls
case, None does the right thing anyway, and for attrs, we can pass down
None until it actually gets used.
>> [PATCH 05/17] selftesthelpers: use immutable value for extra_path
>>
>> -def planpythontestsuite(env, module, name=None, extra_path=[],
>> py3_compatible=False):
>> +def planpythontestsuite(env, module, name=None, extra_path=(),
>> py3_compatible=False):
>> if name is None:
>> name = module
>> pypath = list(extra_path)
>>
> like above extra_path is never used directly & pypath is copied
>
> I wont object to the 2 above if you wish to make those changes, I just
> think we are fixing something that is already catered for.
Looking again, I found a bug in this function. Non-empty extra_path
causes an environment variable to be set as args[0], then py3_compatible
assumes args[0] is the python interpreter.
>> [PATCH 08/17] tests/py/rodc_rwdc: avoid py2/py3 .next compat issues
>>
>> class RodcRwdcTests(password_lockout_base.BasePasswordTestCase):
>> - counter = itertools.count(1).next
>> + next_user_number = [1]
>>
>
> I didn't get why a list was used for next_user_number, was there some
> reason why just a normal variable wouldn't work? Anyway I think if the
> orig style of code was to be ported below would be more in line and
> *should* (hasn't been tested) work
OK, yes, that was a sad hack. The reason a normal variable won't work is
each test gets its own instance of the class, and assigning to it will
only overlay the class copy with an instance copy, meaning the counter
doesn't count.
I'm going to leave that out right now, because it seems more important
that I have lunch than sort it out.
>
>> [PATCH 12/17] s4/scripting/*: py3 compatible print
>>
>
>> diff --git a/source4/scripting/bin/findprovisionusnranges
>> b/source4/scripting/bin/findprovisionusnranges
>> index ee9da3d421e..65e5b3838bc 100755
>> --- a/source4/scripting/bin/findprovisionusnranges
>> +++ b/source4/scripting/bin/findprovisionusnranges
>> @@ -18,7 +18,7 @@
>> # You should have received a copy of the GNU General Public License
>> # along with this program. If not, see <http://www.gnu.org/licenses/>.
>> #
>> -
>> +from __future__ import print_function
>> import sys
>> import optparse
>> sys.path.insert(0, "bin/python")
> do we need the 'from __future__ import print_function here?
>
> same in patch for source4/scripting/bin/mymachinepw and
> python/samba/common.py
>
> don't see we need it for these, if this is the case let's avoid another
> unnecessary import
OK, removed those.
It seems to me though that from __future__ import print_function is a
harmless enough import though, because when the day comes we can remove
them all in a single swoop. And in the mean time it prevents
regressions. But I doubt we'll see a flurry of change in mymachinepw in
any case, so that argument is weak.
The attached patches are the new/changed ones, and the whole lot is at
https://gitlab.com/samba-team/devel/samba/pipelines/35027769.
cheers,
Douglas
-------------- next part --------------
From fa0224982dbffcc0fd77d61f8c66dd9b249567ca Mon Sep 17 00:00:00 2001
From: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Date: Fri, 26 Oct 2018 19:33:48 +1300
Subject: [PATCH 1/3] python dbcheck: don't use mutable default args
In this code
def f(a, b=[]):
b.append(a)
return b
all single argument calls to f() will affect the same copy of b.
In the controls case, controls=None has the same effect as
controls=[].
Signed-off-by: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
---
python/samba/dbchecker.py | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/python/samba/dbchecker.py b/python/samba/dbchecker.py
index c70ca7bc212..a5a2c19cd07 100644
--- a/python/samba/dbchecker.py
+++ b/python/samba/dbchecker.py
@@ -226,7 +226,8 @@ class dbcheck(object):
raise
pass
- def check_database(self, DN=None, scope=ldb.SCOPE_SUBTREE, controls=[], attrs=['*']):
+ def check_database(self, DN=None, scope=ldb.SCOPE_SUBTREE, controls=None,
+ attrs=None):
'''perform a database check, returning the number of errors found'''
res = self.samdb.search(base=DN, scope=scope, attrs=['dn'], controls=controls)
self.report('Checking %u objects' % len(res))
@@ -2025,14 +2026,15 @@ newSuperior: %s""" % (str(from_dn), str(to_rdn), str(to_base)))
raise KeyError
- def check_object(self, dn, attrs=['*']):
+ def check_object(self, dn, attrs=None):
'''check one object'''
if self.verbose:
self.report("Checking object %s" % dn)
-
- # If we modify the pass-by-reference attrs variable, then we get a
- # replPropertyMetadata for every object that we check.
- attrs = list(attrs)
+ if attrs is None:
+ attrs = ['*']
+ else:
+ # make a local copy to modify
+ attrs = list(attrs)
if "dn" in map(str.lower, attrs):
attrs.append("name")
if "distinguishedname" in map(str.lower, attrs):
--
2.17.1
From 4c8a79a77fca0c0bfcb2b5bd7b54183da8fad5bd Mon Sep 17 00:00:00 2001
From: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Date: Sun, 28 Oct 2018 10:53:49 +1300
Subject: [PATCH 2/3] selftesthelpers: fix py3 tests with extra_path
If a test was supplied with extra_path, a PYTHONPATH= env variable was
prepended to the args list, but the py3_compatible clause assumed the
first args element was /usr/bin/python.
Signed-off-by: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
---
selftest/selftesthelpers.py | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/selftest/selftesthelpers.py b/selftest/selftesthelpers.py
index 25977efe111..b0fc79bd18e 100644
--- a/selftest/selftesthelpers.py
+++ b/selftest/selftesthelpers.py
@@ -136,18 +136,21 @@ def planperltestsuite(name, path):
skiptestsuite(name, "Test::More not available")
-def planpythontestsuite(env, module, name=None, extra_path=[], py3_compatible=False):
+def planpythontestsuite(env, module, name=None, extra_path=None,
+ py3_compatible=False):
if name is None:
name = module
- pypath = list(extra_path)
args = [python, "-m", "samba.subunit.run", "$LISTOPT", "$LOADLIST", module]
- if pypath:
- args.insert(0, "PYTHONPATH=%s" % ":".join(["$PYTHONPATH"] + pypath))
- plantestsuite_loadlist(name, env, args)
+ if extra_path:
+ pypath = ["PYTHONPATH=$PYTHONPATH:%s" % ":".join(extra_path)]
+ else:
+ pypath = []
+
+ plantestsuite_loadlist(name, env, pypath + args)
if py3_compatible and extra_python is not None:
# Plan one more test for Python 3 compatible module
args[0] = extra_python
- plantestsuite_loadlist(name + ".python3", env, args)
+ plantestsuite_loadlist(name + ".python3", env, pypath + args)
def get_env_torture_options():
--
2.17.1
From c64a7ef3a94fb074bb4b3153bc3a88b880eea301 Mon Sep 17 00:00:00 2001
From: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Date: Sun, 28 Oct 2018 11:12:48 +1300
Subject: [PATCH 3/3] s4/scripting/*: py3 compatible print
Signed-off-by: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
---
source4/scripting/bin/enablerecyclebin | 2 +-
source4/scripting/bin/findprovisionusnranges | 14 ++++++--------
source4/scripting/bin/fullschema | 2 +-
source4/scripting/bin/get-descriptors | 2 +-
source4/scripting/bin/minschema | 2 +-
source4/scripting/bin/mymachinepw | 7 +++----
source4/scripting/bin/rebuildextendeddn | 12 +++++++-----
source4/scripting/bin/sambaundoguididx | 2 +-
source4/scripting/bin/smbstatus | 10 ++++++----
source4/scripting/devel/addlotscontacts | 2 +-
source4/scripting/devel/config_base | 2 +-
source4/scripting/devel/crackname | 2 +-
source4/scripting/devel/enumprivs | 2 +-
source4/scripting/devel/getncchanges | 2 +-
14 files changed, 32 insertions(+), 31 deletions(-)
diff --git a/source4/scripting/bin/enablerecyclebin b/source4/scripting/bin/enablerecyclebin
index ab36ead1b8f..a179698b7fe 100755
--- a/source4/scripting/bin/enablerecyclebin
+++ b/source4/scripting/bin/enablerecyclebin
@@ -50,4 +50,4 @@ msg["enableOptionalFeature"] = ldb.MessageElement(
ldb.FLAG_MOD_ADD, "enableOptionalFeature")
res = sam_ldb.modify(msg)
-print "Recycle Bin feature enabled"
+print("Recycle Bin feature enabled")
diff --git a/source4/scripting/bin/findprovisionusnranges b/source4/scripting/bin/findprovisionusnranges
index ee9da3d421e..540869dbae4 100755
--- a/source4/scripting/bin/findprovisionusnranges
+++ b/source4/scripting/bin/findprovisionusnranges
@@ -18,7 +18,6 @@
# You should have received a copy of the GNU General Public License
# along with this program. If not, see <http://www.gnu.org/licenses/>.
#
-
import sys
import optparse
sys.path.insert(0, "bin/python")
@@ -63,18 +62,17 @@ if res and len(res) == 1 and res[0]["dsServiceName"] != None:
if res and len(res) == 1 and res[0]["invocationId"]:
invocation = str(ndr_unpack(misc.GUID, res[0]["invocationId"][0]))
else:
- print "Unable to find invocation ID"
+ print("Unable to find invocation ID")
sys.exit(1)
else:
- print "Unable to find attribute dsServiceName in rootDSE"
+ print("Unable to find attribute dsServiceName in rootDSE")
sys.exit(1)
minobj = 5
(hash_id, nb_obj) = findprovisionrange(samdb, basedn)
-print "Here is a list of changes that modified more than %d objects in 1 minute." % minobj
-print "Usually changes made by provision and upgradeprovision are those who affect a couple"\
- " of hundred of objects or more"
-print "Total number of objects: %d" % nb_obj
-print
+print("Here is a list of changes that modified more than %d objects in 1 minute." % minobj)
+print("Usually changes made by provision and upgradeprovision are those who affect a couple"
+ " of hundred of objects or more")
+print("Total number of objects: %d\n" % nb_obj)
print_provision_ranges(hash_id, minobj, opts.storedir, str(paths.samdb), invocation)
diff --git a/source4/scripting/bin/fullschema b/source4/scripting/bin/fullschema
index ab0e4e320bd..596de01b91c 100755
--- a/source4/scripting/bin/fullschema
+++ b/source4/scripting/bin/fullschema
@@ -132,7 +132,7 @@ def fix_dn(dn):
def write_ldif_one(o, attrs):
"""dump an object as ldif"""
- print "dn: CN=%s,${SCHEMADN}" % o["cn"]
+ print("dn: CN=%s,${SCHEMADN}" % o["cn"])
for a in attrs:
if not o.has_key(a):
continue
diff --git a/source4/scripting/bin/get-descriptors b/source4/scripting/bin/get-descriptors
index f1a919c3748..70926cdccbb 100755
--- a/source4/scripting/bin/get-descriptors
+++ b/source4/scripting/bin/get-descriptors
@@ -90,7 +90,7 @@ class DescrGetter:
for line in ldif_entry:
length = 79
if len(line) <= length + 1:
- print line
+ print(line)
else:
for i in range(len(line) / length + 1):
if i == 0:
diff --git a/source4/scripting/bin/minschema b/source4/scripting/bin/minschema
index 176ad5c93b9..eba198696e2 100755
--- a/source4/scripting/bin/minschema
+++ b/source4/scripting/bin/minschema
@@ -191,7 +191,7 @@ def fix_dn(dn):
def write_ldif_one(o, attrs):
"""dump an object as ldif"""
- print "dn: CN=%s,${SCHEMADN}" % o["cn"]
+ print("dn: CN=%s,${SCHEMADN}" % o["cn"])
for a in attrs:
if not o.has_key(a):
continue
diff --git a/source4/scripting/bin/mymachinepw b/source4/scripting/bin/mymachinepw
index dc85ad1df44..91dc502a634 100755
--- a/source4/scripting/bin/mymachinepw
+++ b/source4/scripting/bin/mymachinepw
@@ -19,7 +19,6 @@
# You should have received a copy of the GNU General Public License
# along with this program. If not, see <http://www.gnu.org/licenses/>.
#
-
import samba.param as param, ldb, sys, getopt
optlist, args = getopt.getopt(sys.argv[1:], "s:")
@@ -48,9 +47,9 @@ search = ("(&(objectclass=primaryDomain)(samaccountname=" +
msg = secrets.search(expression=search, attrs=['secret'])
if not msg:
- print "Error:"
- print "Password for host[%s] not found in path[%s]." % (netbios, path)
- print "You may want to pass the smb.conf location via the -s option."
+ print("Error:")
+ print("Password for host[%s] not found in path[%s]." % (netbios, path))
+ print("You may want to pass the smb.conf location via the -s option.")
exit(1)
password=msg[0]['secret'][0]
diff --git a/source4/scripting/bin/rebuildextendeddn b/source4/scripting/bin/rebuildextendeddn
index 5a0ab1295a6..5f5e05da795 100755
--- a/source4/scripting/bin/rebuildextendeddn
+++ b/source4/scripting/bin/rebuildextendeddn
@@ -21,7 +21,7 @@
# You should have received a copy of the GNU General Public License
# along with this program. If not, see <http://www.gnu.org/licenses/>.
#
-
+from __future__ import print_function
import optparse
import os
import sys
@@ -53,7 +53,7 @@ opts = parser.parse_args()[0]
def message(text):
"""print a message if quiet is not set."""
if not opts.quiet:
- print text
+ print(text)
if len(sys.argv) == 1:
opts.interactive = True
@@ -77,7 +77,7 @@ def get_paths(targetdir=None,smbconf=None):
smbconf = param.default_path()
if not os.path.exists(smbconf):
- print >>sys.stderr, "Unable to find smb.conf .. "+smbconf
+ print("Unable to find smb.conf .. "+smbconf, file=sys.stderr)
parser.print_usage()
sys.exit(1)
@@ -121,9 +121,11 @@ def rebuild_en_dn(credentials,session_info,paths):
sam_ldb.modify(m)
res3 = sam_ldb.search(expression="(&(distinguishedName=%s)(%s=*))"%(dn,att),scope=SCOPE_SUBTREE, attrs=[att],controls=["search_options:1:2"])
if( len(res3) == 0 or (len(res3[0][att])!= len(saveatt))):
- print >>sys.stderr, str(dn) + " has no attr " +att+ " or a wrong value"
+ print(str(dn) + " has no attr " +att+ " or a wrong value",
+ file=sys.stderr)
for satt in saveatt:
- print >>sys.stderr,str(att)+" = "+satt
+ print("%s = %s" % (att, satt),
+ file=sys.stderr)
sam_ldb.transaction_cancel()
sam_ldb.transaction_commit()
diff --git a/source4/scripting/bin/sambaundoguididx b/source4/scripting/bin/sambaundoguididx
index 24a95e20d7f..a931601cf03 100755
--- a/source4/scripting/bin/sambaundoguididx
+++ b/source4/scripting/bin/sambaundoguididx
@@ -71,7 +71,7 @@ for db in dbs:
samdb.transaction_commit()
-print "Re-opening with the full DB stack"
+print("Re-opening with the full DB stack")
samdb = SamDB(url=url,
lp=lp_ctx)
print "Re-triggering another re-index"
diff --git a/source4/scripting/bin/smbstatus b/source4/scripting/bin/smbstatus
index 473dbaf2ce4..c2834ab12ab 100755
--- a/source4/scripting/bin/smbstatus
+++ b/source4/scripting/bin/smbstatus
@@ -27,12 +27,14 @@ def show_sessions(conn):
"""show open sessions"""
sessions = next(conn.smbsrv_information(irpc.SMBSRV_INFO_SESSIONS))
- print "User Client Connected at"
- print "-" * 79
+ print("User Client Connected at")
+ print("-" * 79)
for session in sessions:
fulluser = "%s/%s" % (session.account_name, session.domain_name)
- print "%-30s %16s %s" % (fulluser, session.client_ip, sys.httptime(session.connect_time))
- print ""
+ print("%-30s %16s %s" % (fulluser,
+ session.client_ip,
+ sys.httptime(session.connect_time)))
+ print()
def show_tcons(open_connection):
"""show open tree connects"""
diff --git a/source4/scripting/devel/addlotscontacts b/source4/scripting/devel/addlotscontacts
index edf54b0bade..e8b2c1af1aa 100644
--- a/source4/scripting/devel/addlotscontacts
+++ b/source4/scripting/devel/addlotscontacts
@@ -75,7 +75,7 @@ if __name__ == '__main__':
ldbs.sam.add(msg)
- print "Creating %d contacts" % num_contacts
+ print("Creating %d contacts" % num_contacts)
count = 0
increment = num_contacts / 10
if increment > 5000:
diff --git a/source4/scripting/devel/config_base b/source4/scripting/devel/config_base
index 0d495c5091b..e74c87486bf 100755
--- a/source4/scripting/devel/config_base
+++ b/source4/scripting/devel/config_base
@@ -37,4 +37,4 @@ for v in vars:
options = options.replace("${PREFIX}", prefix)
-print options
+print(options)
diff --git a/source4/scripting/devel/crackname b/source4/scripting/devel/crackname
index 2e1798511f3..0ae177c133f 100755
--- a/source4/scripting/devel/crackname
+++ b/source4/scripting/devel/crackname
@@ -56,7 +56,7 @@ if __name__ == "__main__":
drs = drsuapi.drsuapi(binding_str, lp, creds)
drs_handle = do_DsBind(drs)
- print "DRS Handle: %s" % drs_handle
+ print("DRS Handle: %s" % drs_handle)
req = drsuapi.DsNameRequest1()
names = drsuapi.DsNameString()
diff --git a/source4/scripting/devel/enumprivs b/source4/scripting/devel/enumprivs
index 6a040402ae3..33597f9388a 100755
--- a/source4/scripting/devel/enumprivs
+++ b/source4/scripting/devel/enumprivs
@@ -55,4 +55,4 @@ if __name__ == "__main__":
(handle, privs) = lsaconn.EnumPrivs(pol_handle, 0, 100)
for p in privs.privs:
disp_name = get_display_name(lsaconn, pol_handle, p.name.string)
- print "0x%08x %31s \"%s\"" % (p.luid.low, p.name.string, disp_name)
+ print("0x%08x %31s \"%s\"" % (p.luid.low, p.name.string, disp_name))
diff --git a/source4/scripting/devel/getncchanges b/source4/scripting/devel/getncchanges
index 9b6361b3548..9c25e39f756 100755
--- a/source4/scripting/devel/getncchanges
+++ b/source4/scripting/devel/getncchanges
@@ -76,7 +76,7 @@ if __name__ == "__main__":
drs = drsuapi.drsuapi(binding_str, lp, creds)
drs_handle, supported_extensions = drs_DsBind(drs)
- print "DRS Handle: %s" % drs_handle
+ print("DRS Handle: %s" % drs_handle)
req8 = drsuapi.DsGetNCChangesRequest8()
--
2.17.1
More information about the samba-technical
mailing list