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

Move to user_settings and WOLFHSM_CFG_*. #52

Merged
merged 17 commits into from
Jul 23, 2024

Conversation

billphipps
Copy link
Contributor

Further isolation of compile-time configuration values and moving to using user_settings.h to be included in those headers that support reconfiguration (wh_client.h, wh_server.h, wh_nvm.h, wh_she_common.h).

A number of additional typos and minor doc edits as well.

@billphipps billphipps force-pushed the nvm_constantname_fixes branch from c89b999 to 430ff58 Compare July 19, 2024 18:12
bigbrett
bigbrett previously approved these changes Jul 22, 2024
Copy link
Contributor

@bigbrett bigbrett left a comment

Choose a reason for hiding this comment

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

A few random things I noticed but good to merge as is.

Something that I do want to discuss though (also mentioned inline in a comment below):

  • I don't think we want to establish the paradigm of putting wolfHSM configuration defines in a wolfSSL user_settings.h
  • We should retain a separate user configuration header for wolfHSM, as supplying defines on CLI would get unwieldy.

What was wrong with wh_config.h?

Comment on lines 124 to 125
#ifndef WOLFHSM_CFG_SERVER_DMAADDR_COUNT
#define WOLFHSM_CFG_SERVER_DMAADDR_COUNT 10
Copy link
Contributor

Choose a reason for hiding this comment

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

small opinion/nit, feel free to disregard: I kind of like keeping the "allow" word in these defines, because it really indicates the number of addresses in the allowlist.

Copy link
Contributor

@bigbrett bigbrett left a comment

Choose a reason for hiding this comment

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

LGTM nice work. I'm wondering if we want to be including wh_settings.h in every header when it isn't needed externally - think it would be best to keep that inclusion in the *.c file and only export it if it is needed for the external API. But that is not a critical item.

@billphipps billphipps merged commit 39343e3 into wolfSSL:main Jul 23, 2024
1 check passed
@billphipps
Copy link
Contributor Author

---I'm wondering if we want to be including wh_settings.h in every header when it isn't needed externally

I decided to err on the side of caution. If EVERY .h file in the base library includes wh_settings.h first, then IF some change later needs a new CFG value, it will already be there. This also means if a customer includes a random wh_XXX.h file, we are assured those configs will be included.

@billphipps billphipps deleted the nvm_constantname_fixes branch August 12, 2024 18:28
jefferyq2 pushed a commit to jefferyq2/wolfHSM that referenced this pull request Nov 10, 2024
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.

2 participants