Opened 7 years ago

Closed 7 years ago

#2924 defect closed fixed (fixed)

conch gmpy support doesn't work anymore

Reported by: therve Owned by:
Priority: highest Milestone:
Component: conch Keywords:
Cc: Branch: branches/conch-gmpy-support-2924
(diff, github, buildbot, log)
Author: therve Launchpad Bug:

Description

This fails with this error:

  File "twisted-trunk/twisted/conch/ssh/common.py", line 105, in _fastgetMP
    mp.append(long(gmpy.mpz(data[c+4:c+l+4][::-1]+'\x00')))
exceptions.ValueError: string without NULL characters expected

Change History (13)

comment:1 Changed 7 years ago by therve

It comes from [21670]... It's not the first problem spotted since this merge, I wonder if it shouldn't be reverted.

comment:2 Changed 7 years ago by exarkun

One step in the right direction would be to get gmp onto at least one of the slaves. Then, there should be unit tests which run both versions of the code, even if gmp is installed.

comment:3 Changed 7 years ago by therve

  • author set to therve
  • Branch set to branches/conch-gmpy-support-2924

(In [21979]) Branching to 'conch-gmpy-support-2924'

comment:4 Changed 7 years ago by therve

  • Keywords review added
  • Owner z3p deleted
  • Priority changed from high to highest

OK this is ready to review. Although it's useless until a slave got gmpy installed.

comment:5 Changed 7 years ago by exarkun

bot-cube has gmpy available now

comment:6 Changed 7 years ago by exarkun

  • Owner set to exarkun
  • Status changed from new to assigned

comment:7 Changed 7 years ago by exarkun

  • Keywords review removed
  • Owner changed from exarkun to therve
  • Status changed from assigned to new

The indentation in MPTestCase.test_multipleGetMP is a bit funny. The 2nd line of the string literal should be indented two spaces fewer than it is, I think.

It might be nice to have a test for an integer > 255.

Otherwise, nice fix, thanks.

As far as reverting the original branch goes, I think we probably missed that opportunity. :( I'm guessing that untangling what has happened since will be more work than just fixing the problems we find. Sorry for not being more rigorous on that review.

comment:8 Changed 7 years ago by therve

(In [22121]) Fix indentation, add one test.

Refs #2924

comment:9 Changed 7 years ago by therve

  • Keywords review added
  • Owner therve deleted

Thanks for the review, I've made the modifications.

Regarding the revert, the branch was big enough so it wasn't very easy to make it land. Sometimes we just have to go forward :).

comment:10 Changed 7 years ago by exarkun

  • Keywords review removed
  • Owner set to therve

Looks good, please merge

comment:11 Changed 7 years ago by therve

(In [22122]) Skip the tests when pyCrypto is not there.

Refs #2924

comment:12 Changed 7 years ago by therve

  • Resolution set to fixed
  • Status changed from new to closed

(In [22123]) Merge conch-gmpy-support-2924

Author: therve
Reviewer: exarkun
Fixes #2924

Make gmpy support in conch works again, and add tests for both standard and fast
implementation of multiple precision. It was broken in r21670.

comment:13 Changed 3 years ago by <automation>

  • Owner therve deleted
Note: See TracTickets for help on using tickets.