From 0af74fccda08fd09bd764af01f8ed4e00167ee2b Mon Sep 17 00:00:00 2001 From: Bananymous Date: Wed, 22 May 2024 20:17:39 +0300 Subject: [PATCH] Kernel/LibC: Rework dirent structure dirent now contains statically sized d_name. This allows using sizeof on the name and dirent properly, which some programs seem to be using. --- kernel/include/kernel/API/DirectoryEntry.h | 21 ------------ kernel/include/kernel/FS/Ext2/Inode.h | 2 +- kernel/include/kernel/FS/Inode.h | 8 ++--- kernel/include/kernel/FS/TmpFS/Inode.h | 2 +- kernel/include/kernel/OpenFileDescriptorSet.h | 2 +- kernel/include/kernel/Process.h | 2 +- kernel/kernel/FS/Ext2/Inode.cpp | 32 +++++++---------- kernel/kernel/FS/Inode.cpp | 2 +- kernel/kernel/FS/TmpFS/Inode.cpp | 27 +++++++-------- kernel/kernel/OpenFileDescriptorSet.cpp | 6 ++-- kernel/kernel/Process.cpp | 7 ++-- libc/dirent.cpp | 34 +++++++------------ libc/include/dirent.h | 3 +- 13 files changed, 52 insertions(+), 96 deletions(-) delete mode 100644 kernel/include/kernel/API/DirectoryEntry.h diff --git a/kernel/include/kernel/API/DirectoryEntry.h b/kernel/include/kernel/API/DirectoryEntry.h deleted file mode 100644 index e51d17cb..00000000 --- a/kernel/include/kernel/API/DirectoryEntry.h +++ /dev/null @@ -1,21 +0,0 @@ -#pragma once - -#include - -namespace Kernel::API -{ - - struct DirectoryEntry - { - size_t rec_len { 0 }; - struct dirent dirent; - DirectoryEntry* next() const { return (DirectoryEntry*)((uintptr_t)this + rec_len); } - }; - - struct DirectoryEntryList - { - size_t entry_count { 0 }; - DirectoryEntry array[]; - }; - -} diff --git a/kernel/include/kernel/FS/Ext2/Inode.h b/kernel/include/kernel/FS/Ext2/Inode.h index 4674ff81..8d8c3db8 100644 --- a/kernel/include/kernel/FS/Ext2/Inode.h +++ b/kernel/include/kernel/FS/Ext2/Inode.h @@ -31,7 +31,7 @@ namespace Kernel protected: virtual BAN::ErrorOr> find_inode_impl(BAN::StringView) override; - virtual BAN::ErrorOr list_next_inodes_impl(off_t, DirectoryEntryList*, size_t) override; + virtual BAN::ErrorOr list_next_inodes_impl(off_t, struct dirent*, size_t) override; virtual BAN::ErrorOr create_file_impl(BAN::StringView, mode_t, uid_t, gid_t) override; virtual BAN::ErrorOr create_directory_impl(BAN::StringView, mode_t, uid_t, gid_t) override; virtual BAN::ErrorOr unlink_impl(BAN::StringView) override; diff --git a/kernel/include/kernel/FS/Inode.h b/kernel/include/kernel/FS/Inode.h index bd581680..dca85c55 100644 --- a/kernel/include/kernel/FS/Inode.h +++ b/kernel/include/kernel/FS/Inode.h @@ -7,11 +7,11 @@ #include #include -#include #include #include #include +#include #include #include #include @@ -19,8 +19,6 @@ namespace Kernel { - using namespace API; - class FileBackedRegion; class SharedFileData; @@ -92,7 +90,7 @@ namespace Kernel // Directory API BAN::ErrorOr> find_inode(BAN::StringView); - BAN::ErrorOr list_next_inodes(off_t, DirectoryEntryList*, size_t); + BAN::ErrorOr list_next_inodes(off_t, struct dirent* list, size_t list_size); BAN::ErrorOr create_file(BAN::StringView, mode_t, uid_t, gid_t); BAN::ErrorOr create_directory(BAN::StringView, mode_t, uid_t, gid_t); BAN::ErrorOr unlink(BAN::StringView); @@ -127,7 +125,7 @@ namespace Kernel // Directory API virtual BAN::ErrorOr> find_inode_impl(BAN::StringView) { return BAN::Error::from_errno(ENOTSUP); } - virtual BAN::ErrorOr list_next_inodes_impl(off_t, DirectoryEntryList*, size_t) { return BAN::Error::from_errno(ENOTSUP); } + virtual BAN::ErrorOr list_next_inodes_impl(off_t, struct dirent*, size_t) { return BAN::Error::from_errno(ENOTSUP); } virtual BAN::ErrorOr create_file_impl(BAN::StringView, mode_t, uid_t, gid_t) { return BAN::Error::from_errno(ENOTSUP); } virtual BAN::ErrorOr create_directory_impl(BAN::StringView, mode_t, uid_t, gid_t) { return BAN::Error::from_errno(ENOTSUP); } virtual BAN::ErrorOr unlink_impl(BAN::StringView) { return BAN::Error::from_errno(ENOTSUP); } diff --git a/kernel/include/kernel/FS/TmpFS/Inode.h b/kernel/include/kernel/FS/TmpFS/Inode.h index 11763af0..4c0340eb 100644 --- a/kernel/include/kernel/FS/TmpFS/Inode.h +++ b/kernel/include/kernel/FS/TmpFS/Inode.h @@ -141,7 +141,7 @@ namespace Kernel protected: virtual BAN::ErrorOr> find_inode_impl(BAN::StringView) override final; - virtual BAN::ErrorOr list_next_inodes_impl(off_t, DirectoryEntryList*, size_t) override final; + virtual BAN::ErrorOr list_next_inodes_impl(off_t, struct dirent*, size_t) override final; virtual BAN::ErrorOr create_file_impl(BAN::StringView, mode_t, uid_t, gid_t) override final; virtual BAN::ErrorOr create_directory_impl(BAN::StringView, mode_t, uid_t, gid_t) override final; virtual BAN::ErrorOr unlink_impl(BAN::StringView) override; diff --git a/kernel/include/kernel/OpenFileDescriptorSet.h b/kernel/include/kernel/OpenFileDescriptorSet.h index c799fd22..504f4b7f 100644 --- a/kernel/include/kernel/OpenFileDescriptorSet.h +++ b/kernel/include/kernel/OpenFileDescriptorSet.h @@ -47,7 +47,7 @@ namespace Kernel BAN::ErrorOr read(int fd, BAN::ByteSpan); BAN::ErrorOr write(int fd, BAN::ConstByteSpan); - BAN::ErrorOr read_dir_entries(int fd, DirectoryEntryList* list, size_t list_size); + BAN::ErrorOr read_dir_entries(int fd, struct dirent* list, size_t list_len); BAN::ErrorOr path_of(int) const; BAN::ErrorOr> inode_of(int); diff --git a/kernel/include/kernel/Process.h b/kernel/include/kernel/Process.h index 5ebc52c5..226af759 100644 --- a/kernel/include/kernel/Process.h +++ b/kernel/include/kernel/Process.h @@ -149,7 +149,7 @@ namespace Kernel BAN::ErrorOr mount(BAN::StringView source, BAN::StringView target); - BAN::ErrorOr sys_readdir(int fd, DirectoryEntryList* buffer, size_t buffer_size); + BAN::ErrorOr sys_readdir(int fd, struct dirent* list, size_t list_len); BAN::ErrorOr sys_mmap(const sys_mmap_t*); BAN::ErrorOr sys_munmap(void* addr, size_t len); diff --git a/kernel/kernel/FS/Ext2/Inode.cpp b/kernel/kernel/FS/Ext2/Inode.cpp index 03f9a0d9..8bfb27dd 100644 --- a/kernel/kernel/FS/Ext2/Inode.cpp +++ b/kernel/kernel/FS/Ext2/Inode.cpp @@ -299,16 +299,13 @@ done: m_fs.delete_inode(ino()); } - BAN::ErrorOr Ext2Inode::list_next_inodes_impl(off_t offset, DirectoryEntryList* list, size_t list_size) + BAN::ErrorOr Ext2Inode::list_next_inodes_impl(off_t offset, struct dirent* list, size_t list_size) { ASSERT(mode().ifdir()); ASSERT(offset >= 0); if (static_cast>(offset) >= max_used_data_block_count()) - { - list->entry_count = 0; - return {}; - } + return 0; // FIXME: can we actually assume directories have all their blocks allocated const uint32_t block_index = fs_block_of_data_block_index(offset).value(); @@ -318,26 +315,25 @@ done: m_fs.read_block(block_index, block_buffer); // First determine if we have big enough list + size_t entry_count = 0; { BAN::ConstByteSpan entry_span = block_buffer.span(); - size_t needed_size = sizeof(DirectoryEntryList); while (entry_span.size() >= sizeof(Ext2::LinkedDirectoryEntry)) { auto& entry = entry_span.as(); if (entry.inode) - needed_size += sizeof(DirectoryEntry) + entry.name_len + 1; + entry_count++; entry_span = entry_span.slice(entry.rec_len); } - if (needed_size > list_size) - return BAN::Error::from_errno(EINVAL); + if (entry_count > list_size) + return BAN::Error::from_errno(ENOBUFS); } // Second fill the list { - DirectoryEntry* ptr = list->array; - list->entry_count = 0; + dirent* dirp = list; BAN::ConstByteSpan entry_span = block_buffer.span(); while (entry_span.size() >= sizeof(Ext2::LinkedDirectoryEntry)) @@ -345,20 +341,16 @@ done: auto& entry = entry_span.as(); if (entry.inode) { - ptr->dirent.d_ino = entry.inode; - ptr->dirent.d_type = entry.file_type; - ptr->rec_len = sizeof(DirectoryEntry) + entry.name_len + 1; - memcpy(ptr->dirent.d_name, entry.name, entry.name_len); - ptr->dirent.d_name[entry.name_len] = '\0'; - - ptr = ptr->next(); - list->entry_count++; + dirp->d_ino = entry.inode; + dirp->d_type = entry.file_type; + strncpy(dirp->d_name, entry.name, entry.name_len); + dirp++; } entry_span = entry_span.slice(entry.rec_len); } } - return {}; + return entry_count; } static bool mode_has_valid_type(mode_t mode) diff --git a/kernel/kernel/FS/Inode.cpp b/kernel/kernel/FS/Inode.cpp index b7341f0f..8dfcce20 100644 --- a/kernel/kernel/FS/Inode.cpp +++ b/kernel/kernel/FS/Inode.cpp @@ -70,7 +70,7 @@ namespace Kernel return find_inode_impl(name); } - BAN::ErrorOr Inode::list_next_inodes(off_t offset, DirectoryEntryList* list, size_t list_len) + BAN::ErrorOr Inode::list_next_inodes(off_t offset, struct dirent* list, size_t list_len) { LockGuard _(m_mutex); if (!mode().ifdir()) diff --git a/kernel/kernel/FS/TmpFS/Inode.cpp b/kernel/kernel/FS/TmpFS/Inode.cpp index 60adb594..3e6e3c99 100644 --- a/kernel/kernel/FS/TmpFS/Inode.cpp +++ b/kernel/kernel/FS/TmpFS/Inode.cpp @@ -432,9 +432,9 @@ namespace Kernel return BAN::RefPtr(inode); } - BAN::ErrorOr TmpDirectoryInode::list_next_inodes_impl(off_t data_block_index, DirectoryEntryList* list, size_t list_len) + BAN::ErrorOr TmpDirectoryInode::list_next_inodes_impl(off_t data_block_index, struct dirent* list, size_t list_len) { - if (list_len < (size_t)blksize()) + if (list_len == 0) { dprintln("buffer is too small"); return BAN::Error::from_errno(ENOBUFS); @@ -442,13 +442,12 @@ namespace Kernel auto block_index = this->block_index(data_block_index); - list->entry_count = 0; - // if we reach a non-allocated block, it marks the end if (!block_index.has_value()) - return {}; + return 0; - auto* dirp = list->array; + dirent* dirp = list; + size_t entry_count = 0; const size_t byte_count = BAN::Math::min(size() - data_block_index * blksize(), blksize()); m_fs.with_block_buffer(block_index.value(), [&](BAN::ByteSpan bytespan) { @@ -462,21 +461,21 @@ namespace Kernel { // TODO: dirents should be aligned - dirp->dirent.d_ino = entry.ino; - dirp->dirent.d_type = entry.type; - strncpy(dirp->dirent.d_name, entry.name, entry.name_len); - dirp->dirent.d_name[entry.name_len] = '\0'; - dirp->rec_len = sizeof(DirectoryEntry) + entry.name_len + 1; - dirp = dirp->next(); + entry_count++; + if (entry_count > list_len) + return; - list->entry_count++; + dirp->d_ino = entry.ino; + dirp->d_type = entry.type; + strncpy(dirp->d_name, entry.name, entry.name_len); + dirp++; } bytespan = bytespan.slice(entry.rec_len); } }); - return {}; + return entry_count; } BAN::ErrorOr TmpDirectoryInode::create_file_impl(BAN::StringView name, mode_t mode, uid_t uid, gid_t gid) diff --git a/kernel/kernel/OpenFileDescriptorSet.cpp b/kernel/kernel/OpenFileDescriptorSet.cpp index 0054fdf3..f1502ac1 100644 --- a/kernel/kernel/OpenFileDescriptorSet.cpp +++ b/kernel/kernel/OpenFileDescriptorSet.cpp @@ -351,15 +351,13 @@ namespace Kernel return nwrite; } - BAN::ErrorOr OpenFileDescriptorSet::read_dir_entries(int fd, DirectoryEntryList* list, size_t list_size) + BAN::ErrorOr OpenFileDescriptorSet::read_dir_entries(int fd, struct dirent* list, size_t list_len) { TRY(validate_fd(fd)); auto& open_file = m_open_files[fd]; if (!(open_file->flags & O_RDONLY)) return BAN::Error::from_errno(EACCES); - TRY(open_file->inode->list_next_inodes(open_file->offset, list, list_size)); - open_file->offset++; - return {}; + return TRY(open_file->inode->list_next_inodes(open_file->offset++, list, list_len)); } BAN::ErrorOr OpenFileDescriptorSet::path_of(int fd) const diff --git a/kernel/kernel/Process.cpp b/kernel/kernel/Process.cpp index 926e19af..7991bd40 100644 --- a/kernel/kernel/Process.cpp +++ b/kernel/kernel/Process.cpp @@ -1220,12 +1220,11 @@ namespace Kernel return clean_poweroff(command); } - BAN::ErrorOr Process::sys_readdir(int fd, DirectoryEntryList* list, size_t list_size) + BAN::ErrorOr Process::sys_readdir(int fd, struct dirent* list, size_t list_len) { LockGuard _(m_process_lock); - TRY(validate_pointer_access(list, list_size)); - TRY(m_open_file_descriptors.read_dir_entries(fd, list, list_size)); - return 0; + TRY(validate_pointer_access(list, sizeof(dirent) * list_len)); + return TRY(m_open_file_descriptors.read_dir_entries(fd, list, list_len)); } BAN::ErrorOr Process::sys_setpwd(const char* path) diff --git a/libc/dirent.cpp b/libc/dirent.cpp index 339162ac..d3c43299 100644 --- a/libc/dirent.cpp +++ b/libc/dirent.cpp @@ -5,16 +5,14 @@ #include #include -#include - struct __DIR { int fd { -1 }; + size_t entry_count { 0 }; size_t entry_index { 0 }; - Kernel::API::DirectoryEntry* current { nullptr }; - - size_t buffer_size { 0 }; - Kernel::API::DirectoryEntryList buffer[]; + // FIXME: we should probably allocate entries dynamically + // based if syscall returns ENOBUFS + dirent entries[128]; }; int closedir(DIR* dirp) @@ -26,7 +24,6 @@ int closedir(DIR* dirp) } close(dirp->fd); - dirp->fd = -1; free(dirp); return 0; @@ -45,13 +42,13 @@ int dirfd(DIR* dirp) DIR* fdopendir(int fd) { - DIR* dirp = (DIR*)malloc(sizeof(DIR) + 4096); + DIR* dirp = (DIR*)malloc(sizeof(DIR)); if (dirp == nullptr) return nullptr; dirp->fd = fd; - dirp->current = nullptr; - dirp->buffer_size = 4096; + dirp->entry_count = 0; + dirp->entry_index = 0; return dirp; } @@ -73,20 +70,15 @@ struct dirent* readdir(DIR* dirp) } dirp->entry_index++; - if (dirp->current && dirp->entry_index < dirp->buffer->entry_count) - { - dirp->current = dirp->current->next(); - return &dirp->current->dirent; - } + if (dirp->entry_index < dirp->entry_count) + return &dirp->entries[dirp->entry_index]; - if (syscall(SYS_READ_DIR, dirp->fd, dirp->buffer, dirp->buffer_size) == -1) - return nullptr; - - if (dirp->buffer->entry_count == 0) + long entry_count = syscall(SYS_READ_DIR, dirp->fd, dirp->entries, sizeof(dirp->entries) / sizeof(dirp->entries[0])); + if (entry_count <= 0) return nullptr; + dirp->entry_count = entry_count; dirp->entry_index = 0; - dirp->current = dirp->buffer->array; - return &dirp->current->dirent; + return &dirp->entries[0]; } diff --git a/libc/include/dirent.h b/libc/include/dirent.h index 64285b5c..d110c139 100644 --- a/libc/include/dirent.h +++ b/libc/include/dirent.h @@ -10,7 +10,6 @@ __BEGIN_DECLS #define __need_ino_t #include -struct __DIR; typedef struct __DIR DIR; #define DT_UNKNOWN 0 @@ -26,7 +25,7 @@ struct dirent { ino_t d_ino; /* File serial number. */ unsigned char d_type; /* File type. One of DT_* definitions. */ - char d_name[]; /* Filename string of entry. */ + char d_name[256]; /* Filename string of entry. */ }; int alphasort(const struct dirent** d1, const struct dirent** d2);