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.

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

apps/speed.c: add -aead flag. #6311

Closed
wants to merge 9 commits into from
Closed

Conversation

dot-asm
Copy link
Contributor

@dot-asm dot-asm commented May 20, 2018

Goal is to exercise AEAD ciphers in TLS-like sequence, i.e. 13-byte
AAD followed by payload.

[While we are at it, address even some styling issues.]

Andy Polyakov added 2 commits May 20, 2018 11:34
Goal is to exercise AEAD ciphers in TLS-like sequence, i.e. 13-byte
AAD followed by payload.

[While we are at it, address even some styling issues.]
@dot-asm
Copy link
Contributor Author

dot-asm commented May 20, 2018

I had a brainslip, pushed update...

@dot-asm
Copy link
Contributor Author

dot-asm commented May 20, 2018

Red cross from travis is unrelated, and [should be] addressed in #6315.


if (EVP_CIPHER_mode(evp_cipher) == EVP_CIPH_CCM_MODE) {
loopfunc = EVP_Update_loop_ccm;
} else if (aead && (EVP_CIPHER_flags(evp_cipher) &
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not apply this to every cipher having EVP_CIPH_FLAG_AEAD_CIPHER flag ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I considered this. Trouble is that those who want to profile low-level subroutines would prefer the original behaviour. So there would be additional flag either way. Being conservative in minor release is more appropriate, i.e. it's more appropriate to engage new behaviour with additional flag and not vice versa.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I get it.

apps/speed.c Outdated
@@ -327,6 +328,8 @@ const OPTIONS speed_options[] = {
"Run benchmarks for pnum seconds"},
{"bytes", OPT_BYTES, 'p',
"Run cipher, digest and rand benchmarks on pnum bytes"},
{"aead", OPT_AEAD, '-',
Copy link
Contributor

Choose a reason for hiding this comment

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

this flag must be used in conjonction with -evp , isn't it ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not only that, but with cipher. I mean -evp can denote digest as well. Yeah, maybe sanity check with an error message would be appropriate...

Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest to take inspiration on the 'mb' option, a few lines above ::

     {"mb", OPT_MB, '-',
      "Enable (tls1.1) multi-block mode on evp_cipher requested with -evp"},

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Like that?

apps/speed.c Outdated
@@ -948,6 +951,41 @@ static int EVP_Update_loop_ccm(void *args)
}
return count;
}
/*
Copy link
Contributor

Choose a reason for hiding this comment

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

style : add a blank line to mark separation ?

Andy Polyakov added 2 commits May 21, 2018 17:07
Copy link
Contributor

@FdaSilvaYY FdaSilvaYY left a comment

Choose a reason for hiding this comment

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

nice job 👍

t-j-h
t-j-h previously approved these changes May 21, 2018
@dot-asm dot-asm dismissed t-j-h’s stale review May 27, 2018 08:08

I didmiss review as ping. I mean there were non-commentary updates past point of approval, and they wer not re-approved.

@FdaSilvaYY
Copy link
Contributor

@dot-asm this seems ready to be committed

dot-asm pushed a commit to dot-asm/openssl that referenced this pull request May 30, 2018
Reviewed-by: Tim Hudson <tjh@openssl.org>
(Merged from openssl#6311)
dot-asm pushed a commit to dot-asm/openssl that referenced this pull request May 30, 2018
Goal is to exercise AEAD ciphers in TLS-like sequence, i.e. 13-byte
AAD followed by payload. Update doc/man1/speed.pod accordingly.

[While we are at it, address even some styling and readability issues.]

Reviewed-by: Tim Hudson <tjh@openssl.org>
(Merged from openssl#6311)
dot-asm pushed a commit to dot-asm/openssl that referenced this pull request May 30, 2018
…ign.

Reviewed-by: Tim Hudson <tjh@openssl.org>
(Merged from openssl#6311)
@dot-asm dot-asm closed this May 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants