apps/speed: merge curve list definitions #6132
Conversation
|
Red cross from appveyor is from TLSProxy tests and caused by server resetting connection before alert went through "wire". This is unrelated. |
apps/speed.c
Outdated
| const char *name; | ||
| unsigned int nid; | ||
| unsigned int bits; | ||
| } curve_nameid_t; |
There was a problem hiding this comment.
It doesn't actually have to typedef, but ok...
apps/speed.c
Outdated
| /* | ||
| * We only test over the following curves as they are representative, To | ||
| * add tests over more curves, simply add the curve NID and curve name to | ||
| * the following arrays and increase the EC_NUM value accordingly. | ||
| */ | ||
| static const unsigned int test_curves[EC_NUM] = { | ||
| static const curve_nameid_t test_curves[EC_NUM] = |
There was a problem hiding this comment.
I wish it was possible to omit EC_NUM. But at least, while you are at it, would you declare this array as [], and insert OPENSSL_assert that compares number of elements to EC_NUM? So that it works as remainder reminder when you add things to it...
There was a problem hiding this comment.
Oups, will looking at this more carefully, I found a larger issue.
There is :
- 18 ecdh curves as EC_NUM states
- and only ecdsa 16 curves but it reuse EC_NUM value
Fix in progress => see #6133
FdaSilvaYY
force-pushed
the
apps/speed/merge_curve_lists
branch
2 times, most recently
from
0f4de48
to
5e820b7
Compare
|
Fixed and rebase; CI's are fully Green 🍾 |
apps/speed.c
Outdated
| unsigned int nid; | ||
| unsigned int bits; | ||
| } test_curves[] = | ||
| { |
There was a problem hiding this comment.
Hmm, I was under impression that preferred style would be to have opening { at same line. See for example ecp_nistp256.c...
There was a problem hiding this comment.
It's minor formatting nit, so I'm approving this provided that fix-up would be a separate commit that would be squashed upon commit to repo.
dot-asm
added
the
approval: review pending
FdaSilvaYY
force-pushed
the
apps/speed/merge_curve_lists
branch
from
5e820b7
to
e28f506
Compare
|
Hmmm... As procedural comment. "Provided that fix-up would be a separate commit" was actually condition that it was commit on top [of] prior commits, more specifically with their original ids. But now all commit |
levitte
added
approval: done
|
Editorial note. I'm going to squash everything into one commit. |
|
@dot-asm : sorry for the rebase, it was a reflex ;) |
|
Merged. Thanks! |
... and unify 'bits' declarations and printing format. Reviewed-by: Andy Polyakov <appro@openssl.org> Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from #6132)
It's a small code improvement: just merge the three list of values declaring each elliptic curve.
Edit : Second commit is because other
bitsare defined as unsigned except test_curves->bits.