Address peer review comments

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

rcl/include/rcl/domain_id.h

@@ -30,20 +30,16 @@ extern "C"
 
 extern const char * const RCL_DOMAIN_ID_ENV_VAR;
 
-/// Determine the default domain id, based on the environment.
+/// Determine the default domain ID, based on the environment.
 /**
- * Checks the default domain id based on ROS_DOMAIN_ID environment variable, if
- * the original `domain_id` was RCL_DEFAULT_DOMAIN_ID.
- * If not, the input `domain_id` is not modified.
- *
- * \param[inout] domain_id Must not be NULL.
+ * \param[out] domain_id Must not be NULL.
  * \returns RCL_RET_INVALID_ARGUMENT if an argument is invalid, or,
  * \returns RCL_RET_ERROR in case of an unexpected error, or,
  * \returns RCL_RET_OK.
  */
 RCL_PUBLIC
 rcl_ret_t
-rcl_domain_id(size_t * domain_id);
+rcl_get_default_domain_id(size_t * domain_id);
 
 #ifdef __cplusplus
 }

rcl/include/rcl/localhost.h

@@ -28,18 +28,16 @@ extern const char * const RCL_LOCALHOST_ENV_VAR;
 
 /// Determine if the user wants to communicate using loopback only.
 /**
- * Checks if localhost should be used for network communication.
- * If `localhost_only` is RMW_LOCALHOST_ONLY_DEFAULT, ROS_LOCALHOST_ONLY env variable is used.
- * If not, `localhost_only` is not modified.
+ * Checks if localhost should be used for network communication based on environment.
  *
- * \param[inout] localhost_only Must not be NULL.
+ * \param[out] localhost_only Must not be NULL.
  * \returns RCL_RET_INVALID_ARGUMENT if an argument is invalid, or
  * \returns RCL_RET_ERROR if an unexpected error happened, or
  * \returns RCL_RET_OK.
  */
 RCL_PUBLIC
 rcl_ret_t
-rcl_localhost_only(rmw_localhost_only_t * localhost_only);
+rcl_get_localhost_only(rmw_localhost_only_t * localhost_only);
 
 #ifdef __cplusplus
 }

rcl/include/rcl/security.h

@@ -28,23 +28,23 @@ extern "C"
 #include "rmw/security_options.h"
 
 #ifndef ROS_SECURITY_NODE_DIRECTORY_VAR_NAME
-  #define ROS_SECURITY_NODE_DIRECTORY_VAR_NAME "ROS_SECURITY_NODE_DIRECTORY"
+# define ROS_SECURITY_NODE_DIRECTORY_VAR_NAME "ROS_SECURITY_NODE_DIRECTORY"
 #endif
 
 #ifndef ROS_SECURITY_ROOT_DIRECTORY_VAR_NAME
-  #define ROS_SECURITY_ROOT_DIRECTORY_VAR_NAME "ROS_SECURITY_ROOT_DIRECTORY"
+# define ROS_SECURITY_ROOT_DIRECTORY_VAR_NAME "ROS_SECURITY_ROOT_DIRECTORY"
 #endif
 
 #ifndef ROS_SECURITY_LOOKUP_TYPE_VAR_NAME
-  #define ROS_SECURITY_LOOKUP_TYPE_VAR_NAME "ROS_SECURITY_LOOKUP_TYPE"
+# define ROS_SECURITY_LOOKUP_TYPE_VAR_NAME "ROS_SECURITY_LOOKUP_TYPE"
 #endif
 
 #ifndef ROS_SECURITY_STRATEGY_VAR_NAME
-#define ROS_SECURITY_STRATEGY_VAR_NAME "ROS_SECURITY_STRATEGY"
+# define ROS_SECURITY_STRATEGY_VAR_NAME "ROS_SECURITY_STRATEGY"
 #endif
 
 #ifndef ROS_SECURITY_ENABLE_VAR_NAME
-#define ROS_SECURITY_ENABLE_VAR_NAME "ROS_SECURITY_ENABLE"
+# define ROS_SECURITY_ENABLE_VAR_NAME "ROS_SECURITY_ENABLE"
 #endif
 
 /// Init the security options from the values of the environment variables and passed names.
@@ -55,10 +55,10 @@ extern "C"
  *  \sa rcl_get_enforcement_policy
  *  \sa rcl_get_secure_root
  *
- * \param name[in] name used to find the securiy root path.
- * \param namespace_[in] namespace_ used to find the security root path.
- * \param allocator[in] used to do allocations.
- * \param security_options[out] security options that will be configured according to
+ * \param[in] name name used to find the securiy root path.
+ * \param[in] namespace_ namespace_ used to find the security root path.
+ * \param[in] allocator used to do allocations.
+ * \param[out] security_options security options that will be configured according to
  *  the environment.
  */
 RCL_PUBLIC
@@ -74,7 +74,7 @@ rcl_get_security_options_from_environment(
  * If `ROS_SECURITY_ENABLE` environment variable is set to "true", `use_security` will be set to
  * true.
  *
- * \param use_security[out] Must not be NULL.
+ * \param[out] use_security Must not be NULL.
  * \returns RCL_RET_INVALID_ARGUMENT if an argument is not valid, or
  * \returns RCL_RET_ERROR if an unexpected error happened, or
  * \returns RCL_RET_OK.
@@ -89,7 +89,7 @@ rcl_security_enabled(bool * use_security);
  * If `ROS_SECURITY_STRATEGY` is "Enforce", `policy` will be `RMW_SECURITY_ENFORCEMENT_ENFORCE`.
  * If not, `policy` will be `RMW_SECURITY_ENFORCEMENT_PERMISSIVE`.
  *
- * \param policy[out] Must not be NULL.
+ * \param[out] policy Must not be NULL.
  * \returns RCL_RET_INVALID_ARGUMENT if an argument is not valid, or
  * \returns RCL_RET_ERROR if an unexpected error happened, or
  * \returns RCL_RET_OK.

rcl/src/rcl/domain_id.c

@@ -24,7 +24,7 @@
 const char * const RCL_DOMAIN_ID_ENV_VAR = "ROS_DOMAIN_ID";
 
 rcl_ret_t
-rcl_domain_id(size_t * domain_id)
+rcl_get_default_domain_id(size_t * domain_id)
 {
   const char * ros_domain_id;
   const char * get_env_error_str;
@@ -33,10 +33,6 @@ rcl_domain_id(size_t * domain_id)
     return RCL_RET_INVALID_ARGUMENT;
   }
 
-  if (RCL_DEFAULT_DOMAIN_ID != *domain_id) {
-    return RCL_RET_OK;
-  }
-
   get_env_error_str = rcutils_get_env(RCL_DOMAIN_ID_ENV_VAR, &ros_domain_id);
   if (NULL != get_env_error_str) {
     RCL_SET_ERROR_MSG_WITH_FORMAT_STRING(

rcl/src/rcl/init.c

@@ -148,18 +148,25 @@ rcl_init(
   rcutils_atomic_store((atomic_uint_least64_t *)(&context->instance_id_storage), next_instance_id);
   context->impl->init_options.impl->rmw_init_options.instance_id = next_instance_id;
 
-  // Get actual domain id based on environment variable, if needed.
-  ret = rcl_domain_id(&context->impl->init_options.impl->rmw_init_options.domain_id);
-  if (RCL_RET_OK != ret) {
-    fail_ret = ret;
-    goto fail;
+  size_t * domain_id = &context->impl->init_options.impl->rmw_init_options.domain_id;
+  if (RCL_DEFAULT_DOMAIN_ID == *domain_id) {
+    // Get actual domain id based on environment variable.
+    ret = rcl_get_default_domain_id(domain_id);
+    if (RCL_RET_OK != ret) {
+      fail_ret = ret;
+      goto fail;
+    }
   }
 
-  // Get actual localhost_only value based on environment variable, if needed.
-  ret = rcl_localhost_only(&context->impl->init_options.impl->rmw_init_options.localhost_only);
-  if (RCL_RET_OK != ret) {
-    fail_ret = ret;
-    goto fail;
+  rmw_localhost_only_t * localhost_only =
+    &context->impl->init_options.impl->rmw_init_options.localhost_only;
+  if (RMW_LOCALHOST_ONLY_DEFAULT == *localhost_only) {
+    // Get actual localhost_only value based on environment variable, if needed.
+    ret = rcl_get_localhost_only(localhost_only);
+    if (RCL_RET_OK != ret) {
+      fail_ret = ret;
+      goto fail;
+    }
   }
 
   if (!rmw_use_node_name_in_security_directory_lookup()) {
@@ -186,7 +193,6 @@ rcl_init(
   }
 
   TRACEPOINT(rcl_init, (const void *)context);
-
   return RCL_RET_OK;
 fail:
   __cleanup_context(context);

rcl/src/rcl/localhost.c

@@ -25,7 +25,7 @@
 const char * const RCL_LOCALHOST_ENV_VAR = "ROS_LOCALHOST_ONLY";
 
 rcl_ret_t
-rcl_localhost_only(rmw_localhost_only_t * localhost_only)
+rcl_get_localhost_only(rmw_localhost_only_t * localhost_only)
 {
   const char * ros_local_host_env_val = NULL;
   const char * get_env_error_str = NULL;
@@ -34,10 +34,6 @@ rcl_localhost_only(rmw_localhost_only_t * localhost_only)
     return RCL_RET_INVALID_ARGUMENT;
   }
 
-  if (RMW_LOCALHOST_ONLY_DEFAULT == *localhost_only) {
-    return RCL_RET_OK;
-  }
-
   get_env_error_str = rcutils_get_env(RCL_LOCALHOST_ENV_VAR, &ros_local_host_env_val);
   if (NULL != get_env_error_str) {
     RCL_SET_ERROR_MSG_WITH_FORMAT_STRING(

rcl/src/rcl/node.c

@@ -259,8 +259,10 @@ rcl_node_init(
     node->impl->logger_name, "creating logger name failed", goto fail);
 
   domain_id = node->impl->options.domain_id;
-  if (RCL_RET_OK != rcl_domain_id(&domain_id)) {
-    goto fail;
+  if (RCL_DEFAULT_DOMAIN_ID == domain_id) {
+    if (RCL_RET_OK != rcl_get_default_domain_id(&domain_id)) {
+      goto fail;
+    }
   }
   RCUTILS_LOG_DEBUG_NAMED(ROS_PACKAGE_NAME, "Using domain ID of '%zu'", domain_id);
   node->impl->actual_domain_id = domain_id;
@@ -277,8 +279,10 @@ rcl_node_init(
     }
   }
 
-  if (RMW_RET_OK != rcl_localhost_only(&localhost_only)) {
-    goto fail;
+  if (RMW_LOCALHOST_ONLY_DEFAULT == localhost_only) {
+    if (RMW_RET_OK != rcl_get_localhost_only(&localhost_only)) {
+      goto fail;
+    }
   }
 
   node->impl->rmw_node_handle = rmw_create_node(

rcl/src/rcl/security.c

@@ -48,31 +48,31 @@ rcl_get_security_options_from_environment(
   if (RCL_RET_OK != ret) {
     return ret;
   }
+
+  if (!use_security) {
+    security_options->enforce_security = RMW_SECURITY_ENFORCEMENT_PERMISSIVE;
+    return RMW_RET_OK;
+  }
+
   RCUTILS_LOG_DEBUG_NAMED(
     ROS_PACKAGE_NAME, "Using security: %s", use_security ? "true" : "false");
 
-  if (!rmw_use_node_name_in_security_directory_lookup()) {
-    ret = rcl_get_enforcement_policy(&security_options->enforce_security);
-    if (RCL_RET_OK != ret) {
-      return ret;
-    }
+  ret = rcl_get_enforcement_policy(&security_options->enforce_security);
+  if (RCL_RET_OK != ret) {
+    return ret;
+  }
 
-    if (!use_security) {
-      security_options->enforce_security = RMW_SECURITY_ENFORCEMENT_PERMISSIVE;
-    } else {  // if use_security
-      // File discovery magic here
-      char * secure_root = rcl_get_secure_root(
-        name,
-        namespace_,
-        allocator);
-      if (secure_root) {
-        RCUTILS_LOG_INFO_NAMED(ROS_PACKAGE_NAME, "Found security directory: %s", secure_root);
-        security_options->security_root_path = secure_root;
-      } else {
-        if (RMW_SECURITY_ENFORCEMENT_ENFORCE == security_options->enforce_security) {
-          return RCL_RET_ERROR;
-        }
-      }
+  // File discovery magic here
+  char * secure_root = rcl_get_secure_root(
+    name,
+    namespace_,
+    allocator);
+  if (secure_root) {
+    RCUTILS_LOG_INFO_NAMED(ROS_PACKAGE_NAME, "Found security directory: %s", secure_root);
+    security_options->security_root_path = secure_root;
+  } else {
+    if (RMW_SECURITY_ENFORCEMENT_ENFORCE == security_options->enforce_security) {
+      return RCL_RET_ERROR;
     }
   }
   return RCL_RET_OK;
@@ -84,9 +84,7 @@ rcl_security_enabled(bool * use_security)
   const char * ros_security_enable = NULL;
   const char * get_env_error_str = NULL;
 
-  if (!use_security) {
-    return RCL_RET_INVALID_ARGUMENT;
-  }
+  RCL_CHECK_ARGUMENT_FOR_NULL(use_security, RCL_RET_INVALID_ARGUMENT);
 
   get_env_error_str = rcutils_get_env(ROS_SECURITY_ENABLE_VAR_NAME, &ros_security_enable);
   if (NULL != get_env_error_str) {
@@ -106,9 +104,7 @@ rcl_get_enforcement_policy(rmw_security_enforcement_policy_t * policy)
   const char * ros_enforce_security = NULL;
   const char * get_env_error_str = NULL;
 
-  if (!policy) {
-    return RCL_RET_INVALID_ARGUMENT;
-  }
+  RCL_CHECK_ARGUMENT_FOR_NULL(policy, RCL_RET_INVALID_ARGUMENT);
 
   get_env_error_str = rcutils_get_env(ROS_SECURITY_STRATEGY_VAR_NAME, &ros_enforce_security);
   if (NULL != get_env_error_str) {
@@ -126,7 +122,7 @@ rcl_get_enforcement_policy(rmw_security_enforcement_policy_t * policy)
 /**
  * A security lookup function takes in the node/context's name, namespace, a security root directory and an allocator;
  *  It returns the relevant information required to load the security credentials,
- *   which is currently a path to a directory on the filesystem containing DDS Security permission files.
+ *  which is currently a path to a directory on the filesystem containing DDS Security permission files.
  */
 typedef char * (* security_lookup_fn_t) (
   const char * name,
@@ -168,7 +164,7 @@ const char * const g_security_lookup_type_strings[] = {
   "MATCH_PREFIX"
 };
 
-/// Return the directory whose name most closely matches name (longest-prefix match),
+/// Return the directory that most closely matches a given name (longest-prefix match),
 /// scanning under base_dir.
 /**
  * By using a prefix match, a name "my_name_123" will be able to load and use the