Fix filesystem iteration on Windows (#469)

* Get rid of unnecessary casts in rcutils_dir_iter_t. Instead of using a void pointer, use the forward-declared pointer type. While this won't make a difference for the generated code, it will let us remove unnecessary casts. * Remove unnecessary FindClose on error. In particular, if we failed to find the next file on Windows, we shouldn't necessarily close the handle; that's the job of rcutils_dir_iter_end. * Remove completely unnecessary #ifdef __cplusplus header. Signed-off-by: Chris Lalancette <[email protected]>
ros2 · Jun 4, 2024 · 8f5d6bc · 8f5d6bc
1 parent ebb174d
commit 8f5d6bc
Show file tree

Hide file tree

Showing 2 changed files with 23 additions and 32 deletions.
diff --git a/include/rcutils/filesystem.h b/include/rcutils/filesystem.h
@@ -245,6 +245,8 @@ RCUTILS_PUBLIC
 size_t
 rcutils_get_file_size(const char * file_path);
 
+struct rcutils_dir_iter_state_s;
+
 /// An iterator used for enumerating directory contents
 typedef struct rcutils_dir_iter_s
 {
@@ -253,7 +255,7 @@ typedef struct rcutils_dir_iter_s
   /// The allocator used internally by iteration functions
   rcutils_allocator_t allocator;
   /// The platform-specific iteration state
-  void * state;
+  struct rcutils_dir_iter_state_s * state;
 } rcutils_dir_iter_t;
 
 /// Begin iterating over the contents of the specified directory.

diff --git a/src/filesystem.c b/src/filesystem.c
@@ -12,10 +12,6 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-#ifdef __cplusplus
-extern "C"
-{
-#endif
 #include "rcutils/filesystem.h"
 
 #include <errno.h>
@@ -54,7 +50,7 @@ extern "C"
 # define RCUTILS_PATH_DELIMITER "/"
 #endif  // _WIN32
 
-typedef struct rcutils_dir_iter_state_t
+typedef struct rcutils_dir_iter_state_s
 {
 #ifdef _WIN32
   HANDLE handle;
@@ -417,49 +413,48 @@ rcutils_dir_iter_start(const char * directory_path, const rcutils_allocator_t al
   RCUTILS_CHECK_ALLOCATOR_WITH_MSG(
     &allocator, "allocator is invalid", return NULL);
 
-  rcutils_dir_iter_t * iter = (rcutils_dir_iter_t *)allocator.zero_allocate(
+  rcutils_dir_iter_t * iter = allocator.zero_allocate(
     1, sizeof(rcutils_dir_iter_t), allocator.state);
   if (NULL == iter) {
     return NULL;
   }
   iter->allocator = allocator;
 
-  rcutils_dir_iter_state_t * state = (rcutils_dir_iter_state_t *)allocator.zero_allocate(
+  iter->state = allocator.zero_allocate(
     1, sizeof(rcutils_dir_iter_state_t), allocator.state);
-  if (NULL == state) {
+  if (NULL == iter->state) {
     RCUTILS_SET_ERROR_MSG(
       "Failed to allocate memory.\n");
     goto rcutils_dir_iter_start_fail;
   }
-  iter->state = (void *)state;
 
 #ifdef _WIN32
   char * search_path = rcutils_join_path(directory_path, "*", allocator);
   if (NULL == search_path) {
     goto rcutils_dir_iter_start_fail;
   }
-  state->handle = FindFirstFile(search_path, &state->data);
+  iter->state->handle = FindFirstFile(search_path, &iter->state->data);
   allocator.deallocate(search_path, allocator.state);
-  if (INVALID_HANDLE_VALUE == state->handle) {
+  if (INVALID_HANDLE_VALUE == iter->state->handle) {
     DWORD error = GetLastError();
     if (ERROR_FILE_NOT_FOUND != error || !rcutils_is_directory(directory_path)) {
       RCUTILS_SET_ERROR_MSG_WITH_FORMAT_STRING(
         "Can't open directory %s. Error code: %d\n", directory_path, error);
       goto rcutils_dir_iter_start_fail;
     }
   } else {
-    iter->entry_name = state->data.cFileName;
+    iter->entry_name = iter->state->data.cFileName;
   }
 #else
-  state->dir = opendir(directory_path);
-  if (NULL == state->dir) {
+  iter->state->dir = opendir(directory_path);
+  if (NULL == iter->state->dir) {
     RCUTILS_SET_ERROR_MSG_WITH_FORMAT_STRING(
       "Can't open directory %s. Error code: %d\n", directory_path, errno);
     goto rcutils_dir_iter_start_fail;
   }
 
   errno = 0;
-  struct dirent * entry = readdir(state->dir);
+  struct dirent * entry = readdir(iter->state->dir);
   if (NULL != entry) {
     iter->entry_name = entry->d_name;
   } else if (0 != errno) {
@@ -480,17 +475,15 @@ bool
 rcutils_dir_iter_next(rcutils_dir_iter_t * iter)
 {
   RCUTILS_CHECK_ARGUMENT_FOR_NULL(iter, false);
-  rcutils_dir_iter_state_t * state = (rcutils_dir_iter_state_t *)iter->state;
-  RCUTILS_CHECK_FOR_NULL_WITH_MSG(state, "iter is invalid", false);
+  RCUTILS_CHECK_FOR_NULL_WITH_MSG(iter->state, "iter is invalid", false);
 
 #ifdef _WIN32
-  if (FindNextFile(state->handle, &state->data)) {
-    iter->entry_name = state->data.cFileName;
+  if (FindNextFile(iter->state->handle, &iter->state->data)) {
+    iter->entry_name = iter->state->data.cFileName;
     return true;
   }
-  FindClose(state->handle);
 #else
-  struct dirent * entry = readdir(state->dir);
+  struct dirent * entry = readdir(iter->state->dir);
   if (NULL != entry) {
     iter->entry_name = entry->d_name;
     return true;
@@ -511,17 +504,17 @@ rcutils_dir_iter_end(rcutils_dir_iter_t * iter)
   rcutils_allocator_t allocator = iter->allocator;
   RCUTILS_CHECK_ALLOCATOR_WITH_MSG(
     &allocator, "allocator is invalid", return );
-  rcutils_dir_iter_state_t * state = (rcutils_dir_iter_state_t *)iter->state;
-  if (NULL != state) {
+
+  if (NULL != iter->state) {
 #ifdef _WIN32
-    FindClose(state->handle);
+    FindClose(iter->state->handle);
 #else
-    if (NULL != state->dir) {
-      closedir(state->dir);
+    if (NULL != iter->state->dir) {
+      closedir(iter->state->dir);
     }
 #endif
 
-    allocator.deallocate(state, allocator.state);
+    allocator.deallocate(iter->state, allocator.state);
   }
 
   allocator.deallocate(iter, allocator.state);
@@ -540,7 +533,3 @@ rcutils_get_file_size(const char * file_path)
   int rc = stat(file_path, &stat_buffer);
   return rc == 0 ? (size_t)(stat_buffer.st_size) : 0;
 }
-
-#ifdef __cplusplus
-}
-#endif