Ticket #2924 defect closed fixed

Opened 6 years ago

Last modified 6 years ago

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

1

Changed 6 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.

2

Changed 6 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.

3

Changed 6 years ago by therve

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

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

4

Changed 6 years ago by therve

  • priority changed from high to highest
  • keywords review added
  • owner z3p deleted

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

5

Changed 6 years ago by exarkun

bot-cube has gmpy available now

6

Changed 6 years ago by exarkun

  • owner set to exarkun
  • status changed from new to assigned

7

Changed 6 years ago by exarkun

  • owner changed from exarkun to therve
  • keywords review removed
  • 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.

8

Changed 6 years ago by therve

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

Refs #2924

9

Changed 6 years ago by therve

  • owner therve deleted
  • keywords review added

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 :).

10

Changed 6 years ago by exarkun

  • owner set to therve
  • keywords review removed

Looks good, please merge

11

Changed 6 years ago by therve

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

Refs #2924

12

Changed 6 years ago by therve

  • status changed from new to closed
  • resolution set to fixed

(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.

13

Changed 3 years ago by <automation>

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