From 5fa1b0e0d9d63de3e6eddda87276cc290857bcb2 Mon Sep 17 00:00:00 2001 From: Shane Loretz Date: Wed, 21 Mar 2018 15:05:30 -0700 Subject: [PATCH] CLI remapping followup (#221) * Document graph functions don't remap * Prettier if statements using rmw_validate_* * all arguments on one line * Whe -> The * Check args for NULL * Fix topic remap being interpretted as namespace remap --- rcl/include/rcl/arguments.h | 2 +- rcl/include/rcl/graph.h | 20 ++++++++++++++++++++ rcl/src/rcl/arguments.c | 29 +++++++++++++++-------------- rcl/test/rcl/test_arguments.cpp | 1 + 4 files changed, 37 insertions(+), 15 deletions(-) diff --git a/rcl/include/rcl/arguments.h b/rcl/include/rcl/arguments.h index 1bf13a3b2..b271dbbdb 100644 --- a/rcl/include/rcl/arguments.h +++ b/rcl/include/rcl/arguments.h @@ -64,7 +64,7 @@ rcl_get_zero_initialized_arguments(void); * Lock-Free | Yes * * \param[in] argc The number of arguments in argv. - * \param[in] argv Whe values of the arguments. + * \param[in] argv The values of the arguments. * \param[in] allocator A valid allocator. * \param[out] args_output A structure that will contain the result of parsing. * \return `RCL_RET_OK` if the arguments were parsed successfully, or diff --git a/rcl/include/rcl/graph.h b/rcl/include/rcl/graph.h index c2755e07f..37199d190 100644 --- a/rcl/include/rcl/graph.h +++ b/rcl/include/rcl/graph.h @@ -56,6 +56,10 @@ typedef rmw_names_and_types_t rcl_names_and_types_t; * * \see rmw_get_topic_names_and_types for more details on no_demangle * + * The returned names are not automatically remapped by this function. + * Attempting to create publishers or subscribers using names returned by this function may not + * result in the desired topic name being used depending on the remap rules in use. + * *
* Attribute | Adherence * ------------------ | ------------- @@ -96,6 +100,10 @@ rcl_get_topic_names_and_types( * it is no longer needed. * Failing to do so will result in leaked memory. * + * The returned names are not automatically remapped by this function. + * Attempting to create clients or services using names returned by this function may not result in + * the desired service name being used depending on the remap rules in use. + * *
* Attribute | Adherence * ------------------ | ------------- @@ -219,6 +227,12 @@ rcl_get_node_names( * In the event that error handling needs to allocate memory, this function * will try to use the node's allocator. * + * The topic name is not automatically remapped by this function. + * If there is a publisher created with topic name `foo` and remap rule `foo:=bar` then calling + * this with `topic_name` set to `bar` will return a count of 1, and with `topic_name` set to `foo` + * will return a count of 0. + * /sa rcl_remap_topic_name() + * *
* Attribute | Adherence * ------------------ | ------------- @@ -260,6 +274,12 @@ rcl_count_publishers( * In the event that error handling needs to allocate memory, this function * will try to use the node's allocator. * + * The topic name is not automatically remapped by this function. + * If there is a subscriber created with topic name `foo` and remap rule `foo:=bar` then calling + * this with `topic_name` set to `bar` will return a count of 1, and with `topic_name` set to `foo` + * will return a count of 0. + * /sa rcl_remap_topic_name() + * *
* Attribute | Adherence * ------------------ | ------------- diff --git a/rcl/src/rcl/arguments.c b/rcl/src/rcl/arguments.c index 4378ca11a..aa1e1f5a4 100644 --- a/rcl/src/rcl/arguments.c +++ b/rcl/src/rcl/arguments.c @@ -50,6 +50,9 @@ _rcl_parse_remap_rule( rcl_allocator_t allocator, rcl_remap_t * output_rule) { + RCL_CHECK_ARGUMENT_FOR_NULL(arg, RCL_RET_INVALID_ARGUMENT, allocator); + RCL_CHECK_ARGUMENT_FOR_NULL(output_rule, RCL_RET_INVALID_ARGUMENT, allocator); + size_t len_node_name = 0; size_t len_match = 0; size_t len_replacement = 0; @@ -108,14 +111,12 @@ _rcl_parse_remap_rule( if (len_node_name) { int validation_result; size_t invalid_index; - if ( - RMW_RET_OK != rmw_validate_node_name_with_size(arg, len_node_name, &validation_result, - &invalid_index)) - { + rmw_ret_t rmw_ret = rmw_validate_node_name_with_size( + arg, len_node_name, &validation_result, &invalid_index); + if (RMW_RET_OK != rmw_ret) { RCL_SET_ERROR_MSG("failed to run check on node name", allocator); return RCL_RET_ERROR; - } - if (RMW_NODE_NAME_VALID != validation_result) { + } else if (RMW_NODE_NAME_VALID != validation_result) { RCL_SET_ERROR_MSG_WITH_FORMAT_STRING( allocator, "node name prefix invalid: %s", rmw_node_name_validation_result_string(validation_result)); @@ -125,9 +126,9 @@ _rcl_parse_remap_rule( // Figure out what type of rule this is, default is to apply to topic and service names rcl_remap_type_t type = RCL_TOPIC_REMAP | RCL_SERVICE_REMAP; - if (0 == strncmp("__ns", match_begin, len_match)) { + if (4 == len_match && 0 == strncmp("__ns", match_begin, len_match)) { type = RCL_NAMESPACE_REMAP; - } else if (0 == strncmp("__node", match_begin, len_match)) { + } else if (6 == len_match && 0 == strncmp("__node", match_begin, len_match)) { type = RCL_NODENAME_REMAP; } @@ -158,11 +159,11 @@ _rcl_parse_remap_rule( } else if (RCL_NAMESPACE_REMAP == type) { int validation_result; size_t invalid_idx; - if (RMW_RET_OK != rmw_validate_namespace(replacement_begin, &validation_result, &invalid_idx)) { + rmw_ret_t rmw_ret = rmw_validate_namespace(replacement_begin, &validation_result, &invalid_idx); + if (RMW_RET_OK != rmw_ret) { RCL_SET_ERROR_MSG("failed to run check on namespace", allocator); return RCL_RET_ERROR; - } - if (RMW_NAMESPACE_VALID != validation_result) { + } else if (RMW_NAMESPACE_VALID != validation_result) { RCL_SET_ERROR_MSG_WITH_FORMAT_STRING( allocator, "namespace is invalid: %s", rmw_namespace_validation_result_string(validation_result)); @@ -171,11 +172,11 @@ _rcl_parse_remap_rule( } else if (RCL_NODENAME_REMAP == type) { int validation_result; size_t invalid_idx; - if (RMW_RET_OK != rmw_validate_node_name(replacement_begin, &validation_result, &invalid_idx)) { + rmw_ret_t rmw_ret = rmw_validate_node_name(replacement_begin, &validation_result, &invalid_idx); + if (RMW_RET_OK != rmw_ret) { RCL_SET_ERROR_MSG("failed to run check on node name", allocator); return RCL_RET_ERROR; - } - if (RMW_NODE_NAME_VALID != validation_result) { + } else if (RMW_NODE_NAME_VALID != validation_result) { RCL_SET_ERROR_MSG_WITH_FORMAT_STRING( allocator, "node name is invalid: %s", rmw_node_name_validation_result_string(validation_result)); diff --git a/rcl/test/rcl/test_arguments.cpp b/rcl/test/rcl/test_arguments.cpp index 31aa00d75..7d6c22660 100644 --- a/rcl/test/rcl/test_arguments.cpp +++ b/rcl/test/rcl/test_arguments.cpp @@ -87,6 +87,7 @@ TEST_F(CLASSNAME(TestArgumentsFixture, RMW_IMPLEMENTATION), check_valid_vs_inval EXPECT_TRUE(is_valid_arg("__node:=nodename123")); EXPECT_TRUE(is_valid_arg("__ns:=/foo/bar")); EXPECT_TRUE(is_valid_arg("__ns:=/")); + EXPECT_TRUE(is_valid_arg("_:=kq")); EXPECT_TRUE(is_valid_arg("nodename:__ns:=/foobar")); EXPECT_TRUE(is_valid_arg("foo:=bar")); EXPECT_TRUE(is_valid_arg("~/foo:=~/bar"));