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 node construction/destruction API. #408

Merged
merged 7 commits into from
Jul 15, 2020
Merged

Ensure compliant node construction/destruction API. #408

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
2f2d195
Ensure compliant node construction/destruction API.
hidmic Jul 6, 2020
85a8b45
Use known validation result values.
hidmic Jul 13, 2020
3921f1c
Revert allocator use changes.
hidmic Jul 13, 2020
468c493
Drop unnecessary assertions.
hidmic Jul 13, 2020
dc6d838
Drop a few more unnecessary assertions.
hidmic Jul 14, 2020
465fb7a
Fix clang warnings.
hidmic Jul 14, 2020
8ee8b24
Fix one last clang warning.
hidmic Jul 14, 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
18 changes: 17 additions & 1 deletion rmw_fastrtps_cpp/src/rmw_node.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@

#include "rmw_fastrtps_shared_cpp/init_rmw_context_impl.hpp"
#include "rmw_fastrtps_shared_cpp/rmw_common.hpp"
#include "rmw_fastrtps_shared_cpp/rmw_context_impl.hpp"

#include "rmw_fastrtps_cpp/identifier.hpp"
#include "rmw_fastrtps_cpp/init_rmw_context_impl.hpp"
Expand All @@ -40,13 +41,21 @@ rmw_create_node(
const char * name,
const char * namespace_)
{
RCUTILS_CHECK_ARGUMENT_FOR_NULL(context, NULL);
RMW_CHECK_ARGUMENT_FOR_NULL(context, nullptr);
RMW_CHECK_TYPE_IDENTIFIERS_MATCH(
init context,
context->implementation_identifier,
eprosima_fastrtps_identifier,
// TODO(wjwwood): replace this with RMW_RET_INCORRECT_RMW_IMPLEMENTATION when refactored
return nullptr);
RMW_CHECK_FOR_NULL_WITH_MSG(
context->impl,
"expected initialized context",
return nullptr);
if (context->impl->is_shutdown) {
RCUTILS_SET_ERROR_MSG("context has been shutdown");
return nullptr;
}

if (RMW_RET_OK != rmw_fastrtps_cpp::increment_context_impl_ref_count(context)) {
return nullptr;
Expand All @@ -68,6 +77,13 @@ rmw_create_node(
rmw_ret_t
rmw_destroy_node(rmw_node_t * node)
{
RMW_CHECK_ARGUMENT_FOR_NULL(node, RMW_RET_INVALID_ARGUMENT);
RMW_CHECK_TYPE_IDENTIFIERS_MATCH(
node,
node->implementation_identifier,
eprosima_fastrtps_identifier,
return RMW_RET_INCORRECT_RMW_IMPLEMENTATION);

rmw_context_t * context = node->context;
rmw_ret_t ret = rmw_fastrtps_shared_cpp::__rmw_destroy_node(
eprosima_fastrtps_identifier, node);
Expand Down
19 changes: 17 additions & 2 deletions rmw_fastrtps_dynamic_cpp/src/rmw_node.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@

#include "rmw_fastrtps_shared_cpp/init_rmw_context_impl.hpp"
#include "rmw_fastrtps_shared_cpp/rmw_common.hpp"
#include "rmw_fastrtps_shared_cpp/rmw_context_impl.hpp"

#include "rmw_fastrtps_dynamic_cpp/identifier.hpp"
#include "rmw_fastrtps_dynamic_cpp/init_rmw_context_impl.hpp"
Expand All @@ -40,13 +41,21 @@ rmw_create_node(
const char * name,
const char * namespace_)
{
RCUTILS_CHECK_ARGUMENT_FOR_NULL(context, NULL);
RMW_CHECK_ARGUMENT_FOR_NULL(context, nullptr);
RMW_CHECK_TYPE_IDENTIFIERS_MATCH(
init context,
context->implementation_identifier,
eprosima_fastrtps_identifier,
// TODO(wjwwood): replace this with RMW_RET_INCORRECT_RMW_IMPLEMENTATION when refactored
return NULL);
return nullptr);
RMW_CHECK_FOR_NULL_WITH_MSG(
context->impl,
"expected initialized context",
return nullptr);
if (context->impl->is_shutdown) {
RCUTILS_SET_ERROR_MSG("context has been shutdown");
return nullptr;
}

if (RMW_RET_OK != rmw_fastrtps_dynamic_cpp::increment_context_impl_ref_count(context)) {
return nullptr;
Expand All @@ -68,6 +77,12 @@ rmw_create_node(
rmw_ret_t
rmw_destroy_node(rmw_node_t * node)
{
RMW_CHECK_ARGUMENT_FOR_NULL(node, RMW_RET_INVALID_ARGUMENT);
RMW_CHECK_TYPE_IDENTIFIERS_MATCH(
node,
node->implementation_identifier,
eprosima_fastrtps_identifier,
return RMW_RET_INCORRECT_RMW_IMPLEMENTATION);
rmw_context_t * context = node->context;
rmw_ret_t ret = rmw_fastrtps_shared_cpp::__rmw_destroy_node(
eprosima_fastrtps_identifier, node);
Expand Down
91 changes: 46 additions & 45 deletions rmw_fastrtps_shared_cpp/src/rmw_node.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,10 @@
#include "rmw/error_handling.h"
#include "rmw/impl/cpp/macros.hpp"
#include "rmw/rmw.h"
#include "rmw/validate_namespace.h"
#include "rmw/validate_node_name.h"

#include "rcpputils/scope_exit.hpp"

#include "rmw_dds_common/context.hpp"

Expand All @@ -42,44 +46,61 @@ __rmw_create_node(
const char * name,
const char * namespace_)
{
if (!name) {
RMW_SET_ERROR_MSG("name is null");
assert(identifier == context->implementation_identifier);

int validation_result = RMW_NODE_NAME_VALID;
rmw_ret_t ret = rmw_validate_node_name(name, &validation_result, nullptr);
if (RMW_RET_OK != ret) {
return nullptr;
}

if (!namespace_) {
RMW_SET_ERROR_MSG("namespace_ is null");
if (RMW_NODE_NAME_VALID != validation_result) {
const char * reason = rmw_node_name_validation_result_string(validation_result);
RMW_SET_ERROR_MSG_WITH_FORMAT_STRING("invalid node name: %s", reason);
return nullptr;
}
validation_result = RMW_NAMESPACE_VALID;
ret = rmw_validate_namespace(namespace_, &validation_result, nullptr);
if (RMW_RET_OK != ret) {
return nullptr;
}
if (RMW_NAMESPACE_VALID != validation_result) {
const char * reason = rmw_node_name_validation_result_string(validation_result);
RMW_SET_ERROR_MSG_WITH_FORMAT_STRING("invalid node namespace: %s", reason);
return nullptr;
}

rmw_node_t * node_handle = nullptr;
auto common_context = static_cast<rmw_dds_common::Context *>(context->impl->common);
rmw_dds_common::GraphCache & graph_cache = common_context->graph_cache;

node_handle = rmw_node_allocate();
if (!node_handle) {
RMW_SET_ERROR_MSG("failed to allocate rmw_node_t");
goto fail;
rmw_node_t * node_handle = rmw_node_allocate();
if (nullptr == node_handle) {
RMW_SET_ERROR_MSG("failed to allocate node");
return nullptr;
}
auto cleanup_node = rcpputils::make_scope_exit(
[node_handle]() {
rmw_free(const_cast<char *>(node_handle->name));
rmw_free(const_cast<char *>(node_handle->namespace_));
rmw_node_free(node_handle);
});
node_handle->implementation_identifier = identifier;
node_handle->data = nullptr;

node_handle->name =
static_cast<const char *>(rmw_allocate(sizeof(char) * strlen(name) + 1));
if (!node_handle->name) {
RMW_SET_ERROR_MSG("failed to allocate memory");
node_handle->namespace_ = nullptr; // to avoid free on uninitialized memory
goto fail;
if (nullptr == node_handle->name) {
RMW_SET_ERROR_MSG("failed to copy node name");
return nullptr;
}
memcpy(const_cast<char *>(node_handle->name), name, strlen(name) + 1);

node_handle->namespace_ =
static_cast<const char *>(rmw_allocate(sizeof(char) * strlen(namespace_) + 1));
if (!node_handle->namespace_) {
RMW_SET_ERROR_MSG("failed to allocate memory");
goto fail;
if (nullptr == node_handle->namespace_) {
RMW_SET_ERROR_MSG("failed to copy node namespace");
return nullptr;
}
memcpy(const_cast<char *>(node_handle->namespace_), namespace_, strlen(namespace_) + 1);

node_handle->context = context;

{
Expand All @@ -92,63 +113,43 @@ __rmw_create_node(
rmw_dds_common::msg::ParticipantEntitiesInfo participant_msg =
graph_cache.add_node(common_context->gid, name, namespace_);
if (RMW_RET_OK != __rmw_publish(
identifier,
node_handle->implementation_identifier,
common_context->pub,
static_cast<void *>(&participant_msg),
nullptr))
{
goto fail;
return nullptr;
}
}
cleanup_node.cancel();
return node_handle;
fail:
if (node_handle) {
rmw_free(const_cast<char *>(node_handle->namespace_));
node_handle->namespace_ = nullptr;
rmw_free(const_cast<char *>(node_handle->name));
node_handle->name = nullptr;
}
rmw_node_free(node_handle);
return nullptr;
}

rmw_ret_t
__rmw_destroy_node(
const char * identifier,
rmw_node_t * node)
{
rmw_ret_t result_ret = RMW_RET_OK;
if (!node) {
RMW_SET_ERROR_MSG("node handle is null");
return RMW_RET_ERROR;
}

if (node->implementation_identifier != identifier) {
RMW_SET_ERROR_MSG("node handle not from this implementation");
return RMW_RET_ERROR;
}
assert(node->implementation_identifier == identifier);

auto common_context = static_cast<rmw_dds_common::Context *>(node->context->impl->common);
rmw_dds_common::GraphCache & graph_cache = common_context->graph_cache;
{
std::lock_guard<std::mutex> guard(common_context->node_update_mutex);
rmw_dds_common::msg::ParticipantEntitiesInfo participant_msg =
graph_cache.remove_node(common_context->gid, node->name, node->namespace_);
result_ret = __rmw_publish(
rmw_ret_t ret = __rmw_publish(
identifier,
common_context->pub,
static_cast<void *>(&participant_msg),
nullptr);
if (RMW_RET_OK != result_ret) {
return result_ret;
if (RMW_RET_OK != ret) {
return ret;
}
}
rmw_free(const_cast<char *>(node->name));
node->name = nullptr;
rmw_free(const_cast<char *>(node->namespace_));
node->namespace_ = nullptr;
rmw_node_free(node);

return RMW_RET_OK;
}

Expand Down