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

Fix conch MSG_DEBUG parsing on Python 2 #998

Merged
merged 2 commits into from Apr 15, 2018

Conversation

Projects
None yet
2 participants
@cjwatson
Copy link

cjwatson commented Apr 14, 2018

bool(b'\x00...'[0]) returns True, which isn't what's needed here.

Contributor Checklist:

Colin Watson
Fix conch MSG_DEBUG parsing on Python 2
bool(b'\x00...'[0]) returns True, which isn't what's needed here.
@codecov

This comment has been minimized.

Copy link

codecov bot commented Apr 14, 2018

Codecov Report

Merging #998 into trunk will increase coverage by 0.03%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##            trunk     #998      +/-   ##
==========================================
+ Coverage   91.79%   91.82%   +0.03%     
==========================================
  Files         842      842              
  Lines      149569   150263     +694     
  Branches    13090    13193     +103     
==========================================
+ Hits       137294   137986     +692     
+ Misses      10178    10177       -1     
- Partials     2097     2100       +3
@markrwilliams
Copy link
Member

markrwilliams left a comment

👍 Thanks so much for improving Conch (on Python 2, too!)

@@ -967,7 +967,7 @@ def ssh_DEBUG(self, packet):
@type packet: L{bytes}
@param packet: The message data.
"""
alwaysDisplay = bool(packet[0])

This comment has been minimized.

Copy link
@markrwilliams

markrwilliams Apr 15, 2018

Member

On Python 3 this was correct because indexing a bytes object results in the integer value for that index:

>>> b'\x00'[0], b'\x01'[0]
(0, 1)

On Python 2, however, bytes is str, so indexing a bytes object returns another bytes object:

>>> b'\x00'[0], b'\x01'[0]
('\x00', '\x01')

Because items in the returned tuple are truthy this always evaluated to True on Python 2. Whoops!

@@ -967,7 +967,7 @@ def ssh_DEBUG(self, packet):
@type packet: L{bytes}
@param packet: The message data.
"""
alwaysDisplay = bool(packet[0])
alwaysDisplay = bool(ord(packet[0:1]))

This comment has been minimized.

Copy link
@markrwilliams

markrwilliams Apr 15, 2018

Member

This is correct on both Python 2 and 3 because both return bytes for slices of bytes, and ord returns the ASCII integral value for single-item bytes objects on Python 3.

@markrwilliams

This comment has been minimized.

Copy link
Member

markrwilliams commented Apr 15, 2018

I've pushed this to a branch of https://github.com/twisted/twisted so the Buildbot builders will run it. It can be merged after the builds are green (this sometimes takes a few tries due to some unreliable tests.)

@markrwilliams markrwilliams merged commit 22bc980 into twisted:trunk Apr 15, 2018

30 checks passed

buildbot/documentation Buildbot test done.
Details
buildbot/fedora26-py2.7 Buildbot test done.
Details
buildbot/fedora26-py2.7-coverage Buildbot test done.
Details
buildbot/fedora26-py3.6 Buildbot test done.
Details
buildbot/fedora26-py3.6-coverage Buildbot test done.
Details
buildbot/fedora27-py2.7 Buildbot test done.
Details
buildbot/fedora27-py2.7-coverage Buildbot test done.
Details
buildbot/fedora27-py3.6 Buildbot test done.
Details
buildbot/fedora27-py3.6-coverage Buildbot test done.
Details
buildbot/osx10.11-py2.7-coverage Buildbot test done.
Details
buildbot/rhel7-py2.7 Buildbot test done.
Details
buildbot/rhel7-py2.7-coverage Buildbot test done.
Details
buildbot/ubuntu14.04-py2.7 Buildbot test done.
Details
buildbot/ubuntu16.04-py2.7 Buildbot test done.
Details
buildbot/ubuntu16.04-py2.7-coverage Buildbot test done.
Details
buildbot/ubuntu16.04-py2.7-newstyle-coverage Buildbot test done.
Details
buildbot/ubuntu16.04-py2.7-nodeps Buildbot test done.
Details
buildbot/ubuntu16.04-py2.7-nodeps-coverage Buildbot test done.
Details
buildbot/ubuntu16.04-py3.5 Buildbot test done.
Details
buildbot/ubuntu16.04-py3.5-asyncio-coverage Buildbot test done.
Details
buildbot/ubuntu16.04-py3.5-coverage Buildbot test done.
Details
buildbot/windows7-64-py2.7 Buildbot test done.
Details
buildbot/windows7-64-py2.7-coverage Buildbot test done.
Details
buildbot/windows7-64-py2.7-wheel Buildbot test done.
Details
ci/circleci: documentation Your tests passed on CircleCI!
Details
ci/circleci: pyflakes3 Your tests passed on CircleCI!
Details
ci/circleci: static_checkers Your tests passed on CircleCI!
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@cjwatson cjwatson deleted the cjwatson:9422-conch-debug-always-display branch Apr 16, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.