Opened 9 years ago

Closed 9 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
branch-diff, diff-cov, branch-cov, buildbot
Author: therve

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 9 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 9 years ago by Jean-Paul Calderone

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 9 years ago by therve

author: therve
Branch: branches/conch-gmpy-support-2924

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

comment:4 Changed 9 years ago by therve

Keywords: review added
Owner: z3p deleted
Priority: highhighest

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

comment:5 Changed 9 years ago by Jean-Paul Calderone

bot-cube has gmpy available now

comment:6 Changed 9 years ago by Jean-Paul Calderone

Owner: set to Jean-Paul Calderone
Status: newassigned

comment:7 Changed 9 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: changed from Jean-Paul Calderone to therve
Status: assignednew

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 9 years ago by therve

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

Refs #2924

comment:9 Changed 9 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 9 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to therve

Looks good, please merge

comment:11 Changed 9 years ago by therve

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

Refs #2924

comment:12 Changed 9 years ago by therve

Resolution: fixed
Status: newclosed

(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 6 years ago by <automation>

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