From 0f5477c721c4cd8f1dbbf34eafd348bd248a1f79 Mon Sep 17 00:00:00 2001 From: Andreas Kling Date: Sun, 7 Nov 2021 00:37:07 +0100 Subject: [PATCH] AK: Use ErrorOr for MappedFile factories Replace Result with ErrorOr and propagate the error to callers. --- AK/MappedFile.cpp | 19 +++--- AK/MappedFile.h | 6 +- Userland/Applications/Help/ManualModel.cpp | 11 ++-- Userland/Applications/Help/ManualModel.h | 2 +- Userland/Applications/Help/main.cpp | 2 +- .../Applications/PixelPaint/ProjectLoader.cpp | 2 +- .../Libraries/LibGfx/TrueTypeFont/Font.cpp | 62 +++++++++---------- Userland/Libraries/LibGfx/TrueTypeFont/Font.h | 6 +- .../LibSymbolication/Symbolication.cpp | 2 +- Userland/Services/CrashDaemon/main.cpp | 2 +- Userland/Utilities/disasm.cpp | 2 +- Userland/Utilities/fdtdump.cpp | 2 +- 12 files changed, 54 insertions(+), 64 deletions(-) diff --git a/AK/MappedFile.cpp b/AK/MappedFile.cpp index 38e13f3d82..2e4adc5577 100644 --- a/AK/MappedFile.cpp +++ b/AK/MappedFile.cpp @@ -15,36 +15,33 @@ namespace AK { -Result, OSError> MappedFile::map(String const& path) +ErrorOr> MappedFile::map(String const& path) { int fd = open(path.characters(), O_RDONLY | O_CLOEXEC, 0); if (fd < 0) - return OSError(errno); + return Error::from_errno(errno); return map_from_fd_and_close(fd, path); } -Result, OSError> MappedFile::map_from_fd_and_close(int fd, [[maybe_unused]] String const& path) +ErrorOr> MappedFile::map_from_fd_and_close(int fd, [[maybe_unused]] String const& path) { - if (fcntl(fd, F_SETFD, FD_CLOEXEC) == -1) { - return OSError(errno); - } + if (fcntl(fd, F_SETFD, FD_CLOEXEC) < 0) + return Error::from_errno(errno); ScopeGuard fd_close_guard = [fd] { close(fd); }; struct stat st; - if (fstat(fd, &st) < 0) { - auto saved_errno = errno; - return OSError(saved_errno); - } + if (fstat(fd, &st) < 0) + return Error::from_errno(errno); auto size = st.st_size; auto* ptr = mmap(nullptr, size, PROT_READ, MAP_SHARED, fd, 0); if (ptr == MAP_FAILED) - return OSError(errno); + return Error::from_errno(errno); #ifdef __serenity__ if (set_mmap_name(ptr, size, path.characters()) < 0) { diff --git a/AK/MappedFile.h b/AK/MappedFile.h index 2dcf197a9e..95bed66b30 100644 --- a/AK/MappedFile.h +++ b/AK/MappedFile.h @@ -6,9 +6,9 @@ #pragma once +#include #include #include -#include #include #include @@ -19,8 +19,8 @@ class MappedFile : public RefCounted { AK_MAKE_NONMOVABLE(MappedFile); public: - static Result, OSError> map(String const& path); - static Result, OSError> map_from_fd_and_close(int fd, String const& path); + static ErrorOr> map(String const& path); + static ErrorOr> map_from_fd_and_close(int fd, String const& path); ~MappedFile(); void* data() { return m_data; } diff --git a/Userland/Applications/Help/ManualModel.cpp b/Userland/Applications/Help/ManualModel.cpp index 353502cd49..8461788b56 100644 --- a/Userland/Applications/Help/ManualModel.cpp +++ b/Userland/Applications/Help/ManualModel.cpp @@ -9,6 +9,7 @@ #include "ManualPageNode.h" #include "ManualSectionNode.h" #include +#include #include #include @@ -59,7 +60,7 @@ String ManualModel::page_path(const GUI::ModelIndex& index) const return page->path(); } -Result ManualModel::page_view(const String& path) const +ErrorOr ManualModel::page_view(String const& path) const { if (path.is_empty()) return StringView {}; @@ -71,12 +72,10 @@ Result ManualModel::page_view(const String& path) const return StringView { mapped_file.value()->bytes() }; } - auto file_or_error = MappedFile::map(path); - if (file_or_error.is_error()) - return file_or_error.error(); + auto file = TRY(MappedFile::map(path)); - StringView view { file_or_error.value()->bytes() }; - m_mapped_files.set(path, file_or_error.release_value()); + StringView view { file->bytes() }; + m_mapped_files.set(path, move(file)); return view; } diff --git a/Userland/Applications/Help/ManualModel.h b/Userland/Applications/Help/ManualModel.h index 9c2002f15f..1345fb0548 100644 --- a/Userland/Applications/Help/ManualModel.h +++ b/Userland/Applications/Help/ManualModel.h @@ -25,7 +25,7 @@ public: String page_path(const GUI::ModelIndex&) const; String page_and_section(const GUI::ModelIndex&) const; - Result page_view(const String& path) const; + ErrorOr page_view(String const& path) const; void update_section_node_on_toggle(const GUI::ModelIndex&, const bool); virtual int row_count(const GUI::ModelIndex& = GUI::ModelIndex()) const override; diff --git a/Userland/Applications/Help/main.cpp b/Userland/Applications/Help/main.cpp index f7b7be2092..7ecac91743 100644 --- a/Userland/Applications/Help/main.cpp +++ b/Userland/Applications/Help/main.cpp @@ -139,7 +139,7 @@ int main(int argc, char* argv[]) auto source_result = model->page_view(path); if (source_result.is_error()) { - GUI::MessageBox::show(window, source_result.error().string(), "Failed to open man page", GUI::MessageBox::Type::Error); + GUI::MessageBox::show(window, String::formatted("{}", source_result.error()), "Failed to open man page", GUI::MessageBox::Type::Error); return; } diff --git a/Userland/Applications/PixelPaint/ProjectLoader.cpp b/Userland/Applications/PixelPaint/ProjectLoader.cpp index a6c703fe7e..df48b2747a 100644 --- a/Userland/Applications/PixelPaint/ProjectLoader.cpp +++ b/Userland/Applications/PixelPaint/ProjectLoader.cpp @@ -31,7 +31,7 @@ Result ProjectLoader::try_load_from_fd_and_close(int fd, StringVie auto file_or_error = MappedFile::map_from_fd_and_close(fd, path); if (file_or_error.is_error()) - return String::formatted("Unable to mmap file {}", file_or_error.error().string()); + return String::formatted("Unable to mmap file {}", file_or_error.error()); auto& mapped_file = *file_or_error.value(); // FIXME: Find a way to avoid the memory copy here. diff --git a/Userland/Libraries/LibGfx/TrueTypeFont/Font.cpp b/Userland/Libraries/LibGfx/TrueTypeFont/Font.cpp index cc89ffbfde..9636371d49 100644 --- a/Userland/Libraries/LibGfx/TrueTypeFont/Font.cpp +++ b/Userland/Libraries/LibGfx/TrueTypeFont/Font.cpp @@ -7,6 +7,7 @@ #include #include +#include #include #include #include @@ -224,52 +225,45 @@ GlyphHorizontalMetrics Hmtx::get_glyph_horizontal_metrics(u32 glyph_id) const }; } -Result, String> Font::try_load_from_file(String path, unsigned index) +ErrorOr> Font::try_load_from_file(String path, unsigned index) { - auto file_or_error = MappedFile::map(path); - if (file_or_error.is_error()) - return String { file_or_error.error().string() }; - - auto& file = *file_or_error.value(); - auto result = try_load_from_externally_owned_memory(file.bytes(), index); - if (result.is_error()) - return result.error(); - auto& font = *result.value(); - font.m_mapped_file = file_or_error.release_value(); - return result; + auto file = TRY(MappedFile::map(path)); + auto font = TRY(try_load_from_externally_owned_memory(file->bytes(), index)); + font->m_mapped_file = move(file); + return font; } -Result, String> Font::try_load_from_externally_owned_memory(ReadonlyBytes buffer, unsigned index) +ErrorOr> Font::try_load_from_externally_owned_memory(ReadonlyBytes buffer, unsigned index) { if (buffer.size() < 4) - return String { "Font file too small" }; + return Error::from_string_literal("Font file too small"sv); u32 tag = be_u32(buffer.data()); if (tag == tag_from_str("ttcf")) { // It's a font collection if (buffer.size() < (u32)Sizes::TTCHeaderV1 + sizeof(u32) * (index + 1)) - return String { "Font file too small" }; + return Error::from_string_literal("Font file too small"sv); u32 offset = be_u32(buffer.offset_pointer((u32)Sizes::TTCHeaderV1 + sizeof(u32) * index)); - return try_load_from_offset(move(buffer), offset); + return try_load_from_offset(buffer, offset); } if (tag == tag_from_str("OTTO")) - return String { "CFF fonts not supported yet" }; + return Error::from_string_literal("CFF fonts not supported yet"sv); if (tag != 0x00010000) - return String { "Not a valid font" }; + return Error::from_string_literal("Not a valid font"sv); - return try_load_from_offset(move(buffer), 0); + return try_load_from_offset(buffer, 0); } // FIXME: "loca" and "glyf" are not available for CFF fonts. -Result, String> Font::try_load_from_offset(ReadonlyBytes buffer, u32 offset) +ErrorOr> Font::try_load_from_offset(ReadonlyBytes buffer, u32 offset) { if (Checked::addition_would_overflow(offset, (u32)Sizes::OffsetTable)) - return String { "Invalid offset in font header" }; + return Error::from_string_literal("Invalid offset in font header"sv); if (buffer.size() < offset + (u32)Sizes::OffsetTable) - return String { "Font file too small" }; + return Error::from_string_literal("Font file too small"sv); Optional opt_head_slice = {}; Optional opt_name_slice = {}; @@ -292,7 +286,7 @@ Result, String> Font::try_load_from_offset(ReadonlyBytes buf auto num_tables = be_u16(buffer.offset_pointer(offset + (u32)Offsets::NumTables)); if (buffer.size() < offset + (u32)Sizes::OffsetTable + num_tables * (u32)Sizes::TableRecord) - return String { "Font file too small" }; + return Error::from_string_literal("Font file too small"sv); for (auto i = 0; i < num_tables; i++) { u32 record_offset = offset + (u32)Sizes::OffsetTable + i * (u32)Sizes::TableRecord; @@ -301,10 +295,10 @@ Result, String> Font::try_load_from_offset(ReadonlyBytes buf u32 table_length = be_u32(buffer.offset_pointer(record_offset + (u32)Offsets::TableRecord_Length)); if (Checked::addition_would_overflow(table_offset, table_length)) - return String { "Invalid table offset/length in font." }; + return Error::from_string_literal("Invalid table offset/length in font."sv); if (buffer.size() < table_offset + table_length) - return String { "Font file too small" }; + return Error::from_string_literal("Font file too small"sv); auto buffer_here = ReadonlyBytes(buffer.offset_pointer(table_offset), table_length); @@ -331,39 +325,39 @@ Result, String> Font::try_load_from_offset(ReadonlyBytes buf } if (!opt_head_slice.has_value() || !(opt_head = Head::from_slice(opt_head_slice.value())).has_value()) - return String { "Could not load Head" }; + return Error::from_string_literal("Could not load Head"sv); auto head = opt_head.value(); if (!opt_name_slice.has_value() || !(opt_name = Name::from_slice(opt_name_slice.value())).has_value()) - return String { "Could not load Name" }; + return Error::from_string_literal("Could not load Name"sv); auto name = opt_name.value(); if (!opt_hhea_slice.has_value() || !(opt_hhea = Hhea::from_slice(opt_hhea_slice.value())).has_value()) - return String { "Could not load Hhea" }; + return Error::from_string_literal("Could not load Hhea"sv); auto hhea = opt_hhea.value(); if (!opt_maxp_slice.has_value() || !(opt_maxp = Maxp::from_slice(opt_maxp_slice.value())).has_value()) - return String { "Could not load Maxp" }; + return Error::from_string_literal("Could not load Maxp"sv); auto maxp = opt_maxp.value(); if (!opt_hmtx_slice.has_value() || !(opt_hmtx = Hmtx::from_slice(opt_hmtx_slice.value(), maxp.num_glyphs(), hhea.number_of_h_metrics())).has_value()) - return String { "Could not load Hmtx" }; + return Error::from_string_literal("Could not load Hmtx"sv); auto hmtx = opt_hmtx.value(); if (!opt_cmap_slice.has_value() || !(opt_cmap = Cmap::from_slice(opt_cmap_slice.value())).has_value()) - return String { "Could not load Cmap" }; + return Error::from_string_literal("Could not load Cmap"sv); auto cmap = opt_cmap.value(); if (!opt_loca_slice.has_value() || !(opt_loca = Loca::from_slice(opt_loca_slice.value(), maxp.num_glyphs(), head.index_to_loc_format())).has_value()) - return String { "Could not load Loca" }; + return Error::from_string_literal("Could not load Loca"sv); auto loca = opt_loca.value(); if (!opt_glyf_slice.has_value()) - return String { "Could not load Glyf" }; + return Error::from_string_literal("Could not load Glyf"sv); auto glyf = Glyf(opt_glyf_slice.value()); if (!opt_os2_slice.has_value()) - return String { "Could not load OS/2" }; + return Error::from_string_literal("Could not load OS/2"sv); auto os2 = OS2(opt_os2_slice.value()); // Select cmap table. FIXME: Do this better. Right now, just looks for platform "Windows" diff --git a/Userland/Libraries/LibGfx/TrueTypeFont/Font.h b/Userland/Libraries/LibGfx/TrueTypeFont/Font.h index 810b509450..c22176a95e 100644 --- a/Userland/Libraries/LibGfx/TrueTypeFont/Font.h +++ b/Userland/Libraries/LibGfx/TrueTypeFont/Font.h @@ -45,8 +45,8 @@ class Font : public RefCounted { AK_MAKE_NONCOPYABLE(Font); public: - static Result, String> try_load_from_file(String path, unsigned index = 0); - static Result, String> try_load_from_externally_owned_memory(ReadonlyBytes bytes, unsigned index = 0); + static ErrorOr> try_load_from_file(String path, unsigned index = 0); + static ErrorOr> try_load_from_externally_owned_memory(ReadonlyBytes bytes, unsigned index = 0); ScaledFontMetrics metrics(float x_scale, float y_scale) const; ScaledGlyphMetrics glyph_metrics(u32 glyph_id, float x_scale, float y_scale) const; @@ -71,7 +71,7 @@ private: TableRecord = 16, }; - static Result, String> try_load_from_offset(ReadonlyBytes, unsigned index = 0); + static ErrorOr> try_load_from_offset(ReadonlyBytes, unsigned index = 0); Font(ReadonlyBytes bytes, Head&& head, Name&& name, Hhea&& hhea, Maxp&& maxp, Hmtx&& hmtx, Cmap&& cmap, Loca&& loca, Glyf&& glyf, OS2&& os2) : m_buffer(move(bytes)) diff --git a/Userland/Libraries/LibSymbolication/Symbolication.cpp b/Userland/Libraries/LibSymbolication/Symbolication.cpp index ef88304429..07cc2205ec 100644 --- a/Userland/Libraries/LibSymbolication/Symbolication.cpp +++ b/Userland/Libraries/LibSymbolication/Symbolication.cpp @@ -83,7 +83,7 @@ Optional symbolicate(String const& path, FlatPtr address, IncludeSourceP if (!s_cache.contains(full_path)) { auto mapped_file = MappedFile::map(full_path); if (mapped_file.is_error()) { - dbgln("Failed to map {}: {}", full_path, mapped_file.error().string()); + dbgln("Failed to map {}: {}", full_path, mapped_file.error()); s_cache.set(full_path, {}); return {}; } diff --git a/Userland/Services/CrashDaemon/main.cpp b/Userland/Services/CrashDaemon/main.cpp index 120c5fd5de..d9f258d88c 100644 --- a/Userland/Services/CrashDaemon/main.cpp +++ b/Userland/Services/CrashDaemon/main.cpp @@ -74,7 +74,7 @@ int main() auto file_or_error = MappedFile::map(coredump_path); if (file_or_error.is_error()) { - dbgln("Unable to map coredump {}: {}", coredump_path, file_or_error.error().string()); + dbgln("Unable to map coredump {}: {}", coredump_path, file_or_error.error()); continue; } diff --git a/Userland/Utilities/disasm.cpp b/Userland/Utilities/disasm.cpp index be78eeaaa6..77d6c73b86 100644 --- a/Userland/Utilities/disasm.cpp +++ b/Userland/Utilities/disasm.cpp @@ -28,7 +28,7 @@ int main(int argc, char** argv) auto file_or_error = MappedFile::map(path); if (file_or_error.is_error()) { - warnln("Could not map file: {}", file_or_error.error().string()); + warnln("Could not map file: {}", file_or_error.error()); return 1; } diff --git a/Userland/Utilities/fdtdump.cpp b/Userland/Utilities/fdtdump.cpp index bc59de5543..0bb0050f25 100644 --- a/Userland/Utilities/fdtdump.cpp +++ b/Userland/Utilities/fdtdump.cpp @@ -26,7 +26,7 @@ int main(int argc, char* argv[]) // FIXME: Figure out how to do this sanely from stdin auto maybe_file = MappedFile::map(filename); if (maybe_file.is_error()) { - warnln("Unable to dump device tree from file {}: {}", filename, maybe_file.error().string()); + warnln("Unable to dump device tree from file {}: {}", filename, maybe_file.error()); return 1; } auto file = maybe_file.release_value();