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

dirent.h: Add d_ino member to struct dirent #15023

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tmedicci
Copy link
Contributor

@tmedicci tmedicci commented Dec 2, 2024

Summary

  • dirent.h: Add d_ino member to struct dirent

This commit adds the d_ino member (ino_t type) to struct dirent to make it compatible with the POSIX definition of the structure.
According to https://pubs.opengroup.org/onlinepubs/9799919799/, the structure dirent shall include the following members:

ino_t  d_ino       File serial number.
char   d_name[]    Filename string of entry.

https://www.man7.org/linux/man-pages/man3/readdir.3.html also states that:

"
Only the fields d_name and (as an XSI extension) d_ino are
specified in POSIX.1. Other than Linux, the d_type field is
available mainly only on BSD systems. The remaining fields are
available on many, but not all systems.
"

Although d_ino isn't being used by NuttX directly, the structure dirent may be used by third-party applications and it's important
to have all the required members defined to avoid compatibility issues.

Impact

No direct impact for NuttX usage, but allows third-party apps to refer to the struct dirent's member correctly.

Testing

Internal CI testing + NuttX CI

@github-actions github-actions bot added Area: OS Components OS Components issues Size: XS The size of the change in this PR is very small labels Dec 2, 2024
This commit adds the `d_ino` member (`ino_t` type) to struct dirent
to make it compatible with the POSIX definition of the structure.
According to https://pubs.opengroup.org/onlinepubs/9799919799/, the
structure `dirent` shall include the following members:

```
ino_t  d_ino       File serial number.
char   d_name[]    Filename string of entry.
```

https://www.man7.org/linux/man-pages/man3/readdir.3.html also
states that:

"
Only the fields d_name and (as an XSI extension) d_ino are
specified in POSIX.1.  Other than Linux, the d_type field is
available mainly only on BSD systems.  The remaining fields are
available on many, but not all systems.
"

Although `d_ino` isn't being used by NuttX directly, the structure
`dirent` may be used by third-party applications and it's important
to have all the required members defined to avoid compatibility
issues.
@tmedicci tmedicci force-pushed the improvement/struct_dirent branch from 9259ddc to 96cd2e3 Compare December 2, 2024 19:34
@github-actions github-actions bot added the Area: File System File System issues label Dec 2, 2024
@tmedicci
Copy link
Contributor Author

tmedicci commented Dec 2, 2024

Just tested locally and the CI error at https://github.com/apache/nuttx/actions/runs/12126636460/job/33809240627?pr=15023#step:7:126 happens on master (3a4b8ed). It doesn't seem to be related to this PR...

Copy link
Contributor

@yamt yamt left a comment

Choose a reason for hiding this comment

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

cf. #13556

if we add the field, i guess we should set a sane value.
at least it should be consistent with st_ino.

honestly speaking, i'm not sure if it's worth to implement d_ino for nuttx.
as the most filesystems here don't implement inode numbers at all,
applications using d_ino likely need modifications to deal with the situation anyway.

@Donny9
Copy link
Contributor

Donny9 commented Dec 3, 2024

The d_ino in the dirent structure and the st_ino in thestatstructure are both inode numbers, which are not supported by NuttX unless it has the capability, like Linux, to assign a unique inode to each file. When we internally adapt to third-party systems, we usually distinguish different files or directories by their filenames, rather than relying on st_dev and st_ino as in Linux.

@xiaoxiang781216
Copy link
Contributor

xiaoxiang781216 commented Dec 3, 2024

some filesystem(nfs, hostfs, 9p etc) can really implement ino, so it make sense to add the basic support in vfs layer.
BTW, stat already include st_ino field, it's natural to add d_ino to dirent.

@yamt
Copy link
Contributor

yamt commented Dec 3, 2024

some filesystem(nfs, hostfs, 9p etc) can really implement ino, so it make sense to add the basic support in vfs layer. BTW, stat already include st_ino field, it's natural to add d_ino to dirent.

if you think it's useful, i don't object.
however, i'd suggest to actually implement it, instead of just adding a dummy field as this PR.

@xiaoxiang781216
Copy link
Contributor

xiaoxiang781216 commented Dec 3, 2024

some filesystem(nfs, hostfs, 9p etc) can really implement ino, so it make sense to add the basic support in vfs layer. BTW, stat already include st_ino field, it's natural to add d_ino to dirent.

if you think it's useful, i don't object. however, i'd suggest to actually implement it, instead of just adding a dummy field as this PR.

Yes, we should zero this field for the unsupported file system and implement it at least for hostfs.

@tmedicci
Copy link
Contributor Author

tmedicci commented Dec 5, 2024

some filesystem(nfs, hostfs, 9p etc) can really implement ino, so it make sense to add the basic support in vfs layer. BTW, stat already include st_ino field, it's natural to add d_ino to dirent.

if you think it's useful, i don't object. however, i'd suggest to actually implement it, instead of just adding a dummy field as this PR.

Yes, we should zero this field for the unsupported file system and implement it at least for hostfs.

I proposed thir PR to enable building Python (apache/nuttx-apps#2879). I was able to make it work without it (same case of #13556), so this PR is not that necessary, although I might be able to work on it soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: File System File System issues Area: OS Components OS Components issues Size: XS The size of the change in this PR is very small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants