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

Add a common function for security files. #51

Merged
merged 4 commits into from
Jun 28, 2021

Conversation

clalancette
Copy link
Contributor

All three of the in-tree DDS RMW implementation hard-code a
list of files in the security enclave. Instead of doing this
in three separate places, add in a common function that finds
the set of files in the security enclave that are needed for
DDS security.

get_security_files() currently returns them in an unordered_map.
The main reason to use a map over a structure is ABI concerns;
it should be easy to add another field to the unordered_map,
whereas adding one to a structure would be harder. (I intend
to add something to the map in the near future). It's also
not performance sensitive, so the extra string allocations
shouldn't hurt performance.

Signed-off-by: Chris Lalancette [email protected]

All three of the in-tree DDS RMW implementation hard-code a
list of files in the security enclave.  Instead of doing this
in three separate places, add in a common function that finds
the set of files in the security enclave that are needed for
DDS security.

get_security_files() currently returns them in an unordered_map.
The main reason to use a map over a structure is ABI concerns;
it should be easy to add another field to the unordered_map,
whereas adding one to a structure would be harder.  (I intend
to add something to the map in the near future).  It's also
not performance sensitive, so the extra string allocations
shouldn't hurt performance.

Signed-off-by: Chris Lalancette <[email protected]>
Copy link
Contributor

@hidmic hidmic left a comment

Choose a reason for hiding this comment

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

LGTM pending green CI !

rmw_dds_common/test/test_security.cpp Show resolved Hide resolved
Signed-off-by: Chris Lalancette <[email protected]>
@clalancette
Copy link
Contributor Author

Here's full CI of this PR and the ones in the individual RMWs:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@clalancette
Copy link
Contributor Author

Windows was pretty cranky, but I think I got it sorted out now. Here's another CI run:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@clalancette
Copy link
Contributor Author

There have been no substantial changes to this PR since the approval (just minor changes for Windows compatibility), so I'm going to go ahead and merge this one. Thanks!

@clalancette clalancette merged commit c67739d into master Jun 28, 2021
@clalancette clalancette deleted the clalancette/common-security-files branch June 28, 2021 13:12
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