Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

py2/py3 patches reviewed #185

Closed
wants to merge 16 commits into from
Closed

Conversation

abartlet
Copy link
Member

These are the py2/py3 patches I'm happy to have my review on.

See also:

https://gitlab.com/catalyst-samba/samba/pipelines/21849494
https://gitlab.com/catalyst-samba/samba/commits/abartlet-np-py3

Reviewed-by: Andrew Bartlett abartlet@samba.org

Signed-off-by: Noel Power <noel.power@suse.com>
Reviewed-by: Andrew Bartlett <abartlet@samba.org>
replace
    (foo, bar) = e
with
    (foo, bar) = e.args

while will run in with both python2 and python3

Signed-off-by: Noel Power <noel.power@suse.com>
Reviewed-by: Andrew Bartlett <abartlet@samba.org>
Fix some missed conversions of
        except Exception, e:
to
        except Exception as e:

Signed-off-by: Noel Power <noel.power@suse.com>
Reviewed-by: Andrew Bartlett <abartlet@samba.org>
Signed-off-by: Noel Power <noel.power@suse.com>
Reviewed-by: Andrew Bartlett <abartlet@samba.org>
Signed-off-by: Noel Power <noel.power@suse.com>
Reviewed-by: Andrew Bartlett <abartlet@samba.org>
To allow code run in both python3 and python2 we have to ensure
that md5 always receives bytes

Signed-off-by: Noel Power <noel.power@suse.com>
Reviewed-by: Andrew Bartlett <abartlet@samba.org>
Signed-off-by: Noel Power <noel.power@suse.com>
Reviewed-by: Andrew Bartlett <abartlet@samba.org>
Signed-off-by: Noel Power <noel.power@suse.com>
Reviewed-by: Andrew Bartlett <abartlet@samba.org>
Signed-off-by: Noel Power <noel.power@suse.com>
Reviewed-by: Andrew Bartlett <abartlet@samba.org>
…cted

Signed-off-by: Noel Power <noel.power@suse.com>
Reviewed-by: Andrew Bartlett <abartlet@samba.org>
…code

Signed-off-by: Noel Power <noel.power@suse.com>
Reviewed-by: Andrew Bartlett <abartlet@samba.org>
…encode

Signed-off-by: Noel Power <noel.power@suse.com>
Reviewed-by: Andrew Bartlett <abartlet@samba.org>
Signed-off-by: Noel Power <noel.power@suse.com>
Reviewed-by: Andrew Bartlett <abartlet@samba.org>
Signed-off-by: Noel Power <noel.power@suse.com>
Reviewed-by: Andrew Bartlett <abartlet@samba.org>
Signed-off-by: Noel Power <noel.power@suse.com>
Reviewed-by: Andrew Bartlett <abartlet@samba.org>
This seems to be a side affect of using StringIO.StringIO in python2.
We need to use StringIO.StringIO (instead of cStringIO.StringIO)

Note: https://docs.python.org/2/library/stringio.html states

"The StringIO object can accept either Unicode or 8-bit strings, but
 mixing the two may take some care. If both are used, 8-bit strings
that cannot be interpreted as 7-bit ASCII (that use the 8th bit) will
cause a UnicodeError to be raised when getvalue() is called"

and also about cStringIO

"Unlike the StringIO module, this module is not able to accept Unicode
strings that cannot be encoded as plain ASCII strings.

We don't use cStringIO because now we do have more unicode (from porting efforts
to make the code py2/py3 compatible). For sure some of these unicode string
cannot be encoded as plain ASCII.

running test 'samba.tests.dcerpc.dnsserver' (with python2)  results in
UnicodeDecodeError: 'ascii' codec can't decode byte 0xff in position 26: ordinal not in range(128)

Change the native string (as it is in python2) that contains a non ascii character
to unicode. (in a plain python3 only world this would not be an issue)

Signed-off-by: Noel Power <noel.power@suse.com>
Reviewed-by: Andrew Bartlett <abartlet@samba.org>
@abartlet
Copy link
Member Author

This is now in autobuild, except for the top patch around the DNS high byte (0xff). I want to think on that some more, we really want to force a high byte into the end record, so I'll check on this more with @GSam tomorrow. If a genuinely valid UTF8 sequence with a high byte will satisfy then we may wish to use that.

@abartlet abartlet mentioned this pull request May 12, 2018
@abartlet
Copy link
Member Author

The Cstring patch needs to be after or alongside the DNS patch I dropped, or else the build fails.

In general try it is best if the patches all pass make test (in theory, there isn't an automated check for this part) to aid a git bisect or dropping patches from the series like I have.

@abartlet
Copy link
Member Author

Also check if the Cstring thing is https://bugzilla.samba.org/show_bug.cgi?id=13435 and if so tag with that bug please!

@abartlet
Copy link
Member Author

Except for the CString patches, merged as 26c4084 in master for Samba 4.9

@abartlet abartlet closed this May 13, 2018
@noelpower
Copy link
Contributor

I don't believe this has anything to do with cString, looking into it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants