From 86bd65df03e3a6f661f9ea543160de54aa136fb8 Mon Sep 17 00:00:00 2001 From: Andrew Gunnerson Date: Tue, 27 Nov 2018 08:29:01 -0500 Subject: [PATCH] libmbcommon: Use bitmask to enable multiple formats and drop set_format() set_format() never worked properly due to it causing the bidding process to be skipped and every FormatReader implementation required open() to be called. Signed-off-by: Andrew Gunnerson --- bootimgtool/bootimgtool.cpp | 4 +- examples/bootimg_compare.cpp | 4 +- libmbbootimg/include/mbbootimg/format.h | 25 +-- libmbbootimg/include/mbbootimg/reader.h | 6 +- libmbbootimg/include/mbbootimg/reader_error.h | 5 +- libmbbootimg/src/format.cpp | 31 ++-- libmbbootimg/src/reader.cpp | 147 +++++++----------- libmbbootimg/src/reader_error.cpp | 2 - .../tests/format/test_android_reader.cpp | 2 +- libmiscstuff-jni/libmiscstuff.cpp | 6 +- mbtool/src/recovery/installer_util.cpp | 2 +- mbtool/src/recovery/rom_installer.cpp | 2 +- 12 files changed, 99 insertions(+), 137 deletions(-) diff --git a/bootimgtool/bootimgtool.cpp b/bootimgtool/bootimgtool.cpp index 2a4e06524..6042d591a 100644 --- a/bootimgtool/bootimgtool.cpp +++ b/bootimgtool/bootimgtool.cpp @@ -829,13 +829,13 @@ static bool unpack_main(int argc, char *argv[]) return false; } - if (auto r = reader.enable_format(*format); !r) { + if (auto r = reader.enable_formats(*format); !r) { fprintf(stderr, "Failed to enable format '%s': %s\n", type, r.error().message().c_str()); return false; } } else { - if (auto r = reader.enable_format_all(); !r) { + if (auto r = reader.enable_formats_all(); !r) { fprintf(stderr, "Failed to enable all formats: %s\n", r.error().message().c_str()); return false; diff --git a/examples/bootimg_compare.cpp b/examples/bootimg_compare.cpp index f044d3951..92b6d6dc4 100644 --- a/examples/bootimg_compare.cpp +++ b/examples/bootimg_compare.cpp @@ -79,12 +79,12 @@ int main(int argc, char *argv[]) size_t entries = 0; // Set up reader formats - if (auto r = reader1.enable_format_all(); !r) { + if (auto r = reader1.enable_formats_all(); !r) { fprintf(stderr, "Failed to enable all boot image formats: %s\n", r.error().message().c_str()); return EXIT_FAILURE; } - if (auto r = reader2.enable_format_all(); !r) { + if (auto r = reader2.enable_formats_all(); !r) { fprintf(stderr, "Failed to enable all boot image formats: %s\n", r.error().message().c_str()); return EXIT_FAILURE; diff --git a/libmbbootimg/include/mbbootimg/format.h b/libmbbootimg/include/mbbootimg/format.h index f4ae95ea0..73f092469 100644 --- a/libmbbootimg/include/mbbootimg/format.h +++ b/libmbbootimg/include/mbbootimg/format.h @@ -23,24 +23,31 @@ #include #include "mbcommon/common.h" +#include "mbcommon/flags.h" namespace mb::bootimg { -enum class Format +enum class Format : uint8_t { - Android, - Bump, - Loki, - Mtk, - SonyElf, + Android = 1 << 0, + Bump = 1 << 1, + Loki = 1 << 2, + Mtk = 1 << 3, + SonyElf = 1 << 4, }; +MB_DECLARE_FLAGS(Formats, Format) +MB_DECLARE_OPERATORS_FOR_FLAGS(Formats) + +constexpr Formats ALL_FORMATS = + Format::Android + | Format::Bump + | Format::Loki + | Format::Mtk + | Format::SonyElf; MB_EXPORT std::string_view format_to_name(Format format); MB_EXPORT std::optional name_to_format(std::string_view name); -// TODO: Switch to span eventually -MB_EXPORT std::basic_string_view formats(); - } diff --git a/libmbbootimg/include/mbbootimg/reader.h b/libmbbootimg/include/mbbootimg/reader.h index c19211b25..60951ba05 100644 --- a/libmbbootimg/include/mbbootimg/reader.h +++ b/libmbbootimg/include/mbbootimg/reader.h @@ -68,9 +68,8 @@ class MB_EXPORT Reader // Format operations std::optional format(); - oc::result enable_format(Format format); - oc::result enable_format_all(); - oc::result set_format(Format format); + oc::result enable_formats(Formats formats); + oc::result enable_formats_all(); // Reader state bool is_open(); @@ -85,7 +84,6 @@ class MB_EXPORT Reader std::vector> m_formats; detail::FormatReader *m_format; - bool m_format_user_set; }; } diff --git a/libmbbootimg/include/mbbootimg/reader_error.h b/libmbbootimg/include/mbbootimg/reader_error.h index 1abd7312f..6e1301a12 100644 --- a/libmbbootimg/include/mbbootimg/reader_error.h +++ b/libmbbootimg/include/mbbootimg/reader_error.h @@ -33,9 +33,8 @@ enum class ReaderError UnknownOption = 20, // Format errors - FormatAlreadyEnabled = 30, - NoFormatsRegistered = 31, - UnknownFileFormat = 32, + NoFormatsRegistered = 30, + UnknownFileFormat = 31, EndOfEntries = 40, diff --git a/libmbbootimg/src/format.cpp b/libmbbootimg/src/format.cpp index 799ea1497..65a7a14aa 100644 --- a/libmbbootimg/src/format.cpp +++ b/libmbbootimg/src/format.cpp @@ -73,13 +73,20 @@ namespace mb::bootimg * \brief Format code for Sony ELF boot images */ -static std::array g_formats{{ - Format::Android, - Format::Bump, - Format::Loki, - Format::Mtk, - Format::SonyElf, -}}; +/*! + * \class Formats + * + * \brief Bitmask of Format variants + */ + +/*! + * \var ALL_FORMATS + * + * \brief Bitmask of all supported formats + * + * Note that this Formats object can be iterated over to get each supported + * \ref Format variant. + */ /*! * \brief Convert format to string @@ -130,14 +137,4 @@ std::optional name_to_format(std::string_view name) } } -/*! - * \brief Get list of supported formats - * - * \return View to static list of supported formats - */ -std::basic_string_view formats() -{ - return {g_formats.data(), g_formats.size()}; -} - } diff --git a/libmbbootimg/src/reader.cpp b/libmbbootimg/src/reader.cpp index 31521c64f..6035ea580 100644 --- a/libmbbootimg/src/reader.cpp +++ b/libmbbootimg/src/reader.cpp @@ -216,7 +216,6 @@ Reader::Reader() , m_owned_file() , m_file() , m_format() - , m_format_user_set(false) { } @@ -240,7 +239,6 @@ Reader::Reader(Reader &&other) noexcept std::swap(m_file, other.m_file); std::swap(m_formats, other.m_formats); std::swap(m_format, other.m_format); - std::swap(m_format_user_set, other.m_format_user_set); } Reader & Reader::operator=(Reader &&rhs) noexcept @@ -248,16 +246,14 @@ Reader & Reader::operator=(Reader &&rhs) noexcept if (this != &rhs) { (void) close(); - // close() only resets the autodetected format + // close() doesn't reset enabled formats m_formats.clear(); - m_format = nullptr; std::swap(m_state, rhs.m_state); std::swap(m_owned_file, rhs.m_owned_file); std::swap(m_file, rhs.m_file); std::swap(m_formats, rhs.m_formats); std::swap(m_format, rhs.m_format); - std::swap(m_format_user_set, rhs.m_format_user_set); } return *this; @@ -349,42 +345,40 @@ oc::result Reader::open(File *file) } }); - // Perform bid if a format wasn't explicitly chosen - if (!m_format) { - for (auto &f : m_formats) { - // Seek to beginning - OUTCOME_TRYV(file->seek(0, SEEK_SET)); + // Perform bid for autodetection + for (auto &f : m_formats) { + // Seek to beginning + OUTCOME_TRYV(file->seek(0, SEEK_SET)); - auto close_f = finally([&] { - (void) f->close(*file); - }); + auto close_f = finally([&] { + (void) f->close(*file); + }); - // Call bidder - OUTCOME_TRY(bid, f->open(*file, best_bid)); + // Call bidder + OUTCOME_TRY(bid, f->open(*file, best_bid)); - if (bid > best_bid) { - // Close previous best format - if (format) { - (void) format->close(*file); - } + if (bid > best_bid) { + // Close previous best format + if (format) { + (void) format->close(*file); + } - // Don't close this format - close_f.dismiss(); + // Don't close this format + close_f.dismiss(); - best_bid = bid; - format = f.get(); - } + best_bid = bid; + format = f.get(); } + } - if (!format) { - return ReaderError::UnknownFileFormat; - } + if (!format) { + return ReaderError::UnknownFileFormat; + } - // We've found a matching format, so don't close it - close_format.dismiss(); + // We've found a matching format, so don't close it + close_format.dismiss(); - m_format = format; - } + m_format = format; m_state = ReaderState::Header; m_file = file; @@ -409,10 +403,7 @@ oc::result Reader::close() m_owned_file.reset(); m_file = nullptr; - // Reset auto-detected format - if (!m_format_user_set) { - m_format = nullptr; - } + m_format = nullptr; }); oc::result ret = oc::success(); @@ -509,12 +500,7 @@ oc::result Reader::read_data(void *buf, size_t size) } /*! - * \brief Get detected or forced boot image format code. - * - * * If enable_format() was used, then the detected boot image format code is - * returned. - * * If set_format() was used, then the forced boot image format code is - * returned. + * \brief Get detected boot image format code. * * \note The return value is meaningful only after the boot image has been * successfully opened. @@ -549,24 +535,39 @@ static std::unique_ptr _construct_format(Format format) } /*! - * \brief Enable support for a boot image format. + * \brief Enable support for boot image formats. + * + * \note Calling this function replaces previously enabled formats. It is not + * cumulative. * - * \param format Format to enable + * \param formats Formats to enable * - * \return Nothing if the format is successfully enabled. Otherwise, the error + * \return Nothing if the formats are successfully enabled. Otherwise, the error * code. */ -oc::result Reader::enable_format(Format format) +oc::result Reader::enable_formats(Formats formats) { ENSURE_STATE_OR_RETURN_ERROR(ReaderState::New); - for (auto const &f : m_formats) { - if (f->type() == format) { - return ReaderError::FormatAlreadyEnabled; + if (formats != (formats & ALL_FORMATS)) { + return std::errc::invalid_argument; + } + + // Remove formats that shouldn't be enabled + for (auto it = m_formats.begin(); it != m_formats.end();) { + auto format = (*it)->type(); + if (formats & format) { + formats.set_flag(format, false); + ++it; + } else { + it = m_formats.erase(it); } } - m_formats.push_back(_construct_format(format)); + // Enable what's left to be enabled + for (auto const &format : formats) { + m_formats.push_back(_construct_format(format)); + } return oc::success(); } @@ -574,52 +575,14 @@ oc::result Reader::enable_format(Format format) /*! * \brief Enable support for all boot image formats. * + * This is equivalent to calling `enable_formats(ALL_FORMATS)`. + * * \return Nothing if all formats are successfully enabled. Otherwise, the error * code. */ -oc::result Reader::enable_format_all() -{ - ENSURE_STATE_OR_RETURN_ERROR(ReaderState::New); - - for (auto const &format : formats()) { - auto ret = enable_format(format); - if (!ret && ret.error() != ReaderError::FormatAlreadyEnabled) { - return ret.as_failure(); - } - } - - return oc::success(); -} - -/*! - * \brief Force parsing as a specific boot image format. - * - * Calling this function causes the bidding process to be skipped. The chosen - * format will be used regardless of which formats are enabled. - * - * \param format Boot image format - * - * \return Nothing if the format is successfully set. Otherwise, the error code. - */ -oc::result Reader::set_format(Format format) +oc::result Reader::enable_formats_all() { - ENSURE_STATE_OR_RETURN_ERROR(ReaderState::New); - - auto ret = enable_format(format); - if (!ret && ret.error() != ReaderError::FormatAlreadyEnabled) { - return ret.as_failure(); - } - - for (auto &f : m_formats) { - if (f->type() == format) { - m_format = f.get(); - m_format_user_set = true; - - return oc::success(); - } - } - - MB_UNREACHABLE("Enabled format not found"); + return enable_formats(ALL_FORMATS); } /*! diff --git a/libmbbootimg/src/reader_error.cpp b/libmbbootimg/src/reader_error.cpp index 850a55d7a..a3651d78c 100644 --- a/libmbbootimg/src/reader_error.cpp +++ b/libmbbootimg/src/reader_error.cpp @@ -54,8 +54,6 @@ std::string ReaderErrorCategory::message(int ev) const return "invalid state"; case ReaderError::UnknownOption: return "unknown option"; - case ReaderError::FormatAlreadyEnabled: - return "format already enabled"; case ReaderError::NoFormatsRegistered: return "no formats registered"; case ReaderError::UnknownFileFormat: diff --git a/libmbbootimg/tests/format/test_android_reader.cpp b/libmbbootimg/tests/format/test_android_reader.cpp index b0c51d769..219a5ea71 100644 --- a/libmbbootimg/tests/format/test_android_reader.cpp +++ b/libmbbootimg/tests/format/test_android_reader.cpp @@ -252,7 +252,7 @@ struct AndroidReaderGoToEntryTest : testing::Test // Write secondboot memcpy(_data.data() + 3 * ahdr.page_size, "secondboot", 10); - ASSERT_TRUE(_reader.enable_format(Format::Android)); + ASSERT_TRUE(_reader.enable_formats(Format::Android)); (void) _file.open(_data.data(), _data.size()); ASSERT_TRUE(_file.is_open()); diff --git a/libmiscstuff-jni/libmiscstuff.cpp b/libmiscstuff-jni/libmiscstuff.cpp index 0ea7a441f..7758f6ce9 100644 --- a/libmiscstuff-jni/libmiscstuff.cpp +++ b/libmiscstuff-jni/libmiscstuff.cpp @@ -256,7 +256,7 @@ CLASS_METHOD(getBootImageRomId)(JNIEnv *env, jclass clazz, jstring jfilename) }); // Open input boot image - if (auto r = reader.enable_format_all(); !r) { + if (auto r = reader.enable_formats_all(); !r) { throw_exception(env, IOException, "Failed to enable all boot image formats: %s", r.error().message().c_str()); @@ -397,13 +397,13 @@ CLASS_METHOD(bootImagesEqual)(JNIEnv *env, jclass clazz, jstring jfilename1, }); // Set up reader formats - if (auto r = reader1.enable_format_all(); !r) { + if (auto r = reader1.enable_formats_all(); !r) { throw_exception(env, IOException, "Failed to enable all boot image formats: %s", r.error().message().c_str()); return false; } - if (auto r = reader2.enable_format_all(); !r) { + if (auto r = reader2.enable_formats_all(); !r) { throw_exception(env, IOException, "Failed to enable all boot image formats: %s", r.error().message().c_str()); diff --git a/mbtool/src/recovery/installer_util.cpp b/mbtool/src/recovery/installer_util.cpp index 0bee71d99..ef422b788 100644 --- a/mbtool/src/recovery/installer_util.cpp +++ b/mbtool/src/recovery/installer_util.cpp @@ -335,7 +335,7 @@ bool InstallerUtil::patch_boot_image(const std::string &input_file, LOGD("- Output: %s", output_file.c_str()); // Open input boot image - if (auto r = reader.enable_format_all(); !r) { + if (auto r = reader.enable_formats_all(); !r) { LOGE("Failed to enable input boot image formats: %s", r.error().message().c_str()); return false; diff --git a/mbtool/src/recovery/rom_installer.cpp b/mbtool/src/recovery/rom_installer.cpp index b2ef42b16..e48e52e30 100644 --- a/mbtool/src/recovery/rom_installer.cpp +++ b/mbtool/src/recovery/rom_installer.cpp @@ -292,7 +292,7 @@ bool RomInstaller::extract_ramdisk(const std::string &boot_image_file, Reader reader; // Open input boot image - if (auto r = reader.enable_format_all(); !r) { + if (auto r = reader.enable_formats_all(); !r) { LOGE("Failed to enable input boot image formats: %s", r.error().message().c_str()); return false;