From 9068c77bea56ae6c6eacbbaf82919bfaa099a75f Mon Sep 17 00:00:00 2001
From: Scott K Logan <logans@cottsay.net>
Date: Thu, 14 May 2020 14:54:18 -0700
Subject: [PATCH] PR feedback

---
 README.md             |  5 ++++-
 include/rcutils/env.h | 14 +++++++++++++-
 src/env.c             |  3 +++
 3 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/README.md b/README.md
index 5c54504b..08655995 100644
--- a/README.md
+++ b/README.md
@@ -26,8 +26,11 @@ The API is a combination of parts:
 - A convenient string formatting function, which takes a custom allocator:
   - rcutils_format_string()
   - rcutils/format_string.h
-- A function to get an environment variable's value:
+- Functions for interfacing with process environment variables:
   - rcutils_get_env()
+  - rcutils_get_home_dir()
+  - rcutils_set_env()
+  - rcutils/env.h
   - rcutils/get_env.h
 - Extensible logging macros:
   - Some examples (not exhaustive):
diff --git a/include/rcutils/env.h b/include/rcutils/env.h
index 5931e9d2..8db1f73d 100644
--- a/include/rcutils/env.h
+++ b/include/rcutils/env.h
@@ -32,8 +32,20 @@ extern "C"
 /**
  * This function modifies the environment variables for the current process.
  *
+ * \par Thread Safety:
+ * This function is not thread-safe, particularly when called concurrently with
+ * ::rcutils_get_env. Take care not to modify the environment variables while
+ * another thread might be reading or writing environment variables.
+ *
+ * \par Platform Consistency:
+ * The behavior when setting a variable to an empty string (`""`) differs
+ * between platforms. On Windows, the variable is un-set (as if \p env_value was
+ * `NULL`), while on other platforms the variable is set to an empty string as
+ * expected.
+ *
  * \param[in] env_name Name of the environment variable to modify.
- * \param[in] env_value Value to set the environment variable to, or NULL to un-set.
+ * \param[in] env_value Value to set the environment variable to, or `NULL` to
+ *   un-set.
  * \return `True` if success
  * \return `False` if env_name is invalid or NULL
  * \return `False` on failure
diff --git a/src/env.c b/src/env.c
index f8a87a28..c815eacc 100644
--- a/src/env.c
+++ b/src/env.c
@@ -35,15 +35,18 @@ rcutils_set_env(const char * env_name, const char * env_value)
     env_value = "";
   }
   if (0 != _putenv_s(env_name, env_value)) {
+    RCUTILS_SET_ERROR_MSG_WITH_FORMAT_STRING("_putenv_s failed: %d", errno);
     return false;
   }
 #else
   if (NULL == env_value) {
     if (0 != unsetenv(env_name)) {
+      RCUTILS_SET_ERROR_MSG_WITH_FORMAT_STRING("unsetenv failed: %d", errno);
       return false;
     }
   } else {
     if (0 != setenv(env_name, env_value, 1)) {
+      RCUTILS_SET_ERROR_MSG_WITH_FORMAT_STRING("setenv failed: %d", errno);
       return false;
     }
   }