From a5bc9585b2bd09868fb6e0339dd5b8e288303b7e Mon Sep 17 00:00:00 2001 From: Yurii Zubrytskyi Date: Wed, 30 Nov 2022 23:53:59 -0800 Subject: [PATCH] [res] Refactor AssetManager + Providers - More moves where possible - Better interface for PathOrDebugName creation - function_ref as a callback - Get rid of some obsolete Util.h code - Shut up the logging in host mode, and ignore all calls Bug: 237583012 Test: build + boot + UTs Change-Id: Ia71fc1c83f17ab5ce3cac1179f74534f7ad2a3cb --- core/jni/android_content_res_ApkAssets.cpp | 2 +- libs/androidfw/AssetManager2.cpp | 4 +- libs/androidfw/AssetsProvider.cpp | 45 +++++---- libs/androidfw/Idmap.cpp | 32 +++---- .../include/androidfw/AssetManager2.h | 2 +- .../include/androidfw/AssetsProvider.h | 25 +++-- libs/androidfw/include/androidfw/Idmap.h | 4 +- libs/androidfw/include/androidfw/Util.h | 94 ++++--------------- 8 files changed, 80 insertions(+), 128 deletions(-) diff --git a/core/jni/android_content_res_ApkAssets.cpp b/core/jni/android_content_res_ApkAssets.cpp index 29560dce1cd86..e9ada235b3888 100644 --- a/core/jni/android_content_res_ApkAssets.cpp +++ b/core/jni/android_content_res_ApkAssets.cpp @@ -96,7 +96,7 @@ class LoaderAssetsProvider : public AssetsProvider { } bool ForEachFile(const std::string& /* root_path */, - const std::function& /* f */) const { + android::base::function_ref /* f */) const { return true; } diff --git a/libs/androidfw/AssetManager2.cpp b/libs/androidfw/AssetManager2.cpp index cc7e8714c0aca..68f5e4a88c7ef 100644 --- a/libs/androidfw/AssetManager2.cpp +++ b/libs/androidfw/AssetManager2.cpp @@ -1562,11 +1562,11 @@ base::expected Theme::SetTo(const Theme& source) { std::unordered_map src_asset_cookie_id_map; // Determine which ApkAssets are loaded in both theme AssetManagers. - const auto src_assets = source.asset_manager_->GetApkAssets(); + const auto& src_assets = source.asset_manager_->GetApkAssets(); for (size_t i = 0; i < src_assets.size(); i++) { const ApkAssets* src_asset = src_assets[i]; - const auto dest_assets = asset_manager_->GetApkAssets(); + const auto& dest_assets = asset_manager_->GetApkAssets(); for (size_t j = 0; j < dest_assets.size(); j++) { const ApkAssets* dest_asset = dest_assets[j]; if (src_asset != dest_asset) { diff --git a/libs/androidfw/AssetsProvider.cpp b/libs/androidfw/AssetsProvider.cpp index b9264c5d0f2dd..2d3c06506a1fc 100644 --- a/libs/androidfw/AssetsProvider.cpp +++ b/libs/androidfw/AssetsProvider.cpp @@ -73,9 +73,6 @@ std::unique_ptr AssetsProvider::CreateAssetFromFd(base::unique_fd fd, (path != nullptr) ? base::unique_fd(-1) : std::move(fd)); } -ZipAssetsProvider::PathOrDebugName::PathOrDebugName(std::string&& value, bool is_path) - : value_(std::forward(value)), is_path_(is_path) {} - const std::string* ZipAssetsProvider::PathOrDebugName::GetPath() const { return is_path_ ? &value_ : nullptr; } @@ -84,10 +81,14 @@ const std::string& ZipAssetsProvider::PathOrDebugName::GetDebugName() const { return value_; } +void ZipAssetsProvider::ZipCloser::operator()(ZipArchive* a) const { + ::CloseArchive(a); +} + ZipAssetsProvider::ZipAssetsProvider(ZipArchiveHandle handle, PathOrDebugName&& path, package_property_t flags, time_t last_mod_time) - : zip_handle_(handle, ::CloseArchive), - name_(std::forward(path)), + : zip_handle_(handle), + name_(std::move(path)), flags_(flags), last_mod_time_(last_mod_time) {} @@ -110,14 +111,12 @@ std::unique_ptr ZipAssetsProvider::Create(std::string path, // Stat requires execute permissions on all directories path to the file. If the process does // not have execute permissions on this file, allow the zip to be opened but IsUpToDate() will // always have to return true. - LOG(WARNING) << "Failed to stat file '" << path << "': " - << base::SystemErrorCodeToString(errno); + PLOG(WARNING) << "Failed to stat file '" << path << "'"; } } return std::unique_ptr( - new ZipAssetsProvider(handle, PathOrDebugName{std::move(path), - true /* is_path */}, flags, sb.st_mtime)); + new ZipAssetsProvider(handle, PathOrDebugName::Path(std::move(path)), flags, sb.st_mtime)); } std::unique_ptr ZipAssetsProvider::Create(base::unique_fd fd, @@ -150,9 +149,8 @@ std::unique_ptr ZipAssetsProvider::Create(base::unique_fd fd, } } - return std::unique_ptr( - new ZipAssetsProvider(handle, PathOrDebugName{std::move(friendly_name), - false /* is_path */}, flags, sb.st_mtime)); + return std::unique_ptr(new ZipAssetsProvider( + handle, PathOrDebugName::DebugName(std::move(friendly_name)), flags, sb.st_mtime)); } std::unique_ptr ZipAssetsProvider::OpenInternal(const std::string& path, @@ -219,8 +217,9 @@ std::unique_ptr ZipAssetsProvider::OpenInternal(const std::string& path, return asset; } -bool ZipAssetsProvider::ForEachFile(const std::string& root_path, - const std::function& f) const { +bool ZipAssetsProvider::ForEachFile( + const std::string& root_path, + base::function_ref f) const { std::string root_path_full = root_path; if (root_path_full.back() != '/') { root_path_full += '/'; @@ -297,7 +296,7 @@ bool ZipAssetsProvider::IsUpToDate() const { } DirectoryAssetsProvider::DirectoryAssetsProvider(std::string&& path, time_t last_mod_time) - : dir_(std::forward(path)), last_mod_time_(last_mod_time) {} + : dir_(std::move(path)), last_mod_time_(last_mod_time) {} std::unique_ptr DirectoryAssetsProvider::Create(std::string path) { struct stat sb; @@ -312,7 +311,7 @@ std::unique_ptr DirectoryAssetsProvider::Create(std::st return nullptr; } - if (path[path.size() - 1] != OS_PATH_SEPARATOR) { + if (path.back() != OS_PATH_SEPARATOR) { path += OS_PATH_SEPARATOR; } @@ -335,7 +334,7 @@ std::unique_ptr DirectoryAssetsProvider::OpenInternal(const std::string& bool DirectoryAssetsProvider::ForEachFile( const std::string& /* root_path */, - const std::function& /* f */) const { + base::function_ref /* f */) const { return true; } @@ -362,8 +361,7 @@ bool DirectoryAssetsProvider::IsUpToDate() const { MultiAssetsProvider::MultiAssetsProvider(std::unique_ptr&& primary, std::unique_ptr&& secondary) - : primary_(std::forward>(primary)), - secondary_(std::forward>(secondary)) { + : primary_(std::move(primary)), secondary_(std::move(secondary)) { debug_name_ = primary_->GetDebugName() + " and " + secondary_->GetDebugName(); path_ = (primary_->GetDebugName() != kEmptyDebugString) ? primary_->GetPath() : secondary_->GetPath(); @@ -385,8 +383,9 @@ std::unique_ptr MultiAssetsProvider::OpenInternal(const std::string& path return (asset) ? std::move(asset) : secondary_->Open(path, mode, file_exists); } -bool MultiAssetsProvider::ForEachFile(const std::string& root_path, - const std::function& f) const { +bool MultiAssetsProvider::ForEachFile( + const std::string& root_path, + base::function_ref f) const { return primary_->ForEachFile(root_path, f) && secondary_->ForEachFile(root_path, f); } @@ -424,7 +423,7 @@ std::unique_ptr EmptyAssetsProvider::OpenInternal(const std::string& /* p bool EmptyAssetsProvider::ForEachFile( const std::string& /* root_path */, - const std::function& /* f */) const { + base::function_ref /* f */) const { return true; } @@ -447,4 +446,4 @@ bool EmptyAssetsProvider::IsUpToDate() const { return true; } -} // namespace android +} // namespace android diff --git a/libs/androidfw/Idmap.cpp b/libs/androidfw/Idmap.cpp index f3d244342b55d..89835742c8ff5 100644 --- a/libs/androidfw/Idmap.cpp +++ b/libs/androidfw/Idmap.cpp @@ -201,7 +201,7 @@ IdmapResMap::Result IdmapResMap::Lookup(uint32_t target_res_id) const { const auto& config = configurations_[value.config_index]; values_map[config] = value.value; } - return Result(values_map); + return Result(std::move(values_map)); } return {}; } @@ -250,8 +250,7 @@ std::optional ReadString(const uint8_t** in_out_data_ptr, size } } // namespace -LoadedIdmap::LoadedIdmap(std::string&& idmap_path, - const Idmap_header* header, +LoadedIdmap::LoadedIdmap(std::string&& idmap_path, const Idmap_header* header, const Idmap_data_header* data_header, const Idmap_target_entry* target_entries, const Idmap_target_entry_inline* target_inline_entries, @@ -259,20 +258,19 @@ LoadedIdmap::LoadedIdmap(std::string&& idmap_path, const ConfigDescription* configs, const Idmap_overlay_entry* overlay_entries, std::unique_ptr&& string_pool, - std::string_view overlay_apk_path, - std::string_view target_apk_path) - : header_(header), - data_header_(data_header), - target_entries_(target_entries), - target_inline_entries_(target_inline_entries), - inline_entry_values_(inline_entry_values), - configurations_(configs), - overlay_entries_(overlay_entries), - string_pool_(std::move(string_pool)), - idmap_path_(std::move(idmap_path)), - overlay_apk_path_(overlay_apk_path), - target_apk_path_(target_apk_path), - idmap_last_mod_time_(getFileModDate(idmap_path_.data())) {} + std::string_view overlay_apk_path, std::string_view target_apk_path) + : header_(header), + data_header_(data_header), + target_entries_(target_entries), + target_inline_entries_(target_inline_entries), + inline_entry_values_(inline_entry_values), + configurations_(configs), + overlay_entries_(overlay_entries), + string_pool_(std::move(string_pool)), + idmap_path_(std::move(idmap_path)), + overlay_apk_path_(overlay_apk_path), + target_apk_path_(target_apk_path), + idmap_last_mod_time_(getFileModDate(idmap_path_.data())) {} std::unique_ptr LoadedIdmap::Load(StringPiece idmap_path, StringPiece idmap_data) { ATRACE_CALL(); diff --git a/libs/androidfw/include/androidfw/AssetManager2.h b/libs/androidfw/include/androidfw/AssetManager2.h index e4d1218debe60..f10cb9bf480a1 100644 --- a/libs/androidfw/include/androidfw/AssetManager2.h +++ b/libs/androidfw/include/androidfw/AssetManager2.h @@ -105,7 +105,7 @@ class AssetManager2 { // new resource IDs. bool SetApkAssets(std::vector apk_assets, bool invalidate_caches = true); - inline const std::vector GetApkAssets() const { + inline const std::vector& GetApkAssets() const { return apk_assets_; } diff --git a/libs/androidfw/include/androidfw/AssetsProvider.h b/libs/androidfw/include/androidfw/AssetsProvider.h index 7891194a65bdb..d33c325ff369b 100644 --- a/libs/androidfw/include/androidfw/AssetsProvider.h +++ b/libs/androidfw/include/androidfw/AssetsProvider.h @@ -20,6 +20,7 @@ #include #include +#include "android-base/function_ref.h" #include "android-base/macros.h" #include "android-base/unique_fd.h" @@ -46,7 +47,7 @@ struct AssetsProvider { // Iterate over all files and directories provided by the interface. The order of iteration is // stable. virtual bool ForEachFile(const std::string& path, - const std::function& f) const = 0; + base::function_ref f) const = 0; // Retrieves the path to the contents of the AssetsProvider on disk. The path could represent an // APk, a directory, or some other file type. @@ -90,7 +91,7 @@ struct ZipAssetsProvider : public AssetsProvider { off64_t len = kUnknownLength); bool ForEachFile(const std::string& root_path, - const std::function& f) const override; + base::function_ref f) const override; WARN_UNUSED std::optional GetPath() const override; WARN_UNUSED const std::string& GetDebugName() const override; @@ -108,7 +109,12 @@ struct ZipAssetsProvider : public AssetsProvider { time_t last_mod_time); struct PathOrDebugName { - PathOrDebugName(std::string&& value, bool is_path); + static PathOrDebugName Path(std::string value) { + return {std::move(value), true}; + } + static PathOrDebugName DebugName(std::string value) { + return {std::move(value), false}; + } // Retrieves the path or null if this class represents a debug name. WARN_UNUSED const std::string* GetPath() const; @@ -117,11 +123,16 @@ struct ZipAssetsProvider : public AssetsProvider { WARN_UNUSED const std::string& GetDebugName() const; private: + PathOrDebugName(std::string value, bool is_path) : value_(std::move(value)), is_path_(is_path) { + } std::string value_; bool is_path_; }; - std::unique_ptr zip_handle_; + struct ZipCloser { + void operator()(ZipArchive* a) const; + }; + std::unique_ptr zip_handle_; PathOrDebugName name_; package_property_t flags_; time_t last_mod_time_; @@ -132,7 +143,7 @@ struct DirectoryAssetsProvider : public AssetsProvider { static std::unique_ptr Create(std::string root_dir); bool ForEachFile(const std::string& path, - const std::function& f) const override; + base::function_ref f) const override; WARN_UNUSED std::optional GetPath() const override; WARN_UNUSED const std::string& GetDebugName() const override; @@ -157,7 +168,7 @@ struct MultiAssetsProvider : public AssetsProvider { std::unique_ptr&& secondary); bool ForEachFile(const std::string& root_path, - const std::function& f) const override; + base::function_ref f) const override; WARN_UNUSED std::optional GetPath() const override; WARN_UNUSED const std::string& GetDebugName() const override; @@ -184,7 +195,7 @@ struct EmptyAssetsProvider : public AssetsProvider { static std::unique_ptr Create(std::string path); bool ForEachFile(const std::string& path, - const std::function& f) const override; + base::function_ref f) const override; WARN_UNUSED std::optional GetPath() const override; WARN_UNUSED const std::string& GetDebugName() const override; diff --git a/libs/androidfw/include/androidfw/Idmap.h b/libs/androidfw/include/androidfw/Idmap.h index f173e6ef9c164..60689128dffb3 100644 --- a/libs/androidfw/include/androidfw/Idmap.h +++ b/libs/androidfw/include/androidfw/Idmap.h @@ -93,8 +93,8 @@ class IdmapResMap { public: Result() = default; explicit Result(uint32_t value) : data_(value) {}; - explicit Result(const std::map &value) - : data_(value) { }; + explicit Result(std::map value) : data_(std::move(value)) { + } // Returns `true` if the resource is overlaid. explicit operator bool() const { diff --git a/libs/androidfw/include/androidfw/Util.h b/libs/androidfw/include/androidfw/Util.h index ae7c65f2d4e1b..a188abb7ecb51 100644 --- a/libs/androidfw/include/androidfw/Util.h +++ b/libs/androidfw/include/androidfw/Util.h @@ -22,7 +22,6 @@ #include #include -#include #include #include "androidfw/BigBuffer.h" @@ -33,7 +32,14 @@ #ifdef __ANDROID__ #define ANDROID_LOG(x) LOG(x) #else -#define ANDROID_LOG(x) std::stringstream() +namespace android { +// No default logging for aapt2, as it's too noisy for a command line dev tool. +struct NullLogger { + template + friend const NullLogger& operator<<(const NullLogger& l, const T&) { return l; } +}; +} +#define ANDROID_LOG(x) (android::NullLogger{}) #endif namespace android { @@ -49,76 +55,14 @@ std::unique_ptr make_unique(Args&&... args) { return std::unique_ptr(new T{std::forward(args)...}); } -// Based on std::unique_ptr, but uses free() to release malloc'ed memory -// without incurring the size increase of holding on to a custom deleter. -template -class unique_cptr { - public: - using pointer = typename std::add_pointer::type; - - constexpr unique_cptr() : ptr_(nullptr) {} - constexpr explicit unique_cptr(std::nullptr_t) : ptr_(nullptr) {} - explicit unique_cptr(pointer ptr) : ptr_(ptr) {} - unique_cptr(unique_cptr&& o) noexcept : ptr_(o.ptr_) { o.ptr_ = nullptr; } - - ~unique_cptr() { std::free(reinterpret_cast(ptr_)); } - - inline unique_cptr& operator=(unique_cptr&& o) noexcept { - if (&o == this) { - return *this; - } - - std::free(reinterpret_cast(ptr_)); - ptr_ = o.ptr_; - o.ptr_ = nullptr; - return *this; +// Based on std::unique_ptr, but uses free() to release malloc'ed memory. +struct FreeDeleter { + void operator()(void* ptr) const { + ::free(ptr); } - - inline unique_cptr& operator=(std::nullptr_t) { - std::free(reinterpret_cast(ptr_)); - ptr_ = nullptr; - return *this; - } - - pointer release() { - pointer result = ptr_; - ptr_ = nullptr; - return result; - } - - inline pointer get() const { return ptr_; } - - void reset(pointer ptr = pointer()) { - if (ptr == ptr_) { - return; - } - - pointer old_ptr = ptr_; - ptr_ = ptr; - std::free(reinterpret_cast(old_ptr)); - } - - inline void swap(unique_cptr& o) { std::swap(ptr_, o.ptr_); } - - inline explicit operator bool() const { return ptr_ != nullptr; } - - inline typename std::add_lvalue_reference::type operator*() const { return *ptr_; } - - inline pointer operator->() const { return ptr_; } - - inline bool operator==(const unique_cptr& o) const { return ptr_ == o.ptr_; } - - inline bool operator!=(const unique_cptr& o) const { return ptr_ != o.ptr_; } - - inline bool operator==(std::nullptr_t) const { return ptr_ == nullptr; } - - inline bool operator!=(std::nullptr_t) const { return ptr_ != nullptr; } - - private: - DISALLOW_COPY_AND_ASSIGN(unique_cptr); - - pointer ptr_; }; +template +using unique_cptr = std::unique_ptr; void ReadUtf16StringFromDevice(const uint16_t* src, size_t len, std::string* out); @@ -152,13 +96,13 @@ inline uint32_t DeviceToHost32(uint32_t value) { std::vector SplitAndLowercase(android::StringPiece str, char sep); -template -inline bool IsFourByteAligned(const incfs::map_ptr& data) { - return ((size_t)data.unsafe_ptr() & 0x3U) == 0; +inline bool IsFourByteAligned(const void* data) { + return ((uintptr_t)data & 0x3U) == 0; } -inline bool IsFourByteAligned(const void* data) { - return ((size_t)data & 0x3U) == 0; +template +inline bool IsFourByteAligned(const incfs::map_ptr& data) { + return IsFourByteAligned(data.unsafe_ptr()); } // Helper method to extract a UTF-16 string from a StringPool. If the string is stored as UTF-8,