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

Change provider params from int to size_t #9699

Closed
wants to merge 9 commits into from

Conversation

@slontis
Copy link
Contributor

slontis commented Aug 27, 2019

Checklist
  • documentation is added or updated
  • tests are added or updated
@slontis slontis mentioned this pull request Aug 27, 2019
2 of 2 tasks complete
@slontis slontis added this to In progress in 3.0 New Core + FIPS via automation Aug 27, 2019
crypto/evp/evp_enc.c Outdated Show resolved Hide resolved
crypto/evp/evp_lib.c Outdated Show resolved Hide resolved
crypto/evp/evp_enc.c Show resolved Hide resolved
crypto/evp/evp_enc.c Outdated Show resolved Hide resolved
include/openssl/core_names.h Outdated Show resolved Hide resolved
providers/common/ciphers/cipher_common.c Outdated Show resolved Hide resolved
providers/common/ciphers/cipher_common.c Outdated Show resolved Hide resolved
providers/common/ciphers/cipher_gcm.c Outdated Show resolved Hide resolved
providers/common/ciphers/cipher_gcm.c Outdated Show resolved Hide resolved
@slontis

This comment has been minimized.

Copy link
Contributor Author

slontis commented Aug 28, 2019

ping

Copy link
Member

levitte left a comment

A whole bunch of nits. All of them are not so much about what types are compatible with each other, but what we express in code. Do we use size_t just to get an unsigned integer of a certain size, or to express that this value is intended to be used as a size? And while I'm at it, what's wrong with using bit fields for individual flags?

include/openssl/core_names.h Outdated Show resolved Hide resolved
include/openssl/mdc2.h Outdated Show resolved Hide resolved
providers/common/ciphers/cipher_common.c Outdated Show resolved Hide resolved
providers/common/ciphers/cipher_common.c Outdated Show resolved Hide resolved
providers/common/exchange/dh_exch.c Outdated Show resolved Hide resolved
providers/legacy/digests/mdc2_prov.c Outdated Show resolved Hide resolved
providers/legacy/digests/mdc2_prov.c Outdated Show resolved Hide resolved
test/mdc2test.c Outdated Show resolved Hide resolved
@slontis

This comment has been minimized.

Copy link
Contributor Author

slontis commented Sep 1, 2019

Changed pad_type to uint. Changed some boolean values to use bitfields. Change mode back to uint.

@slontis

This comment has been minimized.

Copy link
Contributor Author

slontis commented Sep 1, 2019

@levitte - hopefully this is better?

@slontis

This comment has been minimized.

Copy link
Contributor Author

slontis commented Sep 4, 2019

ping

@levitte

This comment has been minimized.

Copy link
Member

levitte commented Sep 4, 2019

Ok, looks generally good... I have one last thing [+] for your consideration: is OSSL_CIPHER_PARAM_NUM a size or index, semantically speaking? It's a counter, sure, but I wonder. I'm unsure what to decide and leave it to your judgement / justification.


[+] [ahem] the "last thing" is porentially "the thing before the last thing", I hope we're clear on that 😉

@slontis

This comment has been minimized.

Copy link
Contributor Author

slontis commented Sep 4, 2019

Changed to uint..

3.0 New Core + FIPS automation moved this from In progress to Reviewer approved Sep 4, 2019
@levitte
levitte approved these changes Sep 4, 2019
Copy link
Member

levitte left a comment

As long as the CIs agree

levitte pushed a commit that referenced this pull request Sep 5, 2019
Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from #9699)
@slontis

This comment has been minimized.

Copy link
Contributor Author

slontis commented Sep 5, 2019

Thanks. Merged to master.

@slontis slontis closed this Sep 5, 2019
3.0 New Core + FIPS automation moved this from Reviewer approved to Done Sep 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
4 participants
You can’t perform that action at this time.