[SSSD] [PATCHES] SSSDConfig: Port missing parts to python3

Lukas Slebodnik lslebodn at redhat.com
Thu Jan 29 15:39:01 UTC 2015


On (29/01/15 13:09), Petr Viktorin wrote:
>On 01/29/2015 01:05 PM, Bohuslav Kabrda wrote:
>>----- Original Message -----
>>>ehlo,
>>>
>>>some parts of sssd was not properly ported to python3.
>>>I know there were changes related to unicode, string and bytes.
>>>I am not sure whether my patches for read and open are correct,
>>>especially patch "SSSDConfig: os.write".
>>>
>>>SSSDConfig (python-sssdconfig is used by authconfig and ipa-client-install?
>>>So I don't want to break it.
>>>
>>>Please review patches or propose better version.
>>>
>>>LS
>>
>>Not sure why I missed so many things in my original patch (maybe I searched
>> just for *.py, not *.py.in ;)). Anyway:
and IIRC sbus_codegen was not in git when you sent patches.

>>- Patch 1 LGTM, I only don't understand why you need to import "exceptions"
>>module (and why only in Python 2). Note that Python 2 documentation says
>>that "This module never needs to be imported explicitly". [1]
It might have been used due to older version of python (RHEL5),
which we do not support any more.
I removed it.

>>- Patch 2 LGMT, but I think you could just write:
>>
>>'long': long if sys.version_info[0] == 2 else int
>>
That's nicer.

>>(but that's just nitpicking...)
>>- Patch 3: this depends. Is the object that you're writing binary or text?
>>I think what you want to do is keep "wb" mode and encode output to a certain
>>encoding (utf8?). So this could be:
>>
>>of = open(outputfile, "wb")
>>output = self.dump(self.opts).encode("utf-8")
>>of.write(output)
Output file should be valid sssd configuration file which if text file.
I used your proposal.

>>The problem is that the "str" type in Python 2 is sort of polymorphic, in
>>that it can behave both as binary bytestring and as unicode - but .encode()
>>works on it nonetheless. In Python 3 you have either "bytes" (binary string)
>>or "str" (unicode text string) and you have to choose which one you want
>>to write ("wb" mode is for "bytes", while "w" is for "str").
>>If "self.dump(self.opts)" returns "str" in Python 3, then I suggest you use
>>'.encode("utf-8")' on it and leave the mode "wb".
>>This should keep working in Python 2.
>>- Patch 4 LGTM
>>- Patch 5 LGTM
>>
>>Slavek
>>
>>[1] https://docs.python.org/2/library/exceptions.html#module-exceptions
>>
>
>Patch 1/5: Note that even under Python 2, `except ValueError, KeyError:`
>would not work (it catches ValueError, and stores the exception instance
>under the name KeyError); please test these changes well.
>
It means that in python2 KeyError was not handled at all.

python2 documentation says:
"If an exception occurs which does not match the exception named in the except
clause, it is passed on to outer try statements; if no handler is found, it is
an unhandled exception and execution stops with a message as shown above."

In my patch, you can see that different exception was thrown (ParsingError) in
handler. I'm not sure hot to test it therefore you are in CC ;-)
Shall I remove KeyError?

>Patch 4/5: you can use io.StringIO in both 2.6+ and 3.x+
>
Yes, but...
Traceback (most recent call last):
  File "../src/sbus/sbus_codegen", line 931, in <module>
    main()
  File "../src/sbus/sbus_codegen", line 919, in main
    generate_source(parser.parsed_interfaces, filename, options.include)
  File "../src/sbus/sbus_codegen", line 600, in generate_source
    out("/* The following definitions are auto-generated from %s */", basename)
  File "../src/sbus/sbus_codegen", line 217, in out
    sys.stdout.write(str)
TypeError: unicode argument expected, got 'str'

So I'm find with such solution for StringIO. The sbus_codegen is python script
which generate C source file. I do not expect any unicode characters.
But I'm not opposed better version.

Thank you very much for review.
I squashed some small patches.

LS
-------------- next part --------------
>From ec7d5ddae385dcc35c80dc0b77d7c31a4bb710c7 Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik <lslebodn at redhat.com>
Date: Thu, 29 Jan 2015 09:46:27 +0100
Subject: [PATCH 1/2] SSSDConfig: Port missing parts to python3

* fix incompatible imports
* fix translation.[u]?gettext
* fix dict method has_key
* fix catching multiple exception classes
* fix octal literals PEP 3127
* long is not defined in python3

Resolves:
https://fedorahosted.org/sssd/ticket/2017
---
 src/config/SSSDConfig/__init__.py.in | 47 +++++++++++++++++++-----------------
 src/config/SSSDConfigTest.py         | 24 +++++++++---------
 2 files changed, 37 insertions(+), 34 deletions(-)

diff --git a/src/config/SSSDConfig/__init__.py.in b/src/config/SSSDConfig/__init__.py.in
index ae00a2b7f9130725a6a766a4cbbba0a53f86dd7a..de03bd6140689c3935d4affcf5fe365b7bc521e7 100644
--- a/src/config/SSSDConfig/__init__.py.in
+++ b/src/config/SSSDConfig/__init__.py.in
@@ -6,9 +6,9 @@ Created on Sep 18, 2009
 
 import os
 import gettext
-import exceptions
 import re
-from ipachangeconf import SSSDChangeConf
+import sys
+from .ipachangeconf import SSSDChangeConf
 
 # Exceptions
 class SSSDConfigException(Exception): pass
@@ -32,7 +32,10 @@ PACKAGE = 'sss_daemon'
 LOCALEDIR = '/usr/share/locale'
 
 translation = gettext.translation(PACKAGE, LOCALEDIR, fallback=True)
-_ = translation.ugettext
+if sys.version_info[0] > 2:
+    _ = translation.gettext
+else:
+    _ = translation.ugettext
 
 # TODO: This needs to be made external
 option_strings = {
@@ -446,7 +449,7 @@ class SSSDConfigSchema(SSSDChangeConf):
         self.type_lookup = {
             'bool' : bool,
             'int'  : int,
-            'long' : long,
+            'long' : long if sys.version_info[0] == 2 else int,
             'float': float,
             'str'  : str,
             'list' : list,
@@ -481,7 +484,7 @@ class SSSDConfigSchema(SSSDChangeConf):
             subtype = self.type_lookup[split_option[SUBTYPE]]
             mandatory = self.bool_lookup[split_option[MANDATORY]]
 
-            if option_strings.has_key(option['name']):
+            if option['name'] in option_strings:
                 desc = option_strings[option['name']]
             else:
                 desc = None
@@ -527,7 +530,7 @@ class SSSDConfigSchema(SSSDChangeConf):
                                      mandatory,
                                      desc,
                                      [subtype(split_option[DEFAULT])])
-                        except ValueError, KeyError:
+                        except (ValueError, KeyError):
                             raise ParsingError
                 else:
                     try:
@@ -546,7 +549,7 @@ class SSSDConfigSchema(SSSDChangeConf):
                                  mandatory,
                                  desc,
                                  primarytype(split_option[DEFAULT]))
-                    except ValueError, KeyError:
+                    except (ValueError, KeyError):
                         raise ParsingError
 
             elif optionlen > 4:
@@ -561,7 +564,7 @@ class SSSDConfigSchema(SSSDChangeConf):
                             else:
                                 newvalue = subtype(x)
                             fixed_options.extend([newvalue])
-                        except ValueError, KeyError:
+                        except (ValueError, KeyError):
                             raise ParsingError
                     else:
                         fixed_options.extend([x])
@@ -610,7 +613,7 @@ class SSSDConfigSchema(SSSDChangeConf):
             splitsection = section['name'].split('/')
             if (splitsection[0] == 'provider'):
                 if(len(splitsection) == 3):
-                    if not providers.has_key(splitsection[1]):
+                    if splitsection[1] not in providers:
                         providers[splitsection[1]] = []
                     providers[splitsection[1]].extend([splitsection[2]])
         for key in providers.keys():
@@ -674,7 +677,7 @@ class SSSDConfigObject(object):
         === Errors ===
         No errors
         """
-        if self.options.has_key(optionname):
+        if optionname in self.options:
             del self.options[optionname]
 
 class SSSDService(SSSDConfigObject):
@@ -1309,12 +1312,12 @@ class SSSDDomain(SSSDConfigObject):
         # We should now have a list of options used only by this
         # provider. So we remove them.
         for option in options:
-            if self.options.has_key(option):
+            if option in self.options:
                 del self.options[option]
 
         # Remove this provider from the option list
         option = '%s_provider' % provider_type
-        if self.options.has_key(option):
+        if option in self.options:
             del self.options[option]
 
         self.providers.remove((provider, provider_type))
@@ -1452,10 +1455,10 @@ class SSSDConfig(SSSDChangeConf):
             outputfile = self.configfile
 
         # open() will raise IOError if it fails
-        old_umask = os.umask(0177)
+        old_umask = os.umask(0o177)
         of = open(outputfile, "wb")
         output = self.dump(self.opts)
-        of.write(output)
+        of.write(output.encode('utf-8'))
         of.close()
         os.umask(old_umask)
 
@@ -1477,7 +1480,7 @@ class SSSDConfig(SSSDChangeConf):
         if (self.has_option('sssd', 'services')):
             active_services = striplist(self.get('sssd', 'services').split(','))
             service_dict = dict.fromkeys(active_services)
-            if service_dict.has_key(''):
+            if '' in service_dict:
                 del service_dict['']
 
             # Remove any entries in this list that don't
@@ -1633,7 +1636,7 @@ class SSSDConfig(SSSDChangeConf):
         # This guarantees uniqueness and makes it easy
         # to add a new value
         service_dict = dict.fromkeys(striplist(item['value'].split(',')))
-        if service_dict.has_key(''):
+        if '' in service_dict:
             del service_dict['']
 
         # Add a new key for the service being activated
@@ -1674,11 +1677,11 @@ class SSSDConfig(SSSDChangeConf):
         # This guarantees uniqueness and makes it easy
         # to remove the one unwanted value.
         service_dict = dict.fromkeys(striplist(item['value'].split(',')))
-        if service_dict.has_key(''):
+        if '' in service_dict:
             del service_dict['']
 
         # Remove the unwanted service from the lest
-        if service_dict.has_key(name):
+        if name in service_dict:
             del service_dict[name]
 
         # Write out the joined keys
@@ -1760,7 +1763,7 @@ class SSSDConfig(SSSDChangeConf):
         if (self.has_option('sssd', 'domains')):
             active_domains = striplist(self.get('sssd', 'domains').split(','))
             domain_dict = dict.fromkeys(active_domains)
-            if domain_dict.has_key(''):
+            if '' in domain_dict:
                 del domain_dict['']
 
             # Remove any entries in this list that don't
@@ -1955,7 +1958,7 @@ class SSSDConfig(SSSDChangeConf):
         # This guarantees uniqueness and makes it easy
         # to add a new value
         domain_dict = dict.fromkeys(striplist(item['value'].split(',')))
-        if domain_dict.has_key(''):
+        if '' in domain_dict:
             del domain_dict['']
 
         # Add a new key for the domain being activated
@@ -1996,11 +1999,11 @@ class SSSDConfig(SSSDChangeConf):
         # This guarantees uniqueness and makes it easy
         # to remove the one unwanted value.
         domain_dict = dict.fromkeys(striplist(item['value'].split(',')))
-        if domain_dict.has_key(''):
+        if '' in domain_dict:
             del domain_dict['']
 
         # Remove the unwanted domain from the lest
-        if domain_dict.has_key(name):
+        if name in domain_dict:
             del domain_dict[name]
 
         # Write out the joined keys
diff --git a/src/config/SSSDConfigTest.py b/src/config/SSSDConfigTest.py
index 5d6662a9ad5d27280bb3f48e94cc0fb071665fd6..4c4af18d35232362d664480912a9cece01ec8a63 100755
--- a/src/config/SSSDConfigTest.py
+++ b/src/config/SSSDConfigTest.py
@@ -748,12 +748,12 @@ class SSSDConfigTestSSSDDomain(unittest.TestCase):
         # Ensure that all of the expected defaults are there
         for provider in control_provider_dict.keys():
             for ptype in control_provider_dict[provider]:
-                self.assertTrue(providers.has_key(provider))
+                self.assertTrue(provider in providers)
                 self.assertTrue(ptype in providers[provider])
 
         for provider in providers.keys():
             for ptype in providers[provider]:
-                self.assertTrue(control_provider_dict.has_key(provider))
+                self.assertTrue(provider in control_provider_dict)
                 self.assertTrue(ptype in control_provider_dict[provider])
 
     def testListProviderOptions(self):
@@ -1003,7 +1003,7 @@ class SSSDConfigTestSSSDDomain(unittest.TestCase):
         # Remove the local ID provider and add an LDAP one
         # LDAP ID providers can also use the krb5_realm
         domain.remove_provider('id')
-        self.assertFalse(domain.options.has_key('id_provider'))
+        self.assertFalse('id_provider' in domain.options)
 
         domain.add_provider('ldap', 'id')
 
@@ -1020,7 +1020,7 @@ class SSSDConfigTestSSSDDomain(unittest.TestCase):
         domain.remove_provider('id')
         self.assertEquals(domain.get_option('krb5_realm'),
                   'EXAMPLE.COM')
-        self.assertFalse(domain.options.has_key('ldap_uri'))
+        self.assertFalse('ldap_uri' in domain.options)
 
         # Put the LOCAL provider back
         domain.add_provider('local', 'id')
@@ -1028,7 +1028,7 @@ class SSSDConfigTestSSSDDomain(unittest.TestCase):
         # Remove the auth domain and verify that the options
         # revert to the backup_list
         domain.remove_provider('auth')
-        self.assertFalse(domain.options.has_key('auth_provider'))
+        self.assertFalse('auth_provider' in domain.options)
         options = domain.list_options()
 
         self.assertTrue(type(options) == dict,
@@ -1047,21 +1047,21 @@ class SSSDConfigTestSSSDDomain(unittest.TestCase):
                             option)
 
         # Ensure that the krb5_realm option is now gone
-        self.assertFalse(domain.options.has_key('krb5_realm'))
+        self.assertFalse('krb5_realm' in domain.options)
 
         # Test removing nonexistent provider - Real
         domain.remove_provider('id')
-        self.assertFalse(domain.options.has_key('id_provider'))
+        self.assertFalse('id_provider' in domain.options)
 
         # Test removing nonexistent provider - Bad backend type
         # Should pass without complaint
         domain.remove_provider('id')
-        self.assertFalse(domain.options.has_key('id_provider'))
+        self.assertFalse('id_provider' in domain.options)
 
         # Test removing nonexistent provider - Bad provider type
         # Should pass without complaint
         domain.remove_provider('nosuchprovider')
-        self.assertFalse(domain.options.has_key('nosuchprovider_provider'))
+        self.assertFalse('nosuchprovider_provider' in domain.options)
 
     def testGetOption(self):
         domain = SSSDConfig.SSSDDomain('sssd', self.schema)
@@ -1367,7 +1367,7 @@ class SSSDConfigTestSSSDConfig(unittest.TestCase):
         # Positive test - Service with invalid option loads
         # but ignores the invalid option
         service = sssdconfig.get_service('pam')
-        self.assertFalse(service.options.has_key('nosuchoption'))
+        self.assertFalse('nosuchoption' in service.options)
 
     def testNewService(self):
         sssdconfig = SSSDConfig.SSSDConfig(srcdir + "/etc/sssd.api.conf",
@@ -1598,13 +1598,13 @@ class SSSDConfigTestSSSDConfig(unittest.TestCase):
         # Expected result: Domain is imported, but does not contain the
         # unknown provider entry
         domain = sssdconfig.get_domain('INVALIDPROVIDER')
-        self.assertFalse(domain.options.has_key('chpass_provider'))
+        self.assertFalse('chpass_provider' in domain.options)
 
         # Positive Test - Domain with unknown option
         # Expected result: Domain is imported, but does not contain the
         # unknown option entry
         domain = sssdconfig.get_domain('INVALIDOPTION')
-        self.assertFalse(domain.options.has_key('nosuchoption'))
+        self.assertFalse('nosuchoption' in domain.options)
 
     def testNewDomain(self):
         sssdconfig = SSSDConfig.SSSDConfig(srcdir + "/etc/sssd.api.conf",
-- 
2.1.0

-------------- next part --------------
>From 869274e06376a2fa01c3467515654da8ea42a4a8 Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik <lslebodn at redhat.com>
Date: Thu, 29 Jan 2015 10:32:23 +0100
Subject: [PATCH 2/2] sbus_codegen: Port to python3

Resolves:
https://fedorahosted.org/sssd/ticket/2017
---
 src/sbus/sbus_codegen | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/src/sbus/sbus_codegen b/src/sbus/sbus_codegen
index d2de58552b8de5bc6178e7a895d3494515680112..ceaf26c82515ad2741d308e8a7769e09a367ac6e 100755
--- a/src/sbus/sbus_codegen
+++ b/src/sbus/sbus_codegen
@@ -60,14 +60,19 @@
 #    to generate for a given interface or method. By default the codegen will
 #    build up a symbol name from the DBus name.
 #
+from __future__ import print_function
 
 import optparse
 import os
 import re
-import StringIO
 import sys
 import xml.parsers.expat
 
+if sys.version_info[0] > 2:
+    import io
+else:
+    import StringIO as io
+
 # -----------------------------------------------------------------------------
 # Objects
 
@@ -765,13 +770,13 @@ class DBusXMLParser:
         self.arg_count = 0
 
         try:
-            with open(filename, "r") as f:
+            with open(filename, "rb") as f:
                 parser.ParseFile(f)
-        except DBusXmlException, ex:
+        except DBusXmlException as ex:
             ex.line = parser.CurrentLineNumber
             ex.file = filename
             raise
-        except xml.parsers.expat.ExpatError, ex:
+        except xml.parsers.expat.ExpatError as ex:
             exc = DBusXmlException(str(ex))
             exc.line = ex.lineno
             exc.file = filename
@@ -895,11 +900,11 @@ def parse_options():
     (options, args) = parser.parse_args()
 
     if not args:
-        print >> sys.stderr, "sbus_codegen: no input file specified"
+        print("sbus_codegen: no input file specified", file=sys.stderr)
         sys.exit(2)
 
     if options.mode not in ["header", "source"]:
-        print >> sys.stderr, "sbus_codegen: specify --mode=header or --mode=source"
+        print("sbus_codegen: specify --mode=header or --mode=source", file=sys.stderr)
 
     return options, args
 
@@ -907,7 +912,7 @@ def main():
     options, args = parse_options()
 
     if options.output:
-        sys.stdout = buf = StringIO.StringIO()
+        sys.stdout = buf = io.StringIO()
 
     for filename in args:
         parser = DBusXMLParser(filename)
@@ -928,6 +933,6 @@ def main():
 if __name__ == "__main__":
     try:
         main()
-    except DBusXmlException, ex:
-        print >> sys.stderr, str(ex)
+    except DBusXmlException as ex:
+        print(str(ex), file=sys.stderr)
         sys.exit(1)
-- 
2.1.0



More information about the sssd-devel mailing list