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.

Sign up for GitHub

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

Jump to bottom

apps/speed: various cleanups and improvements #10078

Closed
wants to merge 9 commits into from
Closed

apps/speed: various cleanups and improvements #10078

wants to merge 9 commits into from

Conversation

FdaSilvaYY
Copy link
Contributor

@FdaSilvaYY FdaSilvaYY commented Oct 2, 2019 •
edited

  • replace bunchs of #define with enums.
  • load crypto material only when its algorithm is enabled.
  • improve error handling, avoid exit(1) when default provider is missing.
  • factorize some internal structures definitions for tested curves.
Checklist
  • documentation is added or updated
  • tests are added or updated

@FdaSilvaYY
Copy link
Contributor Author

FdaSilvaYY commented Oct 2, 2019 •
edited

Old staging branch on my sandbox.
Ping @paulidale : you may be interested in this
I'm not happy with a few changes in it.

paulidale
paulidale reviewed Oct 4, 2019
View reviewed changes
apps/speed.c Outdated
D_EVP, D_SHA256, D_SHA512, D_WHIRLPOOL,
D_IGE_128_AES, D_IGE_192_AES, D_IGE_256_AES,
D_GHASH, D_RAND, D_EVP_HMAC, D_EVP_CMAC
};
/* name of algorithms to test */
static const char *names[] = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the use of enums above, I'm more concerned about the disconnect between then and the string representations of the algs. This one was already there but some of the later ones remove the name, id pairings in favour of just having the name which means both lists need to be manually kept synchronised.

apps/speed.c Outdated Show resolved Hide resolved
apps/speed.c Outdated Show resolved Hide resolved
@FdaSilvaYY FdaSilvaYY force-pushed the apps/speed/various_cleanup branch 9 times, most recently from 9d0553c to 25d0665 Compare October 19, 2019 18:51
@FdaSilvaYY FdaSilvaYY marked this pull request as ready for review October 19, 2019 19:01
@FdaSilvaYY FdaSilvaYY changed the title apps/speed: various cleanups apps/speed: various cleanups and improvements Oct 19, 2019
@FdaSilvaYY
Copy link
Contributor Author

Ping @openssl : it is ready for review. 200 lines of code less.
I see many more places for improvement, I keep its for a next PR.

t8m
t8m requested changes Dec 2, 2019
View reviewed changes
apps/speed.c Show resolved Hide resolved
@t8m t8m added approval: otc review pending This pull request needs review by an OTC member branch: master Merge to master branch labels Dec 3, 2019
@t8m t8m self-assigned this Dec 3, 2019
t8m
t8m approved these changes Dec 3, 2019
View reviewed changes
Copy link
Member

@t8m t8m left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work.

levitte
levitte requested changes Dec 3, 2019
View reviewed changes
apps/speed.c Show resolved Hide resolved
@FdaSilvaYY FdaSilvaYY force-pushed the apps/speed/various_cleanup branch 2 times, most recently from 53682cd to 63d8fb1 Compare December 3, 2019 21:57
levitte
levitte reviewed Dec 3, 2019
View reviewed changes
apps/speed.c
{ test3072, sizeof(test3072), 3072 },
{ test4096, sizeof(test4096), 4092 },
{ test7680, sizeof(test7680), 7680 },
{ test15360, sizeof(test15360), 15360 }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! 😃

@levitte
Copy link
Member

levitte commented Dec 3, 2019

Travis failure relevant:

apps/speed.c:248:12: error: 'alg_found' defined but not used [-Werror=unused-function]
 static int alg_found(const char *name, unsigned int *result,
            ^
cc1: all warnings being treated as errors

@levitte
Copy link
Member

levitte commented Dec 3, 2019

Other than that, I like what I see now

@FdaSilvaYY
fixup! apps/speed: replace list of #define by enums declarations. it …
5964974 
…simplifies some pieces of code. Improve internal assertions Tag a few #endif with OPENSSL_NO_EC to mark its ending.
@levitte
Copy link
Member

levitte commented Dec 5, 2019

@t8m, if your approval still holds, will you change the labels?

@t8m t8m added approval: done This pull request has the required number of approvals approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: otc review pending This pull request needs review by an OTC member approval: done This pull request has the required number of approvals labels Dec 6, 2019
openssl-machine pushed a commit that referenced this pull request Dec 9, 2019
@FdaSilvaYY @t8m
apps/speed: allow to continue tests after any init failure handling.
af0857f 
previouly the exit(1) call was aborting the whole execution.
Improve error message.

Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
(Merged from #10078)
openssl-machine pushed a commit that referenced this pull request Dec 9, 2019
@FdaSilvaYY @t8m
apps/speed: discard useless protoypes as these methods are defines be…
2cc0765 
…fore being used.

Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
(Merged from #10078)
openssl-machine pushed a commit that referenced this pull request Dec 9, 2019
@FdaSilvaYY @t8m
apps/speed: remove a shared global variable
d02b7e0 
replace |save_count| by the right c[D_EVP(_xxx)] variable.
this may shared a value between various algorithm.

Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
(Merged from #10078)
openssl-machine pushed a commit that referenced this pull request Dec 9, 2019
@FdaSilvaYY @t8m
apps/speed: initialize key material only when its algo is selected.
0609658 
Remove some duplicate key data declarations.

Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
(Merged from #10078)
openssl-machine pushed a commit that referenced this pull request Dec 9, 2019
@FdaSilvaYY @t8m
apps/speed: simplify 'doit' action flag management code.
f607f6e 
Optimize algorithm selection code.

Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
(Merged from #10078)
openssl-machine pushed a commit that referenced this pull request Dec 9, 2019
@FdaSilvaYY @t8m
apps/speed: improve sm2 failure code.
001d5e2 
attach the new objects sooner, so error handling is simplified.

Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
(Merged from #10078)
openssl-machine pushed a commit that referenced this pull request Dec 9, 2019
@FdaSilvaYY @t8m
apps/speed: factorize ec test curves declarations
d63d89e 
remove 'test' prefix from variable names.

Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
(Merged from #10078)
openssl-machine pushed a commit that referenced this pull request Dec 9, 2019
@FdaSilvaYY @t8m
apps/speed: replace list of #define by enums declarations.
1352e0f 
it simplifies some pieces of code.
Improve internal assertions
Tag a few #endif with OPENSSL_NO_EC to mark its ending.

Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
(Merged from #10078)
@t8m
Copy link
Member

t8m commented Dec 9, 2019

Merged to master as: af0857f 2cc0765 d02b7e0 0609658 f607f6e 001d5e2 d63d89e 1352e0f

@t8m t8m closed this Dec 9, 2019
@FdaSilvaYY FdaSilvaYY deleted the apps/speed/various_cleanup branch December 10, 2019 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@levitte levitte levitte approved these changes

@t8m t8m t8m approved these changes

@paulidale paulidale Awaiting requested review from paulidale

Assignees

@t8m t8m

Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@FdaSilvaYY @levitte @t8m @paulidale