[Backport Foxy] Increased code coverage and some fixes

rcl/CMakeLists.txt

@@ -83,6 +83,10 @@ ament_target_dependencies(${PROJECT_NAME}
 target_compile_definitions(${PROJECT_NAME} PRIVATE "RCL_BUILDING_DLL")
 rcl_set_symbol_visibility_hidden(${PROJECT_NAME} LANGUAGE "C")
 
+if(BUILD_TESTING AND NOT RCUTILS_DISABLE_FAULT_INJECTION)
+ target_compile_definitions(${PROJECT_NAME} PUBLIC RCUTILS_ENABLE_FAULT_INJECTION)
+endif()
+
 install(
  TARGETS ${PROJECT_NAME} EXPORT ${PROJECT_NAME}
  ARCHIVE DESTINATION lib

rcl/include/rcl/context.h

@@ -154,8 +154,9 @@ rcl_get_zero_initialized_context(void);
 /**
  * The context to be finalized must have been previously initialized with
  * `rcl_init()`, and then later invalidated with `rcl_shutdown()`.
+ * A zero-initialized context that has not been initialized can be finalized.
  * If context is `NULL`, then `RCL_RET_INVALID_ARGUMENT` is returned.
- * If context is zero-initialized, then `RCL_RET_INVALID_ARGUMENT` is returned.
+ * If context is zero-initialized, then `RCL_RET_OK` is returned.
  * If context is initialized and valid (`rcl_shutdown()` was not called on it),
  * then `RCL_RET_INVALID_ARGUMENT` is returned.
  *

rcl/include/rcl/logging.h

@@ -48,7 +48,7 @@ typedef rcutils_logging_output_handler_t rcl_logging_output_handler_t;
  * \return `RCL_RET_OK` if successful, or
  * \return `RCL_RET_BAD_ALLOC` if allocating memory failed, or
  * \return `RCL_RET_INVALID_ARGUMENT` if any arguments are invalid, or
- * \return `RCL_RET_ERR` if a general error occurs
+ * \return `RCL_RET_ERROR` if a general error occurs
  */
 RCL_PUBLIC
 RCL_WARN_UNUSED
@@ -74,8 +74,9 @@ rcl_logging_configure(
  * \param allocator Used to allocate memory used by the logging system
  * \param output_handler Output handler to be installed
  * \return `RCL_RET_OK` if successful, or
+ * \return `RCL_RET_INVALID_ARGUMENT` if any arguments are invalid, or
  * \return `RCL_RET_BAD_ALLOC` if allocating memory failed, or
- * \return `RCL_RET_ERR` if a general error occurs
+ * \return `RCL_RET_ERROR` if a general error occurs
  */
 RCL_PUBLIC
 RCL_WARN_UNUSED
@@ -97,7 +98,7 @@ rcl_logging_configure_with_output_handler(
  * Lock-Free | Yes
  *
  * \return `RCL_RET_OK` if successful.
- * \return `RCL_RET_ERR` if a general error occurs
+ * \return `RCL_RET_ERROR` if a general error occurs
  */
 RCL_PUBLIC
 RCL_WARN_UNUSED

rcl/include/rcl/logging_rosout.h

@@ -98,7 +98,7 @@ rcl_logging_rosout_fini();
  * \return `RCL_RET_BAD_ALLOC` if allocating memory failed, or
  * \return `RCL_RET_ERROR` if an unspecified error occurs.
  */
-RCL_LOCAL
+RCL_PUBLIC
 RCL_WARN_UNUSED
 rcl_ret_t
 rcl_logging_rosout_init_publisher_for_node(
@@ -124,7 +124,7 @@ rcl_logging_rosout_init_publisher_for_node(
  * \return `RCL_RET_BAD_ALLOC` if allocating memory failed, or
  * \return `RCL_RET_ERROR` if an unspecified error occurs.
  */
-RCL_LOCAL
+RCL_PUBLIC
 RCL_WARN_UNUSED
 rcl_ret_t
 rcl_logging_rosout_fini_publisher_for_node(

rcl/include/rcl/remap.h

@@ -247,6 +247,31 @@ rcl_remap_node_namespace(
  rcl_allocator_t allocator,
  char ** output_namespace);
 
+/// Copy one remap structure into another.
+/**
+ * <hr>
+ * Attribute | Adherence
+ * ------------------ | -------------
+ * Allocates Memory | Yes
+ * Thread-Safe | No
+ * Uses Atomics | No
+ * Lock-Free | Yes
+ *
+ * \param[in] rule The structure to be copied.
+ * Its allocator is used to copy memory into the new structure.
+ * \param[out] rule_out A zero-initialized rcl_remap_t structure to be copied into.
+ * \return `RCL_RET_OK` if the structure was copied successfully, or
+ * \return `RCL_RET_INVALID_ARGUMENT` if any function arguments are invalid, or
+ * \return `RCL_RET_BAD_ALLOC` if allocating memory failed, or
+ * \return `RCL_RET_ERROR` if an unspecified error occurs.
+ */
+RCL_PUBLIC
+RCL_WARN_UNUSED
+rcl_ret_t
+rcl_remap_copy(
+ const rcl_remap_t * rule,
+ rcl_remap_t * rule_out);
+
 /// Reclaim resources held inside rcl_remap_t structure.
 /**
  * <hr>

rcl/include/rcl/rmw_implementation_identifier_check.h

@@ -0,0 +1,35 @@
+// Copyright 2020 Open Source Robotics Foundation, Inc.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#ifndef RCL__RMW_IMPLEMENTATION_IDENTIFIER_CHECK_H_
+#define RCL__RMW_IMPLEMENTATION_IDENTIFIER_CHECK_H_
+
+#ifdef __cplusplus
+extern "C"
+{
+#endif
+
+#include "rcl/visibility_control.h"
+
+#define RMW_IMPLEMENTATION_ENV_VAR_NAME "RMW_IMPLEMENTATION"
+#define RCL_ASSERT_RMW_ID_MATCHES_ENV_VAR_NAME "RCL_ASSERT_RMW_ID_MATCHES"
+
+RCL_PUBLIC
+rcl_ret_t rcl_rmw_implementation_identifier_check(void);
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif // RCL__RMW_IMPLEMENTATION_IDENTIFIER_CHECK_H_

rcl/package.xml

@@ -25,13 +25,14 @@
  <test_depend>ament_cmake_pytest</test_depend>
  <test_depend>ament_lint_auto</test_depend>
  <test_depend>ament_lint_common</test_depend>
- <test_depend>rcpputils</test_depend>
- <test_depend>rmw</test_depend>
- <test_depend>rmw_implementation_cmake</test_depend>
  <test_depend>launch</test_depend>
  <test_depend>launch_testing</test_depend>
  <test_depend>launch_testing_ament_cmake</test_depend>
+ <test_depend>mimick_vendor</test_depend>
  <test_depend>osrf_testing_tools_cpp</test_depend>
+ <test_depend>rcpputils</test_depend>
+ <test_depend>rmw</test_depend>
+ <test_depend>rmw_implementation_cmake</test_depend>
  <test_depend>test_msgs</test_depend>
 
  <group_depend>rcl_logging_packages</group_depend>

rcl/src/rcl/arguments.c

@@ -93,10 +93,7 @@ rcl_arguments_get_param_files(
  if (NULL == (*parameter_files)[i]) {
  // deallocate allocated memory
  for (int r = i; r >= 0; --r) {
- if (NULL == (*parameter_files[r])) {
- break;
- }
- allocator.deallocate((*parameter_files[r]), allocator.state);
+ allocator.deallocate((*parameter_files)[r], allocator.state);
  }
  allocator.deallocate((*parameter_files), allocator.state);
  (*parameter_files) = NULL;
@@ -579,30 +576,36 @@ rcl_parse_arguments(
  }
 
  // Shrink remap_rules array to match number of successfully parsed rules
- if (args_impl->num_remap_rules > 0) {
- args_impl->remap_rules = rcutils_reallocf(
- args_impl->remap_rules, sizeof(rcl_remap_t) * args_impl->num_remap_rules, &allocator);
- if (NULL == args_impl->remap_rules) {
- ret = RCL_RET_BAD_ALLOC;
- goto fail;
- }
- } else {
+ if (0 == args_impl->num_remap_rules) {
  // No remap rules
  allocator.deallocate(args_impl->remap_rules, allocator.state);
  args_impl->remap_rules = NULL;
+ } else if (args_impl->num_remap_rules < argc) {
+ rcl_remap_t * new_remap_rules = allocator.reallocate(
+ args_impl->remap_rules,
+ sizeof(rcl_remap_t) * args_impl->num_remap_rules,
+ &allocator);
+ if (NULL == new_remap_rules) {
+ ret = RCL_RET_BAD_ALLOC;
+ goto fail;
+ }
+ args_impl->remap_rules = new_remap_rules;
  }
 
  // Shrink Parameter files
  if (0 == args_impl->num_param_files_args) {
  allocator.deallocate(args_impl->parameter_files, allocator.state);
  args_impl->parameter_files = NULL;
  } else if (args_impl->num_param_files_args < argc) {
- args_impl->parameter_files = rcutils_reallocf(
- args_impl->parameter_files, sizeof(char *) * args_impl->num_param_files_args, &allocator);
- if (NULL == args_impl->parameter_files) {
+ char ** new_parameter_files = allocator.reallocate(
+ args_impl->parameter_files,
+ sizeof(char *) * args_impl->num_param_files_args,
+ &allocator);
+ if (NULL == new_parameter_files) {
  ret = RCL_RET_BAD_ALLOC;
  goto fail;
  }
+ args_impl->parameter_files = new_parameter_files;
  }
 
  // Drop parameter overrides if none was found.
@@ -786,6 +789,9 @@ rcl_arguments_copy(
  const rcl_arguments_t * args,
  rcl_arguments_t * args_out)
 {
+ RCUTILS_CAN_SET_MSG_AND_RETURN_WITH_ERROR_OF(RCL_RET_INVALID_ARGUMENT);
+ RCUTILS_CAN_SET_MSG_AND_RETURN_WITH_ERROR_OF(RCL_RET_BAD_ALLOC);
+
  RCL_CHECK_ARGUMENT_FOR_NULL(args, RCL_RET_INVALID_ARGUMENT);
  RCL_CHECK_ARGUMENT_FOR_NULL(args->impl, RCL_RET_INVALID_ARGUMENT);
  RCL_CHECK_ARGUMENT_FOR_NULL(args_out, RCL_RET_INVALID_ARGUMENT);
@@ -843,7 +849,6 @@ rcl_arguments_copy(
  }
  return RCL_RET_BAD_ALLOC;
  }
- args_out->impl->num_remap_rules = args->impl->num_remap_rules;
  for (int i = 0; i < args->impl->num_remap_rules; ++i) {
  args_out->impl->remap_rules[i] = rcl_get_zero_initialized_remap();
  ret = rcl_remap_copy(
@@ -854,6 +859,7 @@ rcl_arguments_copy(
  }
  return ret;
  }
+ ++(args_out->impl->num_remap_rules);
  }
  }
 
@@ -865,15 +871,14 @@ rcl_arguments_copy(
 
  // Copy parameter files
  if (args->impl->num_param_files_args) {
- args_out->impl->parameter_files = allocator.allocate(
- sizeof(char *) * args->impl->num_param_files_args, allocator.state);
+ args_out->impl->parameter_files = allocator.zero_allocate(
+ args->impl->num_param_files_args, sizeof(char *), allocator.state);
  if (NULL == args_out->impl->parameter_files) {
  if (RCL_RET_OK != rcl_arguments_fini(args_out)) {
  RCL_SET_ERROR_MSG("Error while finalizing arguments due to another error");
  }
  return RCL_RET_BAD_ALLOC;
  }
- args_out->impl->num_param_files_args = args->impl->num_param_files_args;
  for (int i = 0; i < args->impl->num_param_files_args; ++i) {
  args_out->impl->parameter_files[i] =
  rcutils_strdup(args->impl->parameter_files[i], allocator);
@@ -883,6 +888,7 @@ rcl_arguments_copy(
  }
  return RCL_RET_BAD_ALLOC;
  }
+ ++(args_out->impl->num_param_files_args);
  }
  }
  char * enclave_copy = rcutils_strdup(args->impl->enclave, allocator);
@@ -1619,9 +1625,8 @@ _rcl_parse_remap_rule(
  RCL_CHECK_ARGUMENT_FOR_NULL(arg, RCL_RET_INVALID_ARGUMENT);
  RCL_CHECK_ARGUMENT_FOR_NULL(output_rule, RCL_RET_INVALID_ARGUMENT);
 
- rcl_ret_t ret;
-
- output_rule->impl = allocator.allocate(sizeof(rcl_remap_impl_t), allocator.state);
+ output_rule->impl =
+ allocator.allocate(sizeof(rcl_remap_impl_t), allocator.state);
  if (NULL == output_rule->impl) {
  return RCL_RET_BAD_ALLOC;
  }
@@ -1632,25 +1637,31 @@ _rcl_parse_remap_rule(
  output_rule->impl->replacement = NULL;
 
  rcl_lexer_lookahead2_t lex_lookahead = rcl_get_zero_initialized_lexer_lookahead2();
+ rcl_ret_t ret = rcl_lexer_lookahead2_init(&lex_lookahead, arg, allocator);
 
- ret = rcl_lexer_lookahead2_init(&lex_lookahead, arg, allocator);
- if (RCL_RET_OK != ret) {
- return ret;
- }
-
- ret = _rcl_parse_remap_begin_remap_rule(&lex_lookahead, output_rule);
+ if (RCL_RET_OK == ret) {
+ ret = _rcl_parse_remap_begin_remap_rule(&lex_lookahead, output_rule);
 
- if (RCL_RET_OK != ret) {
- // cleanup stuff, but return the original error code
- if (RCL_RET_OK != rcl_remap_fini(output_rule)) {
- RCUTILS_LOG_ERROR_NAMED(ROS_PACKAGE_NAME, "Failed to fini remap rule after error occurred");
- }
- if (RCL_RET_OK != rcl_lexer_lookahead2_fini(&lex_lookahead)) {
- RCUTILS_LOG_ERROR_NAMED(ROS_PACKAGE_NAME, "Failed to fini lookahead2 after error occurred");
+ rcl_ret_t fini_ret = rcl_lexer_lookahead2_fini(&lex_lookahead);
+ if (RCL_RET_OK != ret) {
+ if (RCL_RET_OK != fini_ret) {
+ RCUTILS_LOG_ERROR_NAMED(
+ ROS_PACKAGE_NAME, "Failed to fini lookahead2 after error occurred");
+ }
+ } else {
+ if (RCL_RET_OK == fini_ret) {
+ return RCL_RET_OK;
+ }
+ ret = fini_ret;
  }
- } else {
- ret = rcl_lexer_lookahead2_fini(&lex_lookahead);
  }
+
+ // cleanup output rule but keep first error return code
+ if (RCL_RET_OK != rcl_remap_fini(output_rule)) {
+ RCUTILS_LOG_ERROR_NAMED(
+ ROS_PACKAGE_NAME, "Failed to fini remap rule after error occurred");
+ }
+
  return ret;
 }
 
@@ -1747,6 +1758,8 @@ _rcl_parse_param_file(
  return RCL_RET_BAD_ALLOC;
  }
  if (!rcl_parse_yaml_file(*param_file, params)) {
+ allocator.deallocate(*param_file, allocator.state);
+ *param_file = NULL;
  // Error message already set.
  return RCL_RET_ERROR;
  }

rcl/src/rcl/client.c

@@ -26,6 +26,7 @@ extern "C"
 #include "rcl/expand_topic_name.h"
 #include "rcl/remap.h"
 #include "rcutils/logging_macros.h"
+#include "rcutils/macros.h"
 #include "rcutils/stdatomic_helper.h"
 #include "rmw/error_handling.h"
 #include "rmw/rmw.h"
@@ -206,6 +207,10 @@ rcl_client_init(
 rcl_ret_t
 rcl_client_fini(rcl_client_t * client, rcl_node_t * node)
 {
+ RCUTILS_CAN_RETURN_WITH_ERROR_OF(RCL_RET_INVALID_ARGUMENT);
+ RCUTILS_CAN_RETURN_WITH_ERROR_OF(RCL_RET_NODE_INVALID);
+ RCUTILS_CAN_RETURN_WITH_ERROR_OF(RCL_RET_ERROR);
+
  RCUTILS_LOG_DEBUG_NAMED(ROS_PACKAGE_NAME, "Finalizing client");
  rcl_ret_t result = RCL_RET_OK;
  RCL_CHECK_ARGUMENT_FOR_NULL(client, RCL_RET_INVALID_ARGUMENT);