From 4a57be824cc6d4fed95f182431ba807b2327395e Mon Sep 17 00:00:00 2001 From: Kenneth Myhra Date: Wed, 23 Mar 2022 16:36:03 +0100 Subject: [PATCH] Userland+Tests: Convert File::read_link() from String to ErrorOr This converts the return value of File::read_link() from String to ErrorOr. The rest of the change is to support the potential of an Error being returned and subsequent release of the value when no Error is returned. Unfortunately at this stage none of the places affected can utililize our TRY() macro. --- Tests/Kernel/TestKernelFilePermissions.cpp | 5 +++- Tests/LibC/TestIo.cpp | 5 +++- Tests/LibC/TestLibCMkTemp.cpp | 10 ++++++-- Tests/LibELF/test-elf.cpp | 25 +++++++++++++++---- .../FileManager/PropertiesWindow.cpp | 5 ++-- Userland/Libraries/LibCore/File.cpp | 20 +++++++-------- Userland/Libraries/LibCore/File.h | 2 +- .../Libraries/LibGUI/FileIconProvider.cpp | 6 ++++- Userland/Libraries/LibGUI/FileSystemModel.cpp | 6 ++++- Userland/Services/LaunchServer/Launcher.cpp | 6 ++++- Userland/Utilities/ls.cpp | 6 ++--- Userland/Utilities/readlink.cpp | 6 +++-- 12 files changed, 72 insertions(+), 30 deletions(-) diff --git a/Tests/Kernel/TestKernelFilePermissions.cpp b/Tests/Kernel/TestKernelFilePermissions.cpp index fa1edef211b..37eb2aa145d 100644 --- a/Tests/Kernel/TestKernelFilePermissions.cpp +++ b/Tests/Kernel/TestKernelFilePermissions.cpp @@ -81,7 +81,10 @@ TEST_CASE(test_change_file_location) ftruncate(fd, 0); EXPECT(fchmod(fd, 06755) != -1); - auto suid_path = Core::File::read_link(String::formatted("/proc/{}/fd/{}", getpid(), fd)); + auto suid_path_or_error = Core::File::read_link(String::formatted("/proc/{}/fd/{}", getpid(), fd)); + EXPECT(!suid_path_or_error.is_error()); + + auto suid_path = suid_path_or_error.release_value(); EXPECT(suid_path.characters()); auto new_path = String::formatted("{}.renamed", suid_path); diff --git a/Tests/LibC/TestIo.cpp b/Tests/LibC/TestIo.cpp index 26083fd1eda..bd47d7d6c82 100644 --- a/Tests/LibC/TestIo.cpp +++ b/Tests/LibC/TestIo.cpp @@ -216,7 +216,10 @@ TEST_CASE(unlink_symlink) perror("symlink"); } - auto target = Core::File::read_link("/tmp/linky"); + auto target_or_error = Core::File::read_link("/tmp/linky"); + EXPECT(!target_or_error.is_error()); + + auto target = target_or_error.release_value(); EXPECT_EQ(target, "/proc/2/foo"); rc = unlink("/tmp/linky"); diff --git a/Tests/LibC/TestLibCMkTemp.cpp b/Tests/LibC/TestLibCMkTemp.cpp index 5045640a869..03d377642b8 100644 --- a/Tests/LibC/TestLibCMkTemp.cpp +++ b/Tests/LibC/TestLibCMkTemp.cpp @@ -86,7 +86,10 @@ TEST_CASE(test_mkstemp_unique_filename) auto fd = mkstemp(path); EXPECT_NE(fd, -1); - auto temp_path = Core::File::read_link(String::formatted("/proc/{}/fd/{}", getpid(), fd)); + auto temp_path_or_error = Core::File::read_link(String::formatted("/proc/{}/fd/{}", getpid(), fd)); + EXPECT(!temp_path_or_error.is_error()); + + auto temp_path = temp_path_or_error.release_value(); EXPECT(temp_path.characters()); close(fd); @@ -104,7 +107,10 @@ TEST_CASE(test_mkstemp_unique_filename) auto fd = mkstemp(path); EXPECT(fd != -1); - auto path2 = Core::File::read_link(String::formatted("/proc/{}/fd/{}", getpid(), fd)); + auto path2_or_error = Core::File::read_link(String::formatted("/proc/{}/fd/{}", getpid(), fd)); + EXPECT(!path2_or_error.is_error()); + + auto path2 = path2_or_error.release_value(); EXPECT(path2.characters()); close(fd); diff --git a/Tests/LibELF/test-elf.cpp b/Tests/LibELF/test-elf.cpp index cbfe8badd20..d15e4856c21 100644 --- a/Tests/LibELF/test-elf.cpp +++ b/Tests/LibELF/test-elf.cpp @@ -58,7 +58,10 @@ TEST_CASE(test_interp_header_tiny_p_filesz) int nwritten = write(fd, buffer, sizeof(buffer)); EXPECT(nwritten); - auto elf_path = Core::File::read_link(String::formatted("/proc/{}/fd/{}", getpid(), fd)); + auto elf_path_or_error = Core::File::read_link(String::formatted("/proc/{}/fd/{}", getpid(), fd)); + EXPECT(!elf_path_or_error.is_error()); + + auto elf_path = elf_path_or_error.release_value(); EXPECT(elf_path.characters()); int rc = execl(elf_path.characters(), "test-elf", nullptr); @@ -112,7 +115,10 @@ TEST_CASE(test_interp_header_p_filesz_larger_than_p_memsz) int nwritten = write(fd, buffer, sizeof(buffer)); EXPECT(nwritten); - auto elf_path = Core::File::read_link(String::formatted("/proc/{}/fd/{}", getpid(), fd)); + auto elf_path_or_error = Core::File::read_link(String::formatted("/proc/{}/fd/{}", getpid(), fd)); + EXPECT(!elf_path_or_error.is_error()); + + auto elf_path = elf_path_or_error.release_value(); EXPECT(elf_path.characters()); int rc = execl(elf_path.characters(), "test-elf", nullptr); @@ -170,7 +176,10 @@ TEST_CASE(test_interp_header_p_filesz_plus_p_offset_overflow_p_memsz) int nwritten = write(fd, buffer, sizeof(buffer)); EXPECT(nwritten); - auto elf_path = Core::File::read_link(String::formatted("/proc/{}/fd/{}", getpid(), fd)); + auto elf_path_or_error = Core::File::read_link(String::formatted("/proc/{}/fd/{}", getpid(), fd)); + EXPECT(!elf_path_or_error.is_error()); + + auto elf_path = elf_path_or_error.release_value(); EXPECT(elf_path.characters()); int rc = execl(elf_path.characters(), "test-elf", nullptr); @@ -225,7 +234,10 @@ TEST_CASE(test_load_header_p_memsz_zero) int nwritten = write(fd, buffer, sizeof(buffer)); EXPECT(nwritten); - auto elf_path = Core::File::read_link(String::formatted("/proc/{}/fd/{}", getpid(), fd)); + auto elf_path_or_error = Core::File::read_link(String::formatted("/proc/{}/fd/{}", getpid(), fd)); + EXPECT(!elf_path_or_error.is_error()); + + auto elf_path = elf_path_or_error.release_value(); EXPECT(elf_path.characters()); int rc = execl(elf_path.characters(), "test-elf", nullptr); @@ -280,7 +292,10 @@ TEST_CASE(test_load_header_p_memsz_not_equal_to_p_align) int nwritten = write(fd, buffer, sizeof(buffer)); EXPECT(nwritten); - auto elf_path = Core::File::read_link(String::formatted("/proc/{}/fd/{}", getpid(), fd)); + auto elf_path_or_error = Core::File::read_link(String::formatted("/proc/{}/fd/{}", getpid(), fd)); + EXPECT(!elf_path_or_error.is_error()); + + auto elf_path = elf_path_or_error.release_value(); EXPECT(elf_path.characters()); int rc = execl(elf_path.characters(), "test-elf", nullptr); diff --git a/Userland/Applications/FileManager/PropertiesWindow.cpp b/Userland/Applications/FileManager/PropertiesWindow.cpp index ac83c0986f0..45a6012f109 100644 --- a/Userland/Applications/FileManager/PropertiesWindow.cpp +++ b/Userland/Applications/FileManager/PropertiesWindow.cpp @@ -95,10 +95,11 @@ PropertiesWindow::PropertiesWindow(String const& path, bool disable_rename, Wind }; if (S_ISLNK(m_mode)) { - auto link_destination = Core::File::read_link(path); - if (link_destination.is_null()) { + auto link_destination_or_error = Core::File::read_link(path); + if (link_destination_or_error.is_error()) { perror("readlink"); } else { + auto link_destination = link_destination_or_error.release_value(); auto link_location = general_tab.find_descendant_of_type_named("link_location"); link_location->set_text(link_destination); link_location->on_click = [link_destination] { diff --git a/Userland/Libraries/LibCore/File.cpp b/Userland/Libraries/LibCore/File.cpp index 9ae04bc0284..811a50dd441 100644 --- a/Userland/Libraries/LibCore/File.cpp +++ b/Userland/Libraries/LibCore/File.cpp @@ -247,28 +247,28 @@ String File::absolute_path(String const& path) #ifdef __serenity__ -String File::read_link(String const& link_path) +ErrorOr File::read_link(String const& link_path) { // First, try using a 64-byte buffer, that ought to be enough for anybody. char small_buffer[64]; int rc = serenity_readlink(link_path.characters(), link_path.length(), small_buffer, sizeof(small_buffer)); if (rc < 0) - return {}; + return Error::from_errno(errno); size_t size = rc; // If the call was successful, the syscall (unlike the LibC wrapper) // returns the full size of the link. Let's see if our small buffer // was enough to read the whole link. if (size <= sizeof(small_buffer)) - return { small_buffer, size }; + return String { small_buffer, size }; // Nope, but at least now we know the right size. char* large_buffer_ptr; auto large_buffer = StringImpl::create_uninitialized(size, large_buffer_ptr); rc = serenity_readlink(link_path.characters(), link_path.length(), large_buffer_ptr, size); if (rc < 0) - return {}; + return Error::from_errno(errno); size_t new_size = rc; if (new_size == size) @@ -277,31 +277,31 @@ String File::read_link(String const& link_path) // If we're here, the symlink has changed while we were looking at it. // If it became shorter, our buffer is valid, we just have to trim it a bit. if (new_size < size) - return { large_buffer_ptr, new_size }; + return String { large_buffer_ptr, new_size }; // Otherwise, here's not much we can do, unless we want to loop endlessly // in this case. Let's leave it up to the caller whether to loop. errno = EAGAIN; - return {}; + return Error::from_errno(errno); } #else // This is a sad version for other systems. It has to always make a copy of the // link path, and to always make two syscalls to get the right size first. -String File::read_link(String const& link_path) +ErrorOr File::read_link(String const& link_path) { struct stat statbuf = {}; int rc = lstat(link_path.characters(), &statbuf); if (rc < 0) - return {}; + return Error::from_errno(errno); char* buffer_ptr; auto buffer = StringImpl::create_uninitialized(statbuf.st_size, buffer_ptr); if (readlink(link_path.characters(), buffer_ptr, statbuf.st_size) < 0) - return {}; + return Error::from_errno(errno); // (See above.) if (rc == statbuf.st_size) return { *buffer }; - return { buffer_ptr, (size_t)rc }; + return String { buffer_ptr, (size_t)rc }; } #endif diff --git a/Userland/Libraries/LibCore/File.h b/Userland/Libraries/LibCore/File.h index 146a3ae239c..43fe19ebfc4 100644 --- a/Userland/Libraries/LibCore/File.h +++ b/Userland/Libraries/LibCore/File.h @@ -76,7 +76,7 @@ public: static ErrorOr copy_file_or_directory(String const& dst_path, String const& src_path, RecursionMode = RecursionMode::Allowed, LinkMode = LinkMode::Disallowed, AddDuplicateFileMarker = AddDuplicateFileMarker::Yes, PreserveMode = PreserveMode::Nothing); static String real_path_for(String const& filename); - static String read_link(String const& link_path); + static ErrorOr read_link(String const& link_path); static ErrorOr link_file(String const& dst_path, String const& src_path); struct RemoveError : public Error { diff --git a/Userland/Libraries/LibGUI/FileIconProvider.cpp b/Userland/Libraries/LibGUI/FileIconProvider.cpp index 27c4b4fa03f..821af954913 100644 --- a/Userland/Libraries/LibGUI/FileIconProvider.cpp +++ b/Userland/Libraries/LibGUI/FileIconProvider.cpp @@ -232,7 +232,11 @@ Icon FileIconProvider::icon_for_path(const String& path, mode_t mode) return s_directory_icon; } if (S_ISLNK(mode)) { - auto raw_symlink_target = Core::File::read_link(path); + auto raw_symlink_target_or_error = Core::File::read_link(path); + if (raw_symlink_target_or_error.is_error()) + return s_symlink_icon; + + auto raw_symlink_target = raw_symlink_target_or_error.release_value(); if (raw_symlink_target.is_null()) return s_symlink_icon; diff --git a/Userland/Libraries/LibGUI/FileSystemModel.cpp b/Userland/Libraries/LibGUI/FileSystemModel.cpp index 1bf7fbcc048..f4f9cfe3643 100644 --- a/Userland/Libraries/LibGUI/FileSystemModel.cpp +++ b/Userland/Libraries/LibGUI/FileSystemModel.cpp @@ -61,7 +61,11 @@ bool FileSystemModel::Node::fetch_data(String const& full_path, bool is_root) mtime = st.st_mtime; if (S_ISLNK(mode)) { - symlink_target = Core::File::read_link(full_path); + auto sym_link_target_or_error = Core::File::read_link(full_path); + if (sym_link_target_or_error.is_error()) + perror("readlink"); + + symlink_target = sym_link_target_or_error.release_value(); if (symlink_target.is_null()) perror("readlink"); } diff --git a/Userland/Services/LaunchServer/Launcher.cpp b/Userland/Services/LaunchServer/Launcher.cpp index 09fa6d496b4..a7bedbb473e 100644 --- a/Userland/Services/LaunchServer/Launcher.cpp +++ b/Userland/Services/LaunchServer/Launcher.cpp @@ -271,7 +271,11 @@ void Launcher::for_each_handler_for_path(const String& path, Function bool { diff --git a/Userland/Utilities/ls.cpp b/Userland/Utilities/ls.cpp index 30fa3a9a961..be1b551cb58 100644 --- a/Userland/Utilities/ls.cpp +++ b/Userland/Utilities/ls.cpp @@ -275,11 +275,11 @@ static size_t print_name(const struct stat& st, const String& name, const char* } if (S_ISLNK(st.st_mode)) { if (path_for_link_resolution) { - auto link_destination = Core::File::read_link(path_for_link_resolution); - if (link_destination.is_null()) { + auto link_destination_or_error = Core::File::read_link(path_for_link_resolution); + if (link_destination_or_error.is_error()) { perror("readlink"); } else { - nprinted += printf(" -> ") + print_escaped(link_destination.characters()); + nprinted += printf(" -> ") + print_escaped(link_destination_or_error.value().characters()); } } else { if (flag_classify) diff --git a/Userland/Utilities/readlink.cpp b/Userland/Utilities/readlink.cpp index 318e31ac1a8..53c7a25d2df 100644 --- a/Userland/Utilities/readlink.cpp +++ b/Userland/Utilities/readlink.cpp @@ -25,11 +25,13 @@ int main(int argc, char** argv) args_parser.parse(argc, argv); for (const char* path : paths) { - auto destination = Core::File::read_link(path); - if (destination.is_null()) { + auto destination_or_error = Core::File::read_link(path); + if (destination_or_error.is_error()) { perror(path); return 1; } + + auto destination = destination_or_error.release_value(); out("{}", destination); if (!no_newline) outln();