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

Ensure compliant init options API implementations. #200

Merged
merged 2 commits into from
Jun 24, 2020

Conversation

hidmic
Copy link
Contributor

@hidmic hidmic commented Jun 23, 2020

Complying with ros2/rmw#244.

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic
Copy link
Contributor Author

hidmic commented Jun 23, 2020

CI up to rcl and test_rmw_implementation against all RMW implementations:

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


rmw_ret_t ret = rmw_security_options_fini(&init_options->security_options, allocator);
if (RMW_RET_OK == ret) {
allocator->deallocate(init_options->enclave, allocator->state);
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Collaborator

@eboasson eboasson left a comment

Choose a reason for hiding this comment

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

Looks fine to me, the only thing that I find debatable is the handling of finalization failures. There are essentially dragons whichever way you turn unless you can always decide that finalization won't be successful before changing any state. And that seems to be unlikely in the general case.

Ignoring the gap between theory and practice, finalization failures are only possible if the environment was changed in invalid ways (e.g., DRAM failure, application deliberately freeing something it didn't own, etc.). In my view, that means "finalization never fails" (and therefore aborts on error) is a generally preferable over one where the state becomes undefined on error.

(For example, the close() system call used to have an interesting failure more on some Unix implementation(s). Sadly, I have forgotten the details ... probably something like the file descriptor might or might not be closed, and if closed, another thread could cause it to be reused before the thread calling close could even inspect the result. Therefore making it impossible to handle the erorr correctly under all circumstances.)

(End of "old greybeard mode" 🙂)

@hidmic
Copy link
Contributor Author

hidmic commented Jun 23, 2020

Ignoring the gap between theory and practice, finalization failures are only possible if the environment was changed in invalid ways (e.g., DRAM failure, application deliberately freeing something it didn't own, etc.). In my view, that means "finalization never fails" (and therefore aborts on error) is a generally preferable over one where the state becomes undefined on error.

Yeah, there's that path too. But it seems to me that for finalization APIs, we're a bit late to the rmw_ret_t vs. void return type discussion. @wjwwood for comments.

@wjwwood
Copy link
Member

wjwwood commented Jun 23, 2020

Yeah we discussed this before but essentially we saw the return codes as a way to more easily assert how it failed in tests, and because the caller can always choose to abort anyways.

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic
Copy link
Contributor Author

hidmic commented Jun 24, 2020

Alright, fini API contract and implementation have been updated (see last commit in ros2/rmw#244 for reference).

One last sanity CI run:

  • Linux Build Status

@hidmic hidmic merged commit 18221f7 into master Jun 24, 2020
@delete-merged-branch delete-merged-branch bot deleted the hidmic/compliant-init-options-impl branch June 24, 2020 20:50
ahcorde pushed a commit that referenced this pull request Oct 8, 2020
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
ahcorde pushed a commit that referenced this pull request Oct 15, 2020
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
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

4 participants