[libc-commits] [PATCH] D147738: [libc] Enable linux directory entries syscalls in riscv64

[Michael Jones via Phabricator via libc-commits]

michaelrj added inline comments.


================
Comment at: libc/include/llvm-libc-types/struct_dirent.h:23
+#ifdef SYS_getdents64
+typedef ino64_t ino_t;
+typedef off64_t off_t;
----------------
sivachandra wrote:
> This type definition should live in `llvm-libc-types/ino_t.h`. Similar structuring for `off_t`;
since `ino64_t` and `ino_t` are both typedef'd to `uint64_t` now, this should be unnecessary. Same for `off64_t` and `off_t`.


================
Comment at: libc/src/__support/File/dir.cpp:50
+  // returns a slightly different dirent struct:
+  struct linux_dirent {
+    unsigned long d_ino;
----------------
mikhail.ramalho wrote:
> michaelrj wrote:
> > this struct should be moved to its own file in `include/llvm-libc-types/` to match `struct dirent`
> `struct linux_dirent` is kinda an oddball... glibc never defines it, see https://man7.org/linux/man-pages/man2/getdents.2.html:
> `Note: There is no definition of struct linux_dirent in glibc; see NOTES.`
> 
> The notes mention: 
> ```
>        Library support for getdents64() was added in glibc 2.30; Glibc
>        does not provide a wrapper for getdents(); call getdents() (or
>        getdents64() on earlier glibc versions) using syscall(2).  In
>        that case you will need to define the linux_dirent or
>        linux_dirent64 structure yourself.
> ```
> 
> Maybe I can add a comment just explaining that?
> 
> I also doesn't seem to be exposed by the kernel: https://elixir.bootlin.com/linux/v4.7/source/fs/readdir.c#L150
glibc not providing the struct publicly doesn't mean that we can't. I think it's cleaner to have the type defined in `llvm-libc-types` and then use `#ifdef` to include the appropriate struct.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147738/new/

https://reviews.llvm.org/D147738