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.

Sign up for GitHub

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

Jump to bottom

Ensure compliant init options API implementations. #399

Merged
merged 2 commits into from
Jun 24, 2020
Merged

Ensure compliant init options API implementations. #399

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a5a0c9a
Ensure compliant init options API implementations.
hidmic Jun 23, 2020
d0982ad
Update rmw_init_options_fini() implementation.
hidmic Jun 24, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 34 additions & 20 deletions rmw_fastrtps_shared_cpp/src/rmw_init.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ rmw_ret_t
rmw_init_options_init(
const char * identifier, rmw_init_options_t * init_options, rcutils_allocator_t allocator)
{
assert(identifier != NULL);
RMW_CHECK_ARGUMENT_FOR_NULL(init_options, RMW_RET_INVALID_ARGUMENT);
RCUTILS_CHECK_ALLOCATOR(&allocator, return RMW_RET_INVALID_ARGUMENT);
if (NULL != init_options->implementation_identifier) {
Expand All @@ -50,8 +51,13 @@ rmw_ret_t
rmw_init_options_copy(
const char * identifier, const rmw_init_options_t * src, rmw_init_options_t * dst)
{
assert(identifier != NULL);
RMW_CHECK_ARGUMENT_FOR_NULL(src, RMW_RET_INVALID_ARGUMENT);
RMW_CHECK_ARGUMENT_FOR_NULL(dst, RMW_RET_INVALID_ARGUMENT);
if (NULL == src->implementation_identifier) {
RMW_SET_ERROR_MSG("expected initialized dst");
return RMW_RET_INVALID_ARGUMENT;
}
RMW_CHECK_TYPE_IDENTIFIERS_MATCH(
src,
src->implementation_identifier,
Expand All @@ -62,39 +68,47 @@ rmw_init_options_copy(
return RMW_RET_INVALID_ARGUMENT;
}
Copy link
Member

Choose a reason for hiding this comment

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

without line 66-69, this function could perfectly work with a non-zero initialized dst AFAIS

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know, I prepared it for it. But it's lacking a finalization halfway through.

We can discuss guarantees later on and apply changes as needed.

const rcutils_allocator_t * allocator = &src->allocator;
rmw_ret_t ret = RMW_RET_OK;
RCUTILS_CHECK_ALLOCATOR(allocator, return RMW_RET_INVALID_ARGUMENT);

allocator->deallocate(dst->enclave, allocator->state);
*dst = *src;
dst->enclave = NULL;
dst->security_options = rmw_get_zero_initialized_security_options();

dst->enclave = rcutils_strdup(src->enclave, *allocator);
if (src->enclave && !dst->enclave) {
ret = RMW_RET_BAD_ALLOC;
goto fail;
rmw_init_options_t tmp = *src;
tmp.enclave = rcutils_strdup(tmp.enclave, *allocator);
if (NULL != src->enclave && NULL == tmp.enclave) {
return RMW_RET_BAD_ALLOC;
}
return rmw_security_options_copy(&src->security_options, allocator, &dst->security_options);
fail:
allocator->deallocate(dst->enclave, allocator->state);
return ret;
tmp.security_options = rmw_get_zero_initialized_security_options();
rmw_ret_t ret =
rmw_security_options_copy(&src->security_options, allocator, &tmp.security_options);
if (RMW_RET_OK != ret) {
allocator->deallocate(tmp.enclave, allocator->state);
return ret;
}
*dst = tmp;
return RMW_RET_OK;
}

rmw_ret_t
rmw_init_options_fini(const char * identifier, rmw_init_options_t * init_options)
{
assert(identifier != NULL);
RMW_CHECK_ARGUMENT_FOR_NULL(init_options, RMW_RET_INVALID_ARGUMENT);
rcutils_allocator_t & allocator = init_options->allocator;
RCUTILS_CHECK_ALLOCATOR(&allocator, return RMW_RET_INVALID_ARGUMENT);
if (NULL == init_options->implementation_identifier) {
RMW_SET_ERROR_MSG("expected initialized init_options");
return RMW_RET_INVALID_ARGUMENT;
}
RMW_CHECK_TYPE_IDENTIFIERS_MATCH(
init_options,
init_options->implementation_identifier,
identifier,
return RMW_RET_INCORRECT_RMW_IMPLEMENTATION);
allocator.deallocate(init_options->enclave, allocator.state);
rmw_security_options_fini(&init_options->security_options, &allocator);
*init_options = rmw_get_zero_initialized_init_options();
return RMW_RET_OK;
rcutils_allocator_t * allocator = &init_options->allocator;
RCUTILS_CHECK_ALLOCATOR(allocator, return RMW_RET_INVALID_ARGUMENT);

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.

should this happen if security options failed to be deallocated?
maybe:

allocator->deallocate(init_options->enclave, allocator->state);
rmw_ret_t ret = rmw_security_options_fini(&init_options->security_options, allocator);
if (RMW_RET_OK == ret) {
  *init_options = rmw_get_zero_initialized_init_options();
}
return ret;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, this is something I wasn't clear about in the API contract. I'm actively trying to ensure a failed finalization doesn't affect state. Alternatively, we could take the finalize-as-much-as-you-can approach AND prepare finalization functions for multiple calls on partially finalized state. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, fini API contract has been updated.

*init_options = rmw_get_zero_initialized_init_options();
}
return ret;
}

} // namespace rmw_fastrtps_shared_cpp