add tests for, and clean up, common

rcl/CMakeLists.txt

@@ -89,6 +89,22 @@ if(AMENT_ENABLE_TESTING)
     set_target_properties(test_time PROPERTIES COMPILE_FLAGS "-std=c++11")
     target_link_libraries(test_time ${PROJECT_NAME} ${extra_test_libraries})
   endif()
+
+  ament_add_gtest(test_common
+    test/rcl/test_common.cpp
+    ENV
+      DYLD_INSERT_LIBRARIES=$<TARGET_FILE:${PROJECT_NAME}_memory_tools_interpose>
+      EMPTY_TEST=
+      NORMAL_TEST=foo
+  )
+  if(TARGET test_common)
+    target_include_directories(test_common PUBLIC
+      ${rcl_interfaces_INCLUDE_DIRS}
+      ${rmw_INCLUDE_DIRS}
+    )
+    set_target_properties(test_common PROPERTIES COMPILE_FLAGS "-std=c++11")
+    target_link_libraries(test_common ${PROJECT_NAME} ${extra_test_libraries})
+  endif()
 endif()
 
 ament_package()

rcl/src/rcl/common.c

@@ -22,12 +22,15 @@ extern "C"
 #include <stdlib.h>
 
 #if defined(WIN32)
-static char __env_buffer[1024];
+#define WINDOWS_ENV_BUFFER_SIZE 2048
+static char __env_buffer[WINDOWS_ENV_BUFFER_SIZE];
 #endif
 
 rcl_ret_t
-rcl_impl_getenv(const char * env_name, char ** env_value)
+rcl_impl_getenv(const char * env_name, const char ** env_value)
 {
+  RCL_CHECK_ARGUMENT_FOR_NULL(env_name, RCL_RET_INVALID_ARGUMENT);
+  RCL_CHECK_ARGUMENT_FOR_NULL(env_value, RCL_RET_INVALID_ARGUMENT);
   *env_value = NULL;
 #if !defined(WIN32)
   *env_value = getenv(env_name);
@@ -38,7 +41,8 @@ rcl_impl_getenv(const char * env_name, char ** env_value)
     RCL_SET_ERROR_MSG("value in env variable too large to read in");
     return RCL_RET_ERROR;
   }
-  env_value = __env_buffer;
+  __env_buffer[WINDOWS_ENV_BUFFER_SIZE - 1] = '\0';
+  *env_value = __env_buffer;
 #endif
   return RCL_RET_OK;
 }

rcl/src/rcl/common.h

@@ -31,9 +31,25 @@ extern "C"
     error_statement; \
 }
 
-// This value put in env_value is only valid until the next call to rcl_impl_getenv (on Windows).
+/// Retrieve the value of the given environment variable if it exists, or NULL.
+/* The returned char is only valid until the next time this function is called,
+ * because the returned char * is a direct pointer to the static storage.
+ * The returned value char * variable should never have free() called on it.
+ *
+ * Environment variable values will be truncated at 2048 characters on Windows.
+ *
+ * This function does not allocate heap memory, but the system calls might.
+ * This function is not thread-safe.
+ * This function is not lock-free.
+ *
+ * \param[in] env_name the name of the environment variable
+ * \param[out] env_value pointer to the value cstring
+ * \return RCL_RET_OK if the value is retrieved successfully, or
+ *         RCL_RET_INVALID_ARGUMENT if any arguments are invalid, or
+ *         RCL_RET_ERROR an unspecified error occur.
+ */
 rcl_ret_t
-rcl_impl_getenv(const char * env_name, char ** env_value);
+rcl_impl_getenv(const char * env_name, const char ** env_value);
 
 #if __cplusplus
 }

rcl/src/rcl/node.c

@@ -46,7 +46,7 @@ rcl_ret_t
 rcl_node_init(rcl_node_t * node, const char * name, const rcl_node_options_t * options)
 {
   size_t domain_id = 0;
-  char * ros_domain_id;
+  const char * ros_domain_id;
   rcl_ret_t ret;
   rcl_ret_t fail_ret = RCL_RET_ERROR;
   RCL_CHECK_ARGUMENT_FOR_NULL(name, RCL_RET_INVALID_ARGUMENT);

rcl/test/rcl/test_common.cpp

@@ -0,0 +1,51 @@
+// Copyright 2015 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.
+
+#include <gtest/gtest.h>
+
+#include <string>
+
+#include "../../src/rcl/common.h"
+
+/* Tests the default allocator.
+ *
+ * Expected environment variables must be set by the calling code:
+ *
+ *   - EMPTY_TEST=
+ *   - NORMAL_TEST=foo
+ *
+ * These are set in the call to `ament_add_gtest()` in the `CMakeLists.txt`.
+ */
+TEST(TestCommon, test_getenv) {
+  const char * env;
+  rcl_ret_t ret;
+  ret = rcl_impl_getenv("NORMAL_TEST", NULL);
+  EXPECT_EQ(ret, RCL_RET_INVALID_ARGUMENT);
+  rcl_reset_error();
+  ret = rcl_impl_getenv(NULL, &env);
+  EXPECT_EQ(ret, RCL_RET_INVALID_ARGUMENT);
+  rcl_reset_error();
+  ret = rcl_impl_getenv("SHOULD_NOT_EXIST_TEST", &env);
+  EXPECT_EQ(ret, RCL_RET_OK);
+  EXPECT_EQ(env, nullptr) << std::string(env);
+  rcl_reset_error();
+  ret = rcl_impl_getenv("NORMAL_TEST", &env);
+  EXPECT_EQ(ret, RCL_RET_OK);
+  ASSERT_NE(env, nullptr);
+  EXPECT_EQ(std::string(env), "foo");
+  ret = rcl_impl_getenv("EMPTY_TEST", &env);
+  EXPECT_EQ(ret, RCL_RET_OK);
+  ASSERT_NE(env, nullptr);
+  EXPECT_EQ(std::string(env), "");
+}