From b0ee2608a971bdce244c37fd85bfe6ed6eb81ff8 Mon Sep 17 00:00:00 2001 From: Michael Carroll Date: Wed, 4 Apr 2018 09:50:08 -0500 Subject: [PATCH 1/5] Correctly clean up arguments structure. --- rclcpp/src/rclcpp/utilities.cpp | 27 +++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-) diff --git a/rclcpp/src/rclcpp/utilities.cpp b/rclcpp/src/rclcpp/utilities.cpp index 2954133ee0..749b489b2d 100644 --- a/rclcpp/src/rclcpp/utilities.cpp +++ b/rclcpp/src/rclcpp/utilities.cpp @@ -210,7 +210,16 @@ rclcpp::remove_ros_arguments(int argc, char const * const argv[]) ret = rcl_parse_arguments(argc, argv, alloc, &parsed_args); if (RCL_RET_OK != ret) { - exceptions::throw_from_rcl_error(ret, "Failed to parse arguments"); + // Not using throw_from_rcl_error, because we may need to append deallocation failures. + exceptions::RCLErrorBase base_exec(ret, rcl_get_error_state()); + rcl_reset_error(); + if (RCL_RET_OK != rcl_arguments_fini(&parsed_args)) { + base_exec.formatted_message += std::string( + ", failed also to cleanup parsed arguments, leaking memory: ") + + rcl_get_error_string_safe(); + rcl_reset_error(); + } + throw base_exec; } int nonros_argc = 0; @@ -224,10 +233,19 @@ rclcpp::remove_ros_arguments(int argc, char const * const argv[]) &nonros_argv); if (RCL_RET_OK != ret) { + // Not using throw_from_rcl_error, because we may need to append deallocation failures. + exceptions::RCLErrorBase base_exec(ret, rcl_get_error_state()); + rcl_reset_error(); if (NULL != nonros_argv) { alloc.deallocate(nonros_argv, alloc.state); } - exceptions::throw_from_rcl_error(ret, "Failed to remove ROS arguments: "); + if (RCL_RET_OK != rcl_arguments_fini(&parsed_args)) { + base_exec.formatted_message += std::string( + ", failed also to cleanup parsed arguments, leaking memory: ") + + rcl_get_error_string_safe(); + rcl_reset_error(); + } + throw base_exec; } std::vector return_arguments; @@ -241,6 +259,11 @@ rclcpp::remove_ros_arguments(int argc, char const * const argv[]) alloc.deallocate(nonros_argv, alloc.state); } + ret = rcl_arguments_fini(&parsed_args); + if (RCL_RET_OK != ret) { + exceptions::throw_from_rcl_error(ret, "failed to cleanup parsed arguments, leaking memory: "); + } + return return_arguments; } From f6f57943b3d6bb5584654ee7d63fb14168555c81 Mon Sep 17 00:00:00 2001 From: Michael Carroll Date: Thu, 5 Apr 2018 16:23:25 -0500 Subject: [PATCH 2/5] Use zero-initialized structure. --- rclcpp/src/rclcpp/utilities.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rclcpp/src/rclcpp/utilities.cpp b/rclcpp/src/rclcpp/utilities.cpp index 749b489b2d..a8ecf47b6d 100644 --- a/rclcpp/src/rclcpp/utilities.cpp +++ b/rclcpp/src/rclcpp/utilities.cpp @@ -204,7 +204,7 @@ std::vector rclcpp::remove_ros_arguments(int argc, char const * const argv[]) { rcl_allocator_t alloc = rcl_get_default_allocator(); - rcl_arguments_t parsed_args; + rcl_arguments_t parsed_args = rcl_get_zero_initialized_arguments(); rcl_ret_t ret; From f284100962dc3dec4834ffa28296b4ac74ca193d Mon Sep 17 00:00:00 2001 From: Michael Carroll Date: Thu, 5 Apr 2018 16:40:10 -0500 Subject: [PATCH 3/5] Avoid repeated cleanup. --- rclcpp/src/rclcpp/utilities.cpp | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/rclcpp/src/rclcpp/utilities.cpp b/rclcpp/src/rclcpp/utilities.cpp index a8ecf47b6d..4d267183d6 100644 --- a/rclcpp/src/rclcpp/utilities.cpp +++ b/rclcpp/src/rclcpp/utilities.cpp @@ -210,16 +210,7 @@ rclcpp::remove_ros_arguments(int argc, char const * const argv[]) ret = rcl_parse_arguments(argc, argv, alloc, &parsed_args); if (RCL_RET_OK != ret) { - // Not using throw_from_rcl_error, because we may need to append deallocation failures. - exceptions::RCLErrorBase base_exec(ret, rcl_get_error_state()); - rcl_reset_error(); - if (RCL_RET_OK != rcl_arguments_fini(&parsed_args)) { - base_exec.formatted_message += std::string( - ", failed also to cleanup parsed arguments, leaking memory: ") + - rcl_get_error_string_safe(); - rcl_reset_error(); - } - throw base_exec; + exceptions::throw_from_rcl_error(ret, "failed to parse arguments"); } int nonros_argc = 0; From c960ce21e37884a140ea12e8959e7982a32a1a93 Mon Sep 17 00:00:00 2001 From: Michael Carroll Date: Fri, 6 Apr 2018 09:47:25 -0500 Subject: [PATCH 4/5] Address reviewer comments. --- rclcpp/src/rclcpp/utilities.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rclcpp/src/rclcpp/utilities.cpp b/rclcpp/src/rclcpp/utilities.cpp index 4d267183d6..f4552dabd8 100644 --- a/rclcpp/src/rclcpp/utilities.cpp +++ b/rclcpp/src/rclcpp/utilities.cpp @@ -252,7 +252,7 @@ rclcpp::remove_ros_arguments(int argc, char const * const argv[]) ret = rcl_arguments_fini(&parsed_args); if (RCL_RET_OK != ret) { - exceptions::throw_from_rcl_error(ret, "failed to cleanup parsed arguments, leaking memory: "); + exceptions::throw_from_rcl_error(ret, "failed to cleanup parsed arguments, leaking memory"); } return return_arguments; From d714b38bb9652798014ac88832294cfd0b98ee84 Mon Sep 17 00:00:00 2001 From: Michael Carroll Date: Fri, 6 Apr 2018 13:05:39 -0500 Subject: [PATCH 5/5] Adress reviewer feedback on naming. --- rclcpp/src/rclcpp/utilities.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/rclcpp/src/rclcpp/utilities.cpp b/rclcpp/src/rclcpp/utilities.cpp index f4552dabd8..b85b857641 100644 --- a/rclcpp/src/rclcpp/utilities.cpp +++ b/rclcpp/src/rclcpp/utilities.cpp @@ -225,18 +225,18 @@ rclcpp::remove_ros_arguments(int argc, char const * const argv[]) if (RCL_RET_OK != ret) { // Not using throw_from_rcl_error, because we may need to append deallocation failures. - exceptions::RCLErrorBase base_exec(ret, rcl_get_error_state()); + exceptions::RCLErrorBase base_exc(ret, rcl_get_error_state()); rcl_reset_error(); if (NULL != nonros_argv) { alloc.deallocate(nonros_argv, alloc.state); } if (RCL_RET_OK != rcl_arguments_fini(&parsed_args)) { - base_exec.formatted_message += std::string( + base_exc.formatted_message += std::string( ", failed also to cleanup parsed arguments, leaking memory: ") + rcl_get_error_string_safe(); rcl_reset_error(); } - throw base_exec; + throw exceptions::RCLError(base_exc, ""); } std::vector return_arguments;