Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add rcutils_list_directory function #249

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions include/rcutils/filesystem.h
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ extern "C"

#include "rcutils/allocator.h"
#include "rcutils/macros.h"
#include "rcutils/types.h"
#include "rcutils/visibility_control.h"

/// Return current working directory.
Expand Down Expand Up @@ -187,6 +188,27 @@ RCUTILS_PUBLIC
size_t
rcutils_get_file_size(const char * file_path);

/// List the entries present in a directory.
/**
* This function lists the name of each file or directory present in the given
* directory in an implementation-specific order.
*
* \param[in] directory_path The directory path to list the contents of.
* \param[in] allocator Allocator being used for resources in the string_array.
* \param[out] string_array The object to store the entries into.
* \return `RCUTILS_RET_OK` if successful, or
* \return `RCUTILS_RET_INVALID_ARGUMENT` for invalid arguments, or
* \return `RCUTILS_RET_BAD_ALLOC` if memory allocation fails, or
* \return `RCUTILS_RET_ERROR` if an unknown error occurs
*/
RCUTILS_PUBLIC
RCUTILS_WARN_UNUSED
rcutils_ret_t
rcutils_list_directory(
const char * directory_path,
rcutils_allocator_t allocator,
rcutils_string_array_t * string_array);

#ifdef __cplusplus
}
#endif
Expand Down
89 changes: 88 additions & 1 deletion src/filesystem.c
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,9 @@ extern "C"
#endif // _WIN32

#include "rcutils/format_string.h"
#include "rcutils/logging_macros.h"
#include "rcutils/repl_str.h"
#include "rcutils/strdup.h"

#ifdef _WIN32
# define RCUTILS_PATH_DELIMITER "\\"
Expand Down Expand Up @@ -225,11 +227,11 @@ rcutils_calculate_directory_size(const char * directory_path, rcutils_allocator_
char * path = rcutils_join_path(directory_path, "*", allocator);
WIN32_FIND_DATA data;
HANDLE handle = FindFirstFile(path, &data);
allocator.deallocate(path, allocator.state);
if (INVALID_HANDLE_VALUE == handle) {
fprintf(stderr, "Can't open directory %s. Error code: %lu\n", directory_path, GetLastError());
return dir_size;
}
allocator.deallocate(path, allocator.state);
if (handle != INVALID_HANDLE_VALUE) {
do {
// Skip over local folder handle (`.`) and parent folder (`..`)
Expand Down Expand Up @@ -275,6 +277,91 @@ rcutils_get_file_size(const char * file_path)
return rc == 0 ? (size_t)(stat_buffer.st_size) : 0;
}

rcutils_ret_t
rcutils_list_directory(
const char * directory_path,
rcutils_allocator_t allocator,
rcutils_string_array_t * string_array)
{
RCUTILS_CHECK_FOR_NULL_WITH_MSG(
directory_path, "directory_path is null", return RCUTILS_RET_INVALID_ARGUMENT);
RCUTILS_CHECK_FOR_NULL_WITH_MSG(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rcutils_string_array_init also does this check.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fair. To be pedantic, the argument name could be different in that function so the error message might not be correct, but it might be worth the performance since this is likely to be a rare error case.

string_array, "string_array is null", return RCUTILS_RET_INVALID_ARGUMENT);

// Start with 8 entries
rcutils_ret_t ret = rcutils_string_array_init(string_array, 8, &allocator);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would do check size 1st in the directory, and the allocate array what we need and store the data. this reduces array adjustment operation. I think this is cost effective compared to allocate/de-allocate.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think my biggest problem with that is that it introduces a race with the filesystem. Entries could be removed or added during or after the initial enumeration and then our size would be wrong. To handle this, we'd essentially need to have all the code already here, plus the initial enumeration to give us the size hint.

I'm also not convinced that the performance would be better, but I could be convinced by some data on the topic.

if (RCUTILS_RET_OK != ret) {
return ret;
}

size_t count = 0;

#ifdef _WIN32
char * path = rcutils_join_path(directory_path, "*", allocator);
WIN32_FIND_DATA data;
HANDLE handle = FindFirstFile(path, &data);
allocator.deallocate(path, allocator.state);
if (INVALID_HANDLE_VALUE == handle) {
fprintf(stderr, "Can't open directory %s. Error code: %lu\n", directory_path, GetLastError());
ret = RCUTILS_RET_ERROR;
goto fail;
}
if (handle != INVALID_HANDLE_VALUE) {
do {
if (count >= string_array->size) {
ret = rcutils_string_array_resize(string_array, count * 2);
if (RCUTILS_RET_OK != ret) {
goto fail;
}
}

string_array->data[count] = rcutils_strdup(data.cFileName, allocator);
if (NULL == string_array->data[count]) {
goto fail;
}
} while (++count, FindNextFile(handle, &data));
FindClose(handle);
}
#else
DIR * dir = opendir(directory_path);
if (NULL == dir) {
fprintf(stderr, "Can't open directory %s. Error code: %d\n", directory_path, errno);
ret = RCUTILS_RET_ERROR;
goto fail;
}
struct dirent * entry;
for (; NULL != (entry = readdir(dir)); ++count) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

counting . and .. directory included, is this intended?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I felt that we can leave it up to the consumer whether they want to ignore those entries rather than performing the comparisons here. The API proposed in #323 mentions this behavior in the API docs.

if (count >= string_array->size) {
ret = rcutils_string_array_resize(string_array, count * 2);
if (RCUTILS_RET_OK != ret) {
goto fail;
}
}

string_array->data[count] = rcutils_strdup(entry->d_name, allocator);
if (NULL == string_array->data[count]) {
goto fail;
}
}
closedir(dir);
#endif

// Shrink the array back down
if (count != string_array->size) {
ret = rcutils_string_array_resize(string_array, count);
if (RCUTILS_RET_OK == ret) {
return RCUTILS_RET_OK;
}
}

fail:
if (RCUTILS_RET_OK != rcutils_string_array_fini(string_array)) {
RCUTILS_LOG_ERROR(
"failed to clean up on error (leaking memory): '%s'", rcutils_get_error_string().str);
}
return ret;
}

#ifdef __cplusplus
}
#endif
50 changes: 50 additions & 0 deletions test/test_filesystem.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,15 @@
#include <gtest/gtest.h>
#include <string>

#include "rcutils/error_handling.h"
#include "rcutils/filesystem.h"

#include "osrf_testing_tools_cpp/scope_exit.hpp"

#ifdef _WIN32
#define strdup _strdup
#endif

static rcutils_allocator_t g_allocator = rcutils_get_default_allocator();

class TestFilesystemFixture : public ::testing::Test
Expand Down Expand Up @@ -373,3 +378,48 @@ TEST_F(TestFilesystemFixture, calculate_file_size) {
g_allocator.deallocate(non_existing_path, g_allocator.state);
});
}

TEST_F(TestFilesystemFixture, list_directory) {
char * path =
rcutils_join_path(this->test_path, "dummy_folder", g_allocator);
ASSERT_NE(nullptr, path);

rcutils_string_array_t actual_contents;
rcutils_ret_t ret = rcutils_list_directory(nullptr, g_allocator, &actual_contents);
EXPECT_EQ(RCUTILS_RET_INVALID_ARGUMENT, ret);
rcutils_reset_error();

ret = rcutils_list_directory(path, g_allocator, nullptr);
EXPECT_EQ(RCUTILS_RET_INVALID_ARGUMENT, ret);
rcutils_reset_error();

ret = rcutils_list_directory(path, g_allocator, &actual_contents);
ASSERT_EQ(RCUTILS_RET_OK, ret);
OSRF_TESTING_TOOLS_CPP_SCOPE_EXIT(
{
if (RCUTILS_RET_OK != rcutils_string_array_fini(&actual_contents)) {
FAIL();
}
});

ret = rcutils_string_array_sort(&actual_contents);
ASSERT_EQ(RCUTILS_RET_OK, ret);

rcutils_string_array_t expected_contents;
ret = rcutils_string_array_init(&expected_contents, 3, &g_allocator);
ASSERT_EQ(RCUTILS_RET_OK, ret);
OSRF_TESTING_TOOLS_CPP_SCOPE_EXIT(
{
if (RCUTILS_RET_OK != rcutils_string_array_fini(&expected_contents)) {
FAIL();
}
});
expected_contents.data[0] = strdup(".");
expected_contents.data[1] = strdup("..");
expected_contents.data[2] = strdup("dummy.dummy");

int res;
ret = rcutils_string_array_cmp(&expected_contents, &actual_contents, &res);
ASSERT_EQ(RCUTILS_RET_OK, ret);
EXPECT_EQ(0, res);
}