posix: implement missing directory functions

[R4ken]

Implement missing POSIX functions (fdopendir,seekdir, telldir) working on DIR structs

JIRA: RTOS-1088

Description

Implemented fdopendir,seekdir and telldir functions.

Motivation and Context

Missing POSIX functions useful in porting applications

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, armv7a9-zynq7000-qemu)

Checklist:

• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• 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]

Summary of Changes

Hello @R4ken, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request introduces three essential POSIX directory functions: fdopendir, seekdir, and telldir. These additions are crucial for improving the system's compatibility with POSIX standards, thereby simplifying the porting of various applications that rely on these common directory operations. The changes enhance the system's file system interaction capabilities without altering existing functionality.

Highlights

• New POSIX Directory Functions: Implemented fdopendir, seekdir, and telldir functions to provide more comprehensive POSIX-compliant directory manipulation capabilities.
• Header Updates: Added declarations for the new directory functions in include/dirent.h.
• Core Logic Implementation: Provided the full implementation for fdopendir, seekdir, and telldir within unistd/dir.c, including necessary error handling and file descriptor management.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in issue comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request implements the missing POSIX directory functions fdopendir, seekdir, and telldir. The implementations are a good start, but I've identified a few high-severity issues related to null pointer dereferences and error handling that could lead to crashes or undefined behavior. I've provided detailed comments and code suggestions to address these points, which also include a small efficiency improvement in fdopendir.

[github-actions]

Unit Test Results

10 553 tests   9 891 ✅  52m 49s ⏱️
   651 suites    662 💤
     1 files        0 ❌

Results for commit 2aaa8f4.

♻️ This comment has been updated with latest results.

[Darchiv]

What if O_RDWR is set, so O_RDONLY is not? I think a file descriptor open for reading can be as well open for writing, as long as it is still readable

[R4ken]

open from libphoenix sets EISDIR on errno while opening directory with either O_WRONLY or O_RDWR flag.
Thus opening directory with O_RDWR is not possible

Fragment of open function from libphoenix

if (oflag & (O_WRONLY | O_RDWR)) {
  if ((err = stat(filename, &st)) < 0) {
	  if (errno != ENOENT)
		  return err;
  }
  else if (S_ISDIR(st.st_mode)) {
	  return SET_ERRNO(-EISDIR);
  }
}

[oI0ck]

This behaviour seems common, I can replicate it on both Linux and one of the BSD's, I think we can keep it like that for the sake of compatibility.

[Darchiv]

Is fd_flags & O_RDONLY) == 0 idiomatic? WHat about use of O_ACCMODE? Should we assume that O_RDONLY, O_WRONLY, and O_RDWR are bitmasks (0x1, 0x2, 0x4)? Or they are an enumeration (0x0, 0x1, 0x2) to be checked with O_ACCMODE == 0x3? (I'm not asking about how we or Linux implement it, but what is the most portable way.)

[oI0ck]

Addressed. Will create a new, linked PR in p-r-k.

[oI0ck]

Also, this check would be a no-op in enum-like access modes. It seems that it is here in this version to check if any mode is set (as rdonly is the default), on most systems O_RDONLY is defined as 0, so it is implicitly a default and there is no need to check it here.

[oI0ck]

I'd be good to introduce NULL checks while we are at it.

[oI0ck]

This behaviour seems common, I can replicate it on both Linux and one of the BSD's, I think we can keep it like that for the sake of compatibility.

[Darchiv]

Is fd_flags & O_RDONLY) == 0 idiomatic? WHat about use of O_ACCMODE? Should we assume that O_RDONLY, O_WRONLY, and O_RDWR are bitmasks (0x1, 0x2, 0x4)? Or they are an enumeration (0x0, 0x1, 0x2) to be checked with O_ACCMODE == 0x3? (I'm not asking about how we or Linux implement it, but what is the most portable way.)

[oI0ck]

@Darchiv

Is fd_flags & O_RDONLY) == 0 idiomatic? WHat about use of O_ACCMODE? Should we assume that O_RDONLY, O_WRONLY, and O_RDWR are bitmasks (0x1, 0x2, 0x4)? Or they are an enumeration (0x0, 0x1, 0x2) to be checked with O_ACCMODE == 0x3? (I'm not asking about how we or Linux implement it, but what is the most portable way.)

Most POSIX conformant/compilant operating systems seem to treat those values as enums, not bitfields.
I'll change it to enum-like values for the sake or portability.

[oI0ck]

I removed the bitfield commit as it required further changes. The dependency graph on the WAMR PR grows too deep and if we make every issue which has come up a hard dependency, we won't merge it ever.

[ziemleszcz]

mtReaddir already provides an offset (i.readdir.offs), so fdopendir() can use the current file offset. In any case, we should add tests to verify this behavior.

[oI0ck]

Yes, it does but this is not the problem, and it is not related to tests, just a problem I noticed. readdir() uses i.readdir.offs for sending dir stream offset to the filesystem server. The problem is that, as you've previously noticed, a directory stream returned by fdopendir() must retain a position/offset of a file descriptor passed to it.

We don't really need file descriptors here, readdir() works by sending messages to the filesystem server, but apart from that, it does not go through the file descriptor modifying 'proxy' syscalls, like write() or read() do.

My point is, with the way that we currently implement walking directory trees, lseek(fd, 0, SEEK_CUR) will ALWAYS return a 0 when fd refers to an open directory, because the offset is accounted for in the userspace, not the kernel.

In order for lseek() to work in this case, and for that matter, in order for position retaining in fdopendir() to work, we'd have to implement a new syscall, like getdents(), which would take a file descriptor, update its offset in the kernel and forward the mtReaddir message to the fs server.

[oI0ck]

So my question here is, do we want to add a new syscall here or shall we leave this functionality as-is and correct it later, possibly after the release.

[oI0ck]

Actually, this is a bit related to tests as it came out as an issue after I implemented a test for it :D

[ziemleszcz]

POSIX states: “The file offset associated with the file descriptor at the time of the call determines which entries are returned.”

So, the initial position of DIR should be set based on the file offset at the time of the fdopendir() call. After that, the file descriptor is under system control (“Upon successful return from fdopendir(), the file descriptor is under the control of the system”).

My understanding is that subsequent readdir() calls do not need to update the file descriptor’s offset. Only the internal DIR position should be advanced.

[ziemleszcz]

You could also check how glibc implements this, but in my opinion updating the file descriptor’s offset is not required by POSIX.

[oI0ck]

It is not required by POSIX, my point is that the state of the file descriptor for a directory will always be 0, so there is no need really to do an lseek() there.

[oI0ck]

We can probably regard this as a quirk of our implementation right now, I'm preparing a p-r-docs PR for this change so I'll mention it there.

[ziemleszcz]

If you mean calling lseek() in fdopendir(), then it is still required, since the current file position does not have to be 0. The user application may have already performed reads on this descriptor. It may seem unusual, but that is what POSIX specifies.

Please check how glibc handles this case (passing descriptor with a non-zero offset to fdopendir()).

[oI0ck]

I checked OpenBSD sources, they do set the lseek() at all times.

The user application may have already performed reads on this descriptor`\

Is performing a read() or write() on a directory file descriptor a valid operation? Do we, or the filesystem servers, ensure that, if it is possible to perform such odd operations, the offset stays valid? Do we even have a filesystem implementation that supports operations like these so we can test that behaviour?

We also have an inconsistency here, because fdopendir() does not make an assumption about the initial position value, but opendir() does, and it's zero. Moreover, because it does not interact with file descriptors at all, there is no way of determining the initial offset.

[oI0ck]

In this case, I think we might want to make opendir() also use file descriptors, so that the userspace won't make assumptions about the position, but gets it from lseek().

[ziemleszcz]

Directories are files, so reading them is fine. I assume the difference between how opendir() and fdopendir() set the initial position stems from this. Not sure how our file systems handle this (reading directories).
I propose to test it on Linux and RTOS:

• open directory,
• move offset past first entry (with known size),
• call fdopendir() + readdir(),
• expect second entry

[ziemleszcz]

For opendir() POSIX clearly states: “The directory stream is positioned at the first entry.”

[oI0ck]

Yep, I do have such test and it fails on RTOS, because we lack a way to report reading directories back to kernel.

I'm almost sure it would work on Linux though, we'd have to move it by using getdents() as most filesystems implement read() for directories as just returining -EISDIR.
getdents() would advance the internal offset of the file descriptor with a filesystem specific callback, so after fdopendir() and readdir() it should return the second entry, which does not happen on RTOS, again, because we lack a primitive, getdents(), to do so.

I've looked through our filesystems source code and you are in fact right, ext2 for some wicked reason implements both mtRead and mtReaddir on directories, so read() will work on a directory, advancing offset in the kernel (though, it's very hackish, me no like). I think FAT allows for it too.

I just think this behavour is confusing, semantically read()'ing from a directory FD is a bit odd (a function for reading contents of a file to a byte buffer? Unices don't treat directories as files or byte arrays. Directories are structured, files don't have to be), although there might be some uses.

In the end, we can leave it as-is for now, maybe implementing getdents() in the future would just make sense.
I'll correct these tests to use read()'s and launch only on targets with filesystems that do support read() on directories.

[oI0ck]

@ziemleszcz I see that while I had compiled some tests of my own, there is already a larger PR for dirent tests, phoenix-rtos/phoenix-rtos-tests#475. I think I can build up on their work by adding tests for new functions after it gets merged.

In the mean time, is this PR by itself clear to go in your opinion?