libc: add dirent tests by lukkrusz · Pull Request #475 · phoenix-rtos/phoenix-rtos-tests

lukkrusz · 2026-04-27T11:58:24Z

Adds a test coverage for dirent.h functions.

Description

The coverage includes:

readdir()
closedir()
opendir()
rewinddir()

Ensures the functions are POSIX compliant.

Motivation and Context

Ensures a fuller test coverage of libc functions

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Chore (refactoring, style fixes, git/CI config, submodule management, no code logic changes)

How Has This Been Tested?

Already covered by automatic testing.
New test added: (add PR link here).
Tested by hand on: ia32-generic-qemu.

Checklist:

My change requires a change to the documentation.
I have updated the documentation accordingly. (libc: update dirent documentation phoenix-rtos-doc#257)
I have added tests to cover my changes.
All new and existing linter checks and tests passed.
My changes generate no new compilation warnings for any of the targets.

Special treatment

This PR needs additional PRs to work (list the PRs, preferably in merge-order).
I will merge this PR by myself when appropriate.

gemini-code-assist

Code Review

This pull request introduces a comprehensive suite of POSIX-compliant tests for directory operations, including opendir, closedir, readdir, and rewinddir. The feedback identifies critical issues in the test implementation, specifically potential buffer overflows in readdir and closedir due to missing bounds checks and undersized buffers for directory names. Additionally, a redundant test definition was identified in the Makefile.

lukkrusz · 2026-04-27T13:08:32Z

/gemini review

gemini-code-assist

Code Review

This pull request introduces a comprehensive test suite for POSIX dirent.h functions, including opendir, closedir, readdir, and rewinddir. The review identified several critical issues: potential out-of-bounds access and buffer overflows in opendir.c, incorrect process handling after a failed fork() in readdir.c, and a potential NULL pointer dereference during cleanup in test_create_directories. Additionally, improvements were suggested for the d_ino_in helper to avoid false positives when checking inode numbers.

github-actions · 2026-04-27T13:23:31Z

Unit Test Results

11 307 tests +418 10 600 ✅ +381 54m 44s ⏱️ + 1m 1s
690 suites + 11 707 💤 + 37
1 files ± 0 0 ❌ ± 0

Results for commit 4818906. ± Comparison against base commit cf7a946.

♻️ This comment has been updated with latest results.

lukkrusz · 2026-04-27T13:56:30Z

/gemini review

gemini-code-assist

Code Review

This pull request introduces a comprehensive test suite for POSIX.1-2017 dirent.h functions, including opendir(), closedir(), readdir(), and rewinddir(). The tests cover various scenarios such as empty and non-empty directories, permission handling, symlink loops, and thread safety. Review feedback identifies a potential buffer overflow in a test error message, suggests containing test-generated directories within a managed path to ensure proper cleanup, recommends consistent use of assertion macros for directory creation, and proposes expanding CI target coverage to include architectures mentioned in the PR description.

lukkrusz · 2026-04-28T09:33:31Z

/gemini review

gemini-code-assist

Code Review

This pull request adds a comprehensive test suite for POSIX directory functions (opendir, closedir, readdir, and rewinddir) to the Phoenix-RTOS libc, along with helper macros and CI integration. Feedback focuses on improving code safety and readability by suggesting snprintf over sprintf, removing redundant type casts in memory management, and fixing a typo in the source comments.

mateusz-bloch · 2026-05-05T11:55:54Z

+#include <string.h>
+
+
+#define TEST_MKDIR_ASSERTED(path, mode) TEST_ASSERT_TRUE_MESSAGE(mkdir(path, mode) != -1 || errno == EEXIST, strerror(errno))


Uses EEXIST, but not include <errno.h>

Good catch. It's now included, thanks

wmlynik · 2026-06-05T12:32:14Z

+		sprintf(dirPath, MAIN_DIR "/%d", i);
+
+		TEST_MKDIR_ASSERTED(dirPath, S_IRUSR);
+	}


If the assertion fails, the test exits and previously created directories are not removed. TEST_TEAR_DOWN will not remove them since rmdir only removes empty directories. Similar issue in other tests.

wmlynik · 2026-06-05T12:35:38Z

+{
+	errno = 0;
+
+	TEST_ASSERT_EQUAL_INT(0, test_create_directories(20));


test_create_directories is only called here and that's the only thing this test does. Is there a reason to move the logic to a separate function rather than placing it in the test directly like in other tests?

Fixed, moved the code to the case where it should've been

wmlynik · 2026-06-05T12:40:07Z

+#include <unity.h>
+
+
+#define TEST_MKDIR_ASSERTED(path, mode) TEST_ASSERT_TRUE_MESSAGE(mkdir(path, mode) != -1 || errno == EEXIST, strerror(errno))


This will succeed if the path already exists, even if it's not a directory.

True, now it only passes if the directory is newly made - up for a debate, but I think the strict approach is better

wmlynik · 2026-06-05T13:08:40Z

+	strcpy(mutualLoop, MAIN_DIR "/D1");
+
+	/* Create a path to a valid symloop */
+	/* Posix says that symloops up to */


The comment seems incomplete.

wmlynik · 2026-06-05T13:16:14Z

+
+	/* Add a few layers of symloops, so it is too deep (selfLoop is not empty at this point) */
+	for (int i = 0; i < symloopMax / 2 - 1; ++i) {
+		if (strlen(selfLoop) + 10 >= PATH_MAX || strlen(mutualLoop) + 10 >= PATH_MAX) {


It's not clear where the number 10 comes from, maybe use a named constant or add a comment

wmlynik · 2026-06-05T13:45:43Z

+	TEST_MKDIR_ASSERTED(MAIN_DIR, 0700);
+
+	mkdir(MAIN_DIR "/dir1", S_IRUSR | S_IWUSR | S_IXUSR);
+	mkdir(MAIN_DIR "/dir2", S_IRUSR | S_IWUSR | S_IXUSR);
+
+	mkdir(MAIN_DIR "/dir1/nest1", S_IRUSR);
+	mkdir(MAIN_DIR "/dir1/nest2", S_IRUSR);
+
+	mkdir(MAIN_DIR "/dir2/nest1", S_IRUSR);
+	mkdir(MAIN_DIR "/dir2/nest2", S_IRUSR);
+
+	int files[] = {
+		creat(MAIN_DIR "/file1.txt", S_IRUSR),
+		creat(MAIN_DIR "/file2.dat", S_IRUSR),
+		creat(MAIN_DIR "/file3.json", S_IRUSR)
+	};
+
+	close(files[0]);
+	close(files[1]);
+	close(files[2]);


Success of MAIN_DIR creation is asserted but the rest of the setup my fail silently.

wmlynik · 2026-06-05T16:21:04Z

+	rewinddir(dp);
+
+	void *ret = readdir(dp);
+	(void)ret;


Why is the result of readdir after rewinddir not checked?

It was assumed to be NULL but now I put an assertion in place. Thanks

wmlynik · 2026-06-05T16:30:37Z

+	errno = 0;
+	TEST_MKDIR_ASSERTED(MAIN_DIR, S_IRWXU);
+	errno = 0;
+	TEST_MKDIR_ASSERTED(MAIN_DIR "/dir1", S_IRUSR);


Here errno is set before TEST_MKDIR_ASSERTED while in other test groups it's not. I don't think it's necessary here because the macro is only checking errno if mkdir fails.

Good point, fixed

JIRA: CI-359

Some tests needed refactoring and logic fixes, and proper clean-ups. There were also a couple of gaps in the coverage of the test suite that had to be filled. JIRA: CI-359 Assisted-by: Claude Opus 4.8

github-actions Bot reviewed Apr 27, 2026

View reviewed changes

Comment thread libc/dirent/opendir.c Outdated

Comment thread libc/dirent/opendir.c Outdated

Comment thread libc/dirent/opendir.c Outdated

Comment thread libc/dirent/opendir.c Outdated

gemini-code-assist Bot reviewed Apr 27, 2026

View reviewed changes

Comment thread libc/dirent/readdir.c Outdated

Comment thread libc/Makefile Outdated

Comment thread libc/dirent/closedir.c Outdated

lukkrusz force-pushed the lukkrusz/dirent_test branch from 6ffcd5b to 0bbefdc Compare April 27, 2026 12:32

github-actions Bot reviewed Apr 27, 2026

View reviewed changes

Comment thread libc/dirent/readdir.c Outdated

lukkrusz force-pushed the lukkrusz/dirent_test branch 2 times, most recently from 54efad0 to feaac4b Compare April 27, 2026 13:06

gemini-code-assist Bot reviewed Apr 27, 2026

View reviewed changes

lukkrusz force-pushed the lukkrusz/dirent_test branch from feaac4b to fb28d68 Compare April 27, 2026 13:51

gemini-code-assist Bot reviewed Apr 27, 2026

View reviewed changes

Comment thread libc/dirent/readdir.c Outdated

Comment thread libc/dirent/opendir.c Outdated

Comment thread libc/test.yaml

Comment thread libc/dirent/readdir.c Outdated

lukkrusz force-pushed the lukkrusz/dirent_test branch from fb28d68 to 5f5a34c Compare April 28, 2026 09:25

gemini-code-assist Bot reviewed Apr 28, 2026

View reviewed changes

lukkrusz force-pushed the lukkrusz/dirent_test branch 3 times, most recently from c3383bd to 69a6434 Compare April 28, 2026 12:42

lukkrusz marked this pull request as ready for review April 28, 2026 13:00

lukkrusz requested review from adamdebek and damianloew April 28, 2026 13:00

lukkrusz marked this pull request as draft April 28, 2026 13:08

lukkrusz force-pushed the lukkrusz/dirent_test branch from 69a6434 to f9e4718 Compare April 28, 2026 14:08

lukkrusz marked this pull request as ready for review April 29, 2026 08:04

mateusz-bloch reviewed May 5, 2026

View reviewed changes

This was referenced May 7, 2026

posix: add tests for dir family functions #477

Closed

posix: implement missing directory functions phoenix-rtos/libphoenix#433

Merged

lukkrusz force-pushed the lukkrusz/dirent_test branch from f9e4718 to 61debf2 Compare May 19, 2026 08:36

damianloew requested a review from wmlynik June 2, 2026 12:48

wmlynik reviewed Jun 8, 2026

View reviewed changes

lukkrusz force-pushed the lukkrusz/dirent_test branch from 61debf2 to adff8e7 Compare June 12, 2026 10:52

github-actions Bot reviewed Jun 12, 2026

View reviewed changes

Comment thread libc/dirent/opendir.c Outdated

lukkrusz force-pushed the lukkrusz/dirent_test branch from adff8e7 to 23ce160 Compare June 12, 2026 10:55

github-actions Bot reviewed Jun 12, 2026

View reviewed changes

Comment thread libc/dirent/rewinddir.c Outdated

Comment thread libc/dirent/rewinddir.c Outdated

Comment thread libc/dirent/rewinddir.c Outdated

lukkrusz marked this pull request as draft June 12, 2026 10:58

lukkrusz force-pushed the lukkrusz/dirent_test branch 2 times, most recently from 884f938 to 4371302 Compare June 12, 2026 13:42

lukkrusz marked this pull request as ready for review June 12, 2026 13:52

lukkrusz requested a review from wmlynik June 12, 2026 13:52

lukkrusz force-pushed the lukkrusz/dirent_test branch 2 times, most recently from 79aaff4 to 40f2647 Compare June 30, 2026 10:06

wmlynik approved these changes Jul 15, 2026

View reviewed changes

KArkadiusz and others added 2 commits July 16, 2026 09:49

libc: add dirent tests

4507241

JIRA: CI-359

libc: improve dirent tests

4818906

Some tests needed refactoring and logic fixes, and proper clean-ups. There were also a couple of gaps in the coverage of the test suite that had to be filled. JIRA: CI-359 Assisted-by: Claude Opus 4.8

lukkrusz force-pushed the lukkrusz/dirent_test branch from 40f2647 to 4818906 Compare July 16, 2026 07:49

lukkrusz merged commit d99e40e into master Jul 16, 2026
47 checks passed

lukkrusz deleted the lukkrusz/dirent_test branch July 16, 2026 08:07

		#include <string.h>


		#define TEST_MKDIR_ASSERTED(path, mode) TEST_ASSERT_TRUE_MESSAGE(mkdir(path, mode) != -1 \|\| errno == EEXIST, strerror(errno))

		#include <unity.h>


		#define TEST_MKDIR_ASSERTED(path, mode) TEST_ASSERT_TRUE_MESSAGE(mkdir(path, mode) != -1 \|\| errno == EEXIST, strerror(errno))

Uh oh!

Conversation

lukkrusz commented Apr 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

Types of changes

How Has This Been Tested?

Checklist:

Special treatment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lukkrusz commented Apr 27, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Apr 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Unit Test Results

Uh oh!

lukkrusz commented Apr 27, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lukkrusz commented Apr 28, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lukkrusz commented Apr 27, 2026 •

edited

Loading

github-actions Bot commented Apr 27, 2026 •

edited

Loading