From 0f76311c1db25cac791a56ef9a3da66f6cacecbb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?M=C3=A5rten=20Kongstad?= Date: Mon, 19 Nov 2018 14:14:37 +0100 Subject: [PATCH] idmap2: replace std::pair with Result Introduce a new type Result to indicate if an operation succeeded or not, and if it did, to hold the return value of the operation. This is the same as how std::pair is already used in the codebase, so replace all instances with Result to improve clarity. Result is simply an alias for std::optional. The difference is semantic: use Result as the return value for functions that can fail, use std::optional when values are truly optional. This is modelled after Rust's std::result and std::option. A future change may graduate Result to a proper class which can hold additional details on why an operation failed, such as a string or an error code. As a special case, continue to use std::unique_ptr instead of Result> for now: the latter would increase code complexity without added benefit. Test: make idmap2_tests Change-Id: I2a8355107ed2b6485409e5e655a84cf1e20b9911 --- cmds/idmap2/idmap2/Lookup.cpp | 55 ++++++++++---------- cmds/idmap2/include/idmap2/ResourceUtils.h | 5 +- cmds/idmap2/include/idmap2/Result.h | 31 +++++++++++ cmds/idmap2/include/idmap2/ZipFile.h | 4 +- cmds/idmap2/libidmap2/Idmap.cpp | 44 ++++++++-------- cmds/idmap2/libidmap2/PrettyPrintVisitor.cpp | 10 ++-- cmds/idmap2/libidmap2/RawPrintVisitor.cpp | 11 ++-- cmds/idmap2/libidmap2/ResourceUtils.cpp | 9 ++-- cmds/idmap2/libidmap2/ZipFile.cpp | 6 +-- cmds/idmap2/tests/ResourceUtilsTests.cpp | 15 +++--- cmds/idmap2/tests/ZipFileTests.cpp | 14 +++-- 11 files changed, 112 insertions(+), 92 deletions(-) create mode 100644 cmds/idmap2/include/idmap2/Result.h diff --git a/cmds/idmap2/idmap2/Lookup.cpp b/cmds/idmap2/idmap2/Lookup.cpp index 020c443582b21..8d0cee5b938d6 100644 --- a/cmds/idmap2/idmap2/Lookup.cpp +++ b/cmds/idmap2/idmap2/Lookup.cpp @@ -36,6 +36,7 @@ #include "idmap2/CommandLineOptions.h" #include "idmap2/Idmap.h" +#include "idmap2/Result.h" #include "idmap2/Xml.h" #include "idmap2/ZipFile.h" @@ -53,39 +54,40 @@ using android::base::StringPrintf; using android::idmap2::CommandLineOptions; using android::idmap2::IdmapHeader; using android::idmap2::ResourceId; +using android::idmap2::Result; using android::idmap2::Xml; using android::idmap2::ZipFile; using android::util::Utf16ToUtf8; namespace { -std::pair WARN_UNUSED ParseResReference(const AssetManager2& am, - const std::string& res, - const std::string& fallback_package) { + +Result WARN_UNUSED ParseResReference(const AssetManager2& am, const std::string& res, + const std::string& fallback_package) { // first, try to parse as a hex number char* endptr = nullptr; ResourceId resid; resid = strtol(res.c_str(), &endptr, 16); if (*endptr == '\0') { - return std::make_pair(true, resid); + return {resid}; } // next, try to parse as a package:type/name string resid = am.GetResourceId(res, "", fallback_package); if (is_valid_resid(resid)) { - return std::make_pair(true, resid); + return {resid}; } // end of the road: res could not be parsed - return std::make_pair(false, 0); + return {}; } -std::pair WARN_UNUSED GetValue(const AssetManager2& am, ResourceId resid) { +Result WARN_UNUSED GetValue(const AssetManager2& am, ResourceId resid) { Res_value value; ResTable_config config; uint32_t flags; ApkAssetsCookie cookie = am.GetResource(resid, false, 0, &value, &config, &flags); if (cookie == kInvalidCookie) { - return std::make_pair(false, ""); + return {}; } std::string out; @@ -123,31 +125,31 @@ std::pair WARN_UNUSED GetValue(const AssetManager2& am, Resou out.append(StringPrintf("dataType=0x%02x data=0x%08x", value.dataType, value.data)); break; } - return std::make_pair(true, out); + return {out}; } -std::pair GetTargetPackageNameFromManifest(const std::string& apk_path) { +Result GetTargetPackageNameFromManifest(const std::string& apk_path) { const auto zip = ZipFile::Open(apk_path); if (!zip) { - return std::make_pair(false, ""); + return {}; } const auto entry = zip->Uncompress("AndroidManifest.xml"); if (!entry) { - return std::make_pair(false, ""); + return {}; } const auto xml = Xml::Create(entry->buf, entry->size); if (!xml) { - return std::make_pair(false, ""); + return {}; } const auto tag = xml->FindTag("overlay"); if (!tag) { - return std::make_pair(false, ""); + return {}; } const auto iter = tag->find("targetPackage"); if (iter == tag->end()) { - return std::make_pair(false, ""); + return {}; } - return std::make_pair(true, iter->second); + return {iter->second}; } } // namespace @@ -195,14 +197,14 @@ bool Lookup(const std::vector& args, std::ostream& out_error) { } apk_assets.push_back(std::move(target_apk)); - bool lookup_ok; - std::tie(lookup_ok, target_package_name) = + const Result package_name = GetTargetPackageNameFromManifest(idmap_header->GetOverlayPath().to_string()); - if (!lookup_ok) { + if (!package_name) { out_error << "error: failed to parse android:targetPackage from overlay manifest" << std::endl; return false; } + target_package_name = *package_name; } else if (target_path != idmap_header->GetTargetPath()) { out_error << "error: different target APKs (expected target APK " << target_path << " but " << idmap_path << " has target APK " << idmap_header->GetTargetPath() << ")" @@ -227,21 +229,18 @@ bool Lookup(const std::vector& args, std::ostream& out_error) { am.SetApkAssets(raw_pointer_apk_assets); am.SetConfiguration(config); - ResourceId resid; - bool lookup_ok; - std::tie(lookup_ok, resid) = ParseResReference(am, resid_str, target_package_name); - if (!lookup_ok) { + const Result resid = ParseResReference(am, resid_str, target_package_name); + if (!resid) { out_error << "error: failed to parse resource ID" << std::endl; return false; } - std::string value; - std::tie(lookup_ok, value) = GetValue(am, resid); - if (!lookup_ok) { - out_error << StringPrintf("error: resource 0x%08x not found", resid) << std::endl; + const Result value = GetValue(am, *resid); + if (!value) { + out_error << StringPrintf("error: resource 0x%08x not found", *resid) << std::endl; return false; } - std::cout << value << std::endl; + std::cout << *value << std::endl; return true; } diff --git a/cmds/idmap2/include/idmap2/ResourceUtils.h b/cmds/idmap2/include/idmap2/ResourceUtils.h index 88a835b6439c9..d106f19af998b 100644 --- a/cmds/idmap2/include/idmap2/ResourceUtils.h +++ b/cmds/idmap2/include/idmap2/ResourceUtils.h @@ -18,19 +18,18 @@ #define IDMAP2_INCLUDE_IDMAP2_RESOURCEUTILS_H_ #include -#include #include "android-base/macros.h" #include "androidfw/AssetManager2.h" #include "idmap2/Idmap.h" +#include "idmap2/Result.h" namespace android { namespace idmap2 { namespace utils { -std::pair WARN_UNUSED ResToTypeEntryName(const AssetManager2& am, - ResourceId resid); +Result WARN_UNUSED ResToTypeEntryName(const AssetManager2& am, ResourceId resid); } // namespace utils } // namespace idmap2 diff --git a/cmds/idmap2/include/idmap2/Result.h b/cmds/idmap2/include/idmap2/Result.h new file mode 100644 index 0000000000000..6189ea371ee17 --- /dev/null +++ b/cmds/idmap2/include/idmap2/Result.h @@ -0,0 +1,31 @@ +/* + * Copyright (C) 2018 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#ifndef IDMAP2_INCLUDE_IDMAP2_RESULT_H_ +#define IDMAP2_INCLUDE_IDMAP2_RESULT_H_ + +#include + +namespace android::idmap2 { + +template +using Result = std::optional; + +static constexpr std::nullopt_t kResultError = std::nullopt; + +} // namespace android::idmap2 + +#endif // IDMAP2_INCLUDE_IDMAP2_RESULT_H_ diff --git a/cmds/idmap2/include/idmap2/ZipFile.h b/cmds/idmap2/include/idmap2/ZipFile.h index 328bd367adfc2..9edbbe0cce90d 100644 --- a/cmds/idmap2/include/idmap2/ZipFile.h +++ b/cmds/idmap2/include/idmap2/ZipFile.h @@ -19,10 +19,10 @@ #include #include -#include #include "android-base/macros.h" #include "ziparchive/zip_archive.h" +#include "idmap2/Result.h" namespace android { namespace idmap2 { @@ -43,7 +43,7 @@ class ZipFile { static std::unique_ptr Open(const std::string& path); std::unique_ptr Uncompress(const std::string& entryPath) const; - std::pair Crc(const std::string& entryPath) const; + Result Crc(const std::string& entryPath) const; ~ZipFile(); diff --git a/cmds/idmap2/libidmap2/Idmap.cpp b/cmds/idmap2/libidmap2/Idmap.cpp index 5a47e301b66c4..1ef326793cb4a 100644 --- a/cmds/idmap2/libidmap2/Idmap.cpp +++ b/cmds/idmap2/libidmap2/Idmap.cpp @@ -33,6 +33,7 @@ #include "idmap2/Idmap.h" #include "idmap2/ResourceUtils.h" +#include "idmap2/Result.h" #include "idmap2/ZipFile.h" namespace android { @@ -143,18 +144,16 @@ bool IdmapHeader::IsUpToDate(std::ostream& out_error) const { return false; } - bool status; - uint32_t target_crc; - std::tie(status, target_crc) = target_zip->Crc("resources.arsc"); - if (!status) { + Result target_crc = target_zip->Crc("resources.arsc"); + if (!target_crc) { out_error << "error: failed to get target crc" << std::endl; return false; } - if (target_crc_ != target_crc) { + if (target_crc_ != *target_crc) { out_error << base::StringPrintf( "error: bad target crc: idmap version 0x%08x, file system version 0x%08x", - target_crc_, target_crc) + target_crc_, *target_crc) << std::endl; return false; } @@ -165,17 +164,16 @@ bool IdmapHeader::IsUpToDate(std::ostream& out_error) const { return false; } - uint32_t overlay_crc; - std::tie(status, overlay_crc) = overlay_zip->Crc("resources.arsc"); - if (!status) { + Result overlay_crc = overlay_zip->Crc("resources.arsc"); + if (!overlay_crc) { out_error << "error: failed to get overlay crc" << std::endl; return false; } - if (overlay_crc_ != overlay_crc) { + if (overlay_crc_ != *overlay_crc) { out_error << base::StringPrintf( "error: bad overlay crc: idmap version 0x%08x, file system version 0x%08x", - overlay_crc_, overlay_crc) + overlay_crc_, *overlay_crc) << std::endl; return false; } @@ -322,17 +320,20 @@ std::unique_ptr Idmap::FromApkAssets(const std::string& target_apk_ std::unique_ptr header(new IdmapHeader()); header->magic_ = kIdmapMagic; header->version_ = kIdmapCurrentVersion; - bool crc_status; - std::tie(crc_status, header->target_crc_) = target_zip->Crc("resources.arsc"); - if (!crc_status) { + + Result crc = target_zip->Crc("resources.arsc"); + if (!crc) { out_error << "error: failed to get zip crc for target" << std::endl; return nullptr; } - std::tie(crc_status, header->overlay_crc_) = overlay_zip->Crc("resources.arsc"); - if (!crc_status) { + header->target_crc_ = *crc; + + crc = overlay_zip->Crc("resources.arsc"); + if (!crc) { out_error << "error: failed to get zip crc for overlay" << std::endl; return nullptr; } + header->overlay_crc_ = *crc; if (target_apk_path.size() > sizeof(header->target_path_)) { out_error << "error: target apk path \"" << target_apk_path << "\" longer that maximum size " @@ -358,15 +359,14 @@ std::unique_ptr Idmap::FromApkAssets(const std::string& target_apk_ const auto end = overlay_pkg->end(); for (auto iter = overlay_pkg->begin(); iter != end; ++iter) { const ResourceId overlay_resid = *iter; - bool lookup_ok; - std::string name; - std::tie(lookup_ok, name) = utils::ResToTypeEntryName(overlay_asset_manager, overlay_resid); - if (!lookup_ok) { + Result name = utils::ResToTypeEntryName(overlay_asset_manager, overlay_resid); + if (!name) { continue; } // prepend ":" to turn name into ":/" - name = base::StringPrintf("%s:%s", target_pkg->GetPackageName().c_str(), name.c_str()); - const ResourceId target_resid = NameToResid(target_asset_manager, name); + const std::string full_name = + base::StringPrintf("%s:%s", target_pkg->GetPackageName().c_str(), name->c_str()); + const ResourceId target_resid = NameToResid(target_asset_manager, full_name); if (target_resid == 0) { continue; } diff --git a/cmds/idmap2/libidmap2/PrettyPrintVisitor.cpp b/cmds/idmap2/libidmap2/PrettyPrintVisitor.cpp index 492e6f049d686..fb3bc5ba9ae8d 100644 --- a/cmds/idmap2/libidmap2/PrettyPrintVisitor.cpp +++ b/cmds/idmap2/libidmap2/PrettyPrintVisitor.cpp @@ -15,7 +15,6 @@ */ #include -#include #include "android-base/macros.h" #include "android-base/stringprintf.h" @@ -23,6 +22,7 @@ #include "idmap2/PrettyPrintVisitor.h" #include "idmap2/ResourceUtils.h" +#include "idmap2/Result.h" namespace android { namespace idmap2 { @@ -63,11 +63,9 @@ void PrettyPrintVisitor::visit(const IdmapData::TypeEntry& te) { stream_ << base::StringPrintf("0x%08x -> 0x%08x", target_resid, overlay_resid); if (target_package_loaded) { - bool lookup_ok; - std::string name; - std::tie(lookup_ok, name) = utils::ResToTypeEntryName(target_am_, target_resid); - if (lookup_ok) { - stream_ << " " << name; + Result name = utils::ResToTypeEntryName(target_am_, target_resid); + if (name) { + stream_ << " " << *name; } } stream_ << std::endl; diff --git a/cmds/idmap2/libidmap2/RawPrintVisitor.cpp b/cmds/idmap2/libidmap2/RawPrintVisitor.cpp index 57cfc8ef85b49..7c24445ef902f 100644 --- a/cmds/idmap2/libidmap2/RawPrintVisitor.cpp +++ b/cmds/idmap2/libidmap2/RawPrintVisitor.cpp @@ -16,7 +16,6 @@ #include #include -#include #include "android-base/macros.h" #include "android-base/stringprintf.h" @@ -24,6 +23,7 @@ #include "idmap2/RawPrintVisitor.h" #include "idmap2/ResourceUtils.h" +#include "idmap2/Result.h" using android::ApkAssets; @@ -75,14 +75,13 @@ void RawPrintVisitor::visit(const IdmapData::TypeEntry& te) { const ResourceId target_resid = RESID(last_seen_package_id_, te.GetTargetTypeId(), te.GetEntryOffset() + i); const ResourceId overlay_resid = RESID(last_seen_package_id_, te.GetOverlayTypeId(), entry); - bool lookup_ok = false; - std::string name; + Result name; if (target_package_loaded) { - std::tie(lookup_ok, name) = utils::ResToTypeEntryName(target_am_, target_resid); + name = utils::ResToTypeEntryName(target_am_, target_resid); } - if (lookup_ok) { + if (name) { print(static_cast(entry), "0x%08x -> 0x%08x %s", target_resid, overlay_resid, - name.c_str()); + name->c_str()); } else { print(static_cast(entry), "0x%08x -> 0x%08x", target_resid, overlay_resid); } diff --git a/cmds/idmap2/libidmap2/ResourceUtils.cpp b/cmds/idmap2/libidmap2/ResourceUtils.cpp index e98f843931c89..5c897832e9d5e 100644 --- a/cmds/idmap2/libidmap2/ResourceUtils.cpp +++ b/cmds/idmap2/libidmap2/ResourceUtils.cpp @@ -15,12 +15,12 @@ */ #include -#include #include "androidfw/StringPiece.h" #include "androidfw/Util.h" #include "idmap2/ResourceUtils.h" +#include "idmap2/Result.h" using android::StringPiece16; using android::util::Utf16ToUtf8; @@ -29,11 +29,10 @@ namespace android { namespace idmap2 { namespace utils { -std::pair WARN_UNUSED ResToTypeEntryName(const AssetManager2& am, - ResourceId resid) { +Result WARN_UNUSED ResToTypeEntryName(const AssetManager2& am, ResourceId resid) { AssetManager2::ResourceName name; if (!am.GetResourceName(resid, &name)) { - return std::make_pair(false, ""); + return {}; } std::string out; if (name.type != nullptr) { @@ -47,7 +46,7 @@ std::pair WARN_UNUSED ResToTypeEntryName(const AssetManager2& } else { out += Utf16ToUtf8(StringPiece16(name.entry16, name.entry_len)); } - return std::make_pair(true, out); + return {out}; } } // namespace utils diff --git a/cmds/idmap2/libidmap2/ZipFile.cpp b/cmds/idmap2/libidmap2/ZipFile.cpp index 3f2079a380d68..9fb611dd8e8dd 100644 --- a/cmds/idmap2/libidmap2/ZipFile.cpp +++ b/cmds/idmap2/libidmap2/ZipFile.cpp @@ -16,8 +16,8 @@ #include #include -#include +#include "idmap2/Result.h" #include "idmap2/ZipFile.h" namespace android { @@ -57,10 +57,10 @@ std::unique_ptr ZipFile::Uncompress(const std::string& entryP return chunk; } -std::pair ZipFile::Crc(const std::string& entryPath) const { +Result ZipFile::Crc(const std::string& entryPath) const { ::ZipEntry entry; int32_t status = ::FindEntry(handle_, ::ZipString(entryPath.c_str()), &entry); - return std::make_pair(status == 0, entry.crc32); + return status == 0 ? Result(entry.crc32) : kResultError; } } // namespace idmap2 diff --git a/cmds/idmap2/tests/ResourceUtilsTests.cpp b/cmds/idmap2/tests/ResourceUtilsTests.cpp index 0547fa00de3d3..7f60d7529a61b 100644 --- a/cmds/idmap2/tests/ResourceUtilsTests.cpp +++ b/cmds/idmap2/tests/ResourceUtilsTests.cpp @@ -16,13 +16,13 @@ #include #include -#include #include "gmock/gmock.h" #include "gtest/gtest.h" #include "androidfw/ApkAssets.h" #include "idmap2/ResourceUtils.h" +#include "idmap2/Result.h" #include "TestHelpers.h" @@ -52,17 +52,14 @@ class ResourceUtilsTests : public Idmap2Tests { }; TEST_F(ResourceUtilsTests, ResToTypeEntryName) { - bool lookup_ok; - std::string name; - std::tie(lookup_ok, name) = utils::ResToTypeEntryName(GetAssetManager(), 0x7f010000u); - ASSERT_TRUE(lookup_ok); - ASSERT_EQ(name, "integer/int1"); + Result name = utils::ResToTypeEntryName(GetAssetManager(), 0x7f010000u); + ASSERT_TRUE(name); + ASSERT_EQ(*name, "integer/int1"); } TEST_F(ResourceUtilsTests, ResToTypeEntryNameNoSuchResourceId) { - bool lookup_ok; - std::tie(lookup_ok, std::ignore) = utils::ResToTypeEntryName(GetAssetManager(), 0x7f123456u); - ASSERT_FALSE(lookup_ok); + Result name = utils::ResToTypeEntryName(GetAssetManager(), 0x7f123456u); + ASSERT_FALSE(name); } } // namespace idmap2 diff --git a/cmds/idmap2/tests/ZipFileTests.cpp b/cmds/idmap2/tests/ZipFileTests.cpp index a504d3126c057..6e4a501a51ac9 100644 --- a/cmds/idmap2/tests/ZipFileTests.cpp +++ b/cmds/idmap2/tests/ZipFileTests.cpp @@ -16,8 +16,8 @@ #include // fclose #include -#include +#include "idmap2/Result.h" #include "idmap2/ZipFile.h" #include "gmock/gmock.h" @@ -44,14 +44,12 @@ TEST(ZipFileTests, Crc) { auto zip = ZipFile::Open(GetTestDataPath() + "/target/target.apk"); ASSERT_THAT(zip, NotNull()); - bool status; - uint32_t crc; - std::tie(status, crc) = zip->Crc("AndroidManifest.xml"); - ASSERT_TRUE(status); - ASSERT_EQ(crc, 0x762f3d24); + Result crc = zip->Crc("AndroidManifest.xml"); + ASSERT_TRUE(crc); + ASSERT_EQ(*crc, 0x762f3d24); - std::tie(status, std::ignore) = zip->Crc("does-not-exist"); - ASSERT_FALSE(status); + Result crc2 = zip->Crc("does-not-exist"); + ASSERT_FALSE(crc2); } TEST(ZipFileTests, Uncompress) {