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

refactor: clean up other hex methods #4664

Merged
merged 3 commits into from
Jul 27, 2024
Merged

Conversation

lrstewart
Copy link
Contributor

@lrstewart lrstewart commented Jul 24, 2024

Resolved issues:

resolves #2759

Description of changes:

  • s2n_key_log_hex_encode: I removed this method. It could be easily replaced with s2n_stuffer_write_hex.
  • s2n_hex_string_to_bytes: This was only used in tests, so I added a new test wrapper around s2n_stuffer_read_hex called "s2n_blob_alloc_from_hex_with_whitespace".
    • I also moved the related macros to s2n_testlib.h. And instead of adding a third "S2N_BLOB_FROM_HEX" variation that used EXPECT_OK, I added a variation that can work for any return type.
  • s2n_str_hex_to_bytes: I left this one be. It's used in s2nc/s2nd, which isn't supposed to use internal code. Technically we probably need a separate implementation in s2nc/s2nd.

I also cleaned up the existing s2n_stuffer_alloc_ro_from_hex_string so it better matched the blob version I added.

Call-outs:

This change touches a lot of files, but most of the changes are just renames / return type changes. If the PR is hard to review, I can make some of those changes in a follow-up PR.

Testing:

Existing tests pass. I moved the interesting tests of s2n_blob_alloc_from_hex_with_whitespace (mostly meaning the ones with whitespace) into a different test file.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@lrstewart lrstewart marked this pull request as ready for review July 24, 2024 07:49
Copy link
Contributor

@maddeleine maddeleine left a comment

Choose a reason for hiding this comment

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

Yay cleanups!

return S2N_RESULT_OK;
}

/* Unlike other hex methods, the hex string read here may include spaces.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Move this comment to the header file so that my IDE picks it up whenever I mouse over the method?

@lrstewart lrstewart enabled auto-merge (squash) July 27, 2024 19:33
@lrstewart lrstewart merged commit b483357 into aws:main Jul 27, 2024
34 checks passed
@lrstewart lrstewart deleted the hex_replace branch July 27, 2024 21:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cleanup and provide only one underlying hex encoding/decoding implementation
3 participants