Merge "Increases fastbootd response message size"
diff --git a/base/include/android-base/expected.h b/base/include/android-base/expected.h
index 44e0b4a..9603bb1 100644
--- a/base/include/android-base/expected.h
+++ b/base/include/android-base/expected.h
@@ -331,6 +331,7 @@
 
   constexpr explicit operator bool() const noexcept { return has_value(); }
   constexpr bool has_value() const noexcept { return var_.index() == 0; }
+  constexpr bool ok() const noexcept { return has_value(); }
 
   constexpr const T& value() const& { return std::get<T>(var_); }
   constexpr T& value() & { return std::get<T>(var_); }
@@ -557,6 +558,7 @@
   // observers
   constexpr explicit operator bool() const noexcept { return has_value(); }
   constexpr bool has_value() const noexcept { return var_.index() == 0; }
+  constexpr bool ok() const noexcept { return has_value(); }
 
   constexpr void value() const& { if (!has_value()) std::get<0>(var_); }
 
diff --git a/base/include/android-base/result.h b/base/include/android-base/result.h
index b6d26e7..4a59552 100644
--- a/base/include/android-base/result.h
+++ b/base/include/android-base/result.h
@@ -156,11 +156,11 @@
   Error& operator=(const Error&) = delete;
   Error& operator=(Error&&) = delete;
 
-  template <typename... Args>
-  friend Error Errorf(const char* fmt, const Args&... args);
+  template <typename T, typename... Args>
+  friend Error ErrorfImpl(const T&& fmt, const Args&... args);
 
-  template <typename... Args>
-  friend Error ErrnoErrorf(const char* fmt, const Args&... args);
+  template <typename T, typename... Args>
+  friend Error ErrnoErrorfImpl(const T&& fmt, const Args&... args);
 
  private:
   Error(bool append_errno, int errno_to_append, const std::string& message)
@@ -191,18 +191,49 @@
   return ErrorCode(code, args...);
 }
 
-template <typename... Args>
-inline Error Errorf(const char* fmt, const Args&... args) {
+// TODO(tomcherry): Remove this once we've removed all `using android::base::Errorf` and `using
+// android::base::ErrnoErrorf` lines.
+enum Errorf {};
+enum ErrnoErrorf {};
+
+template <typename T, typename... Args>
+inline Error ErrorfImpl(const T&& fmt, const Args&... args) {
   return Error(false, ErrorCode(0, args...), fmt::format(fmt, args...));
 }
 
-template <typename... Args>
-inline Error ErrnoErrorf(const char* fmt, const Args&... args) {
+template <typename T, typename... Args>
+inline Error ErrnoErrorfImpl(const T&& fmt, const Args&... args) {
   return Error(true, errno, fmt::format(fmt, args...));
 }
 
+#define Errorf(fmt, ...) android::base::ErrorfImpl(FMT_STRING(fmt), ##__VA_ARGS__)
+#define ErrnoErrorf(fmt, ...) android::base::ErrnoErrorfImpl(FMT_STRING(fmt), ##__VA_ARGS__)
+
 template <typename T>
 using Result = android::base::expected<T, ResultError>;
 
+// Macros for testing the results of functions that return android::base::Result.
+// These also work with base::android::expected.
+
+#define CHECK_RESULT_OK(stmt)       \
+  do {                              \
+    const auto& tmp = (stmt);       \
+    CHECK(tmp.ok()) << tmp.error(); \
+  } while (0)
+
+#define ASSERT_RESULT_OK(stmt)            \
+  do {                                    \
+    const auto& tmp = (stmt);             \
+    ASSERT_TRUE(tmp.ok()) << tmp.error(); \
+  } while (0)
+
+#define EXPECT_RESULT_OK(stmt)            \
+  do {                                    \
+    auto tmp = (stmt);                    \
+    EXPECT_TRUE(tmp.ok()) << tmp.error(); \
+  } while (0)
+
+// TODO: Maybe add RETURN_IF_ERROR() and ASSIGN_OR_RETURN()
+
 }  // namespace base
 }  // namespace android
diff --git a/base/result_test.cpp b/base/result_test.cpp
index 2ee4057..0ea62e8 100644
--- a/base/result_test.cpp
+++ b/base/result_test.cpp
@@ -362,7 +362,7 @@
   result = Errorf("{} {}!", std::string("hello"), std::string("world"));
   EXPECT_EQ("hello world!", result.error().message());
 
-  result = Errorf("{h} {w}!", fmt::arg("w", "world"), fmt::arg("h", "hello"));
+  result = Errorf("{1} {0}!", "world", "hello");
   EXPECT_EQ("hello world!", result.error().message());
 
   result = Errorf("hello world!");
diff --git a/fs_mgr/TEST_MAPPING b/fs_mgr/TEST_MAPPING
index 60e3226..705d4e3 100644
--- a/fs_mgr/TEST_MAPPING
+++ b/fs_mgr/TEST_MAPPING
@@ -11,6 +11,9 @@
     },
     {
       "name": "fiemap_writer_test"
+    },
+    {
+      "name": "vts_libsnapshot_test_presubmit"
     }
   ]
 }
diff --git a/fs_mgr/libsnapshot/Android.bp b/fs_mgr/libsnapshot/Android.bp
index c58101a..bc177a0 100644
--- a/fs_mgr/libsnapshot/Android.bp
+++ b/fs_mgr/libsnapshot/Android.bp
@@ -160,8 +160,8 @@
     ],
 }
 
-cc_test {
-    name: "libsnapshot_test",
+cc_defaults {
+    name: "libsnapshot_test_defaults",
     defaults: ["libsnapshot_defaults"],
     srcs: [
         "partition_cow_creator_test.cpp",
@@ -173,10 +173,7 @@
         "android.hardware.boot@1.1",
         "libbinder",
         "libcrypto",
-        "libfs_mgr",
-        "libgsi",
         "libhidlbase",
-        "liblp",
         "libprotobuf-cpp-lite",
         "libsparse",
         "libutils",
@@ -186,16 +183,38 @@
         "libutilscallstack",
     ],
     static_libs: [
+        "libfs_mgr",
+        "libgsi",
         "libgmock",
+        "liblp",
         "libsnapshot",
         "libsnapshot_test_helpers",
     ],
     header_libs: [
         "libstorage_literals_headers",
     ],
+    test_suites: [
+        "vts-core",
+        "device-tests"
+    ],
+    test_min_api_level: 29,
+    auto_gen_config: true,
     require_root: true,
 }
 
+cc_test {
+    name: "vts_libsnapshot_test",
+    defaults: ["libsnapshot_test_defaults"],
+}
+
+cc_test {
+    name: "vts_libsnapshot_test_presubmit",
+    defaults: ["libsnapshot_test_defaults"],
+    cppflags: [
+        "-DSKIP_TEST_IN_PRESUBMIT",
+    ],
+}
+
 cc_binary {
     name: "snapshotctl",
     srcs: [
diff --git a/fs_mgr/libsnapshot/device_info.cpp b/fs_mgr/libsnapshot/device_info.cpp
index bacb41c..0e90100 100644
--- a/fs_mgr/libsnapshot/device_info.cpp
+++ b/fs_mgr/libsnapshot/device_info.cpp
@@ -22,6 +22,7 @@
 namespace snapshot {
 
 #ifdef LIBSNAPSHOT_USE_HAL
+using android::hardware::boot::V1_0::BoolResult;
 using android::hardware::boot::V1_0::CommandResult;
 #endif
 
diff --git a/fs_mgr/libsnapshot/include/libsnapshot/snapshot.h b/fs_mgr/libsnapshot/include/libsnapshot/snapshot.h
index e786bc9..ed92dd7 100644
--- a/fs_mgr/libsnapshot/include/libsnapshot/snapshot.h
+++ b/fs_mgr/libsnapshot/include/libsnapshot/snapshot.h
@@ -174,6 +174,7 @@
     // See InitiateMerge() and ProcessUpdateState() for details.
     // Returns:
     //   - None if no merge to initiate
+    //   - Unverified if called on the source slot
     //   - MergeCompleted if merge is completed
     //   - other states indicating an error has occurred
     UpdateState InitiateMergeAndWait();
@@ -273,6 +274,7 @@
     FRIEND_TEST(SnapshotTest, UpdateBootControlHal);
     FRIEND_TEST(SnapshotUpdateTest, DataWipeAfterRollback);
     FRIEND_TEST(SnapshotUpdateTest, DataWipeRollbackInRecovery);
+    FRIEND_TEST(SnapshotUpdateTest, FullUpdateFlow);
     FRIEND_TEST(SnapshotUpdateTest, MergeCannotRemoveCow);
     FRIEND_TEST(SnapshotUpdateTest, MergeInRecovery);
     FRIEND_TEST(SnapshotUpdateTest, SnapshotStatusFileWithoutCow);
@@ -374,7 +376,7 @@
     bool HandleCancelledUpdate(LockedFile* lock);
 
     // Helper for HandleCancelledUpdate. Assumes booting from new slot.
-    bool HandleCancelledUpdateOnNewSlot(LockedFile* lock);
+    bool AreAllSnapshotsCancelled(LockedFile* lock);
 
     // Remove artifacts created by the update process, such as snapshots, and
     // set the update state to None.
@@ -439,7 +441,7 @@
     std::string GetSnapshotStatusFilePath(const std::string& name);
 
     std::string GetSnapshotBootIndicatorPath();
-    void RemoveSnapshotBootIndicator();
+    std::string GetRollbackIndicatorPath();
 
     // Return the name of the device holding the "snapshot" or "snapshot-merge"
     // target. This may not be the final device presented via MapSnapshot(), if
@@ -503,6 +505,8 @@
     friend std::ostream& operator<<(std::ostream& os, SnapshotManager::Slot slot);
     Slot GetCurrentSlot();
 
+    std::string ReadUpdateSourceSlotSuffix();
+
     std::string gsid_dir_;
     std::string metadata_dir_;
     std::unique_ptr<IDeviceInfo> device_;
diff --git a/fs_mgr/libsnapshot/snapshot.cpp b/fs_mgr/libsnapshot/snapshot.cpp
index 63a9302..a937b43 100644
--- a/fs_mgr/libsnapshot/snapshot.cpp
+++ b/fs_mgr/libsnapshot/snapshot.cpp
@@ -188,11 +188,19 @@
     return true;
 }
 
-SnapshotManager::Slot SnapshotManager::GetCurrentSlot() {
+std::string SnapshotManager::ReadUpdateSourceSlotSuffix() {
     auto boot_file = GetSnapshotBootIndicatorPath();
     std::string contents;
     if (!android::base::ReadFileToString(boot_file, &contents)) {
         PLOG(WARNING) << "Cannot read " << boot_file;
+        return {};
+    }
+    return contents;
+}
+
+SnapshotManager::Slot SnapshotManager::GetCurrentSlot() {
+    auto contents = ReadUpdateSourceSlotSuffix();
+    if (contents.empty()) {
         return Slot::Unknown;
     }
     if (device_->GetSlotSuffix() == contents) {
@@ -201,6 +209,15 @@
     return Slot::Target;
 }
 
+static bool RemoveFileIfExists(const std::string& path) {
+    std::string message;
+    if (!android::base::RemoveFileIfExists(path, &message)) {
+        LOG(ERROR) << "Remove failed: " << path << ": " << message;
+        return false;
+    }
+    return true;
+}
+
 bool SnapshotManager::RemoveAllUpdateState(LockedFile* lock) {
     LOG(INFO) << "Removing all update state.";
 
@@ -223,7 +240,13 @@
         return false;
     }
 
-    RemoveSnapshotBootIndicator();
+    // It's okay if these fail - first-stage init performs a deeper check after
+    // reading the indicator file, so it's not a problem if it still exists
+    // after the update completes.
+    std::vector<std::string> files = {GetSnapshotBootIndicatorPath(), GetRollbackIndicatorPath()};
+    for (const auto& file : files) {
+        RemoveFileIfExists(file);
+    }
 
     // If this fails, we'll keep trying to remove the update state (as the
     // device reboots or starts a new update) until it finally succeeds.
@@ -250,6 +273,13 @@
         return false;
     }
 
+    // This file is written on boot to detect whether a rollback occurred. It
+    // MUST NOT exist before rebooting, otherwise, we're at risk of deleting
+    // snapshots too early.
+    if (!RemoveFileIfExists(GetRollbackIndicatorPath())) {
+        return false;
+    }
+
     // This file acts as both a quick indicator for init (it can use access(2)
     // to decide how to do first-stage mounts), and it stores the old slot, so
     // we can tell whether or not we performed a rollback.
@@ -966,14 +996,8 @@
     return metadata_dir_ + "/" + android::base::Basename(kBootIndicatorPath);
 }
 
-void SnapshotManager::RemoveSnapshotBootIndicator() {
-    // It's okay if this fails - first-stage init performs a deeper check after
-    // reading the indicator file, so it's not a problem if it still exists
-    // after the update completes.
-    auto boot_file = GetSnapshotBootIndicatorPath();
-    if (unlink(boot_file.c_str()) == -1 && errno != ENOENT) {
-        PLOG(ERROR) << "unlink " << boot_file;
-    }
+std::string SnapshotManager::GetRollbackIndicatorPath() {
+    return metadata_dir_ + "/rollback-indicator";
 }
 
 void SnapshotManager::AcknowledgeMergeSuccess(LockedFile* lock) {
@@ -1144,25 +1168,18 @@
     if (slot == Slot::Unknown) {
         return false;
     }
-    if (slot == Slot::Target) {
-        // We're booted into the target slot, which means we just rebooted
-        // after applying the update.
-        if (!HandleCancelledUpdateOnNewSlot(lock)) {
-            return false;
-        }
+
+    // If all snapshots were reflashed, then cancel the entire update.
+    if (AreAllSnapshotsCancelled(lock)) {
+        RemoveAllUpdateState(lock);
+        return true;
     }
 
-    // The only way we can get here is if:
-    //  (1) The device rolled back to the previous slot.
-    //  (2) This function was called prematurely before rebooting the device.
-    //  (3) fastboot set_active was used.
-    //  (4) The device updates to the new slot but re-flashed *all* partitions
-    //      in the new slot.
-    //
-    // In any case, delete the snapshots. It may be worth using the boot_control
-    // HAL to differentiate case (2).
-    RemoveAllUpdateState(lock);
-    return true;
+    // This unverified update might be rolled back, or it might not (b/147347110
+    // comment #77). Take no action, as update_engine is responsible for deciding
+    // whether to cancel.
+    LOG(ERROR) << "Update state is being processed before reboot, taking no action.";
+    return false;
 }
 
 std::unique_ptr<LpMetadata> SnapshotManager::ReadCurrentMetadata() {
@@ -1187,7 +1204,7 @@
     return MetadataPartitionState::Flashed;
 }
 
-bool SnapshotManager::HandleCancelledUpdateOnNewSlot(LockedFile* lock) {
+bool SnapshotManager::AreAllSnapshotsCancelled(LockedFile* lock) {
     std::vector<std::string> snapshots;
     if (!ListSnapshots(lock, &snapshots)) {
         LOG(WARNING) << "Failed to list snapshots to determine whether device has been flashed "
@@ -1196,35 +1213,45 @@
         return true;
     }
 
+    auto source_slot_suffix = ReadUpdateSourceSlotSuffix();
+    if (source_slot_suffix.empty()) {
+        return false;
+    }
+    uint32_t source_slot = SlotNumberForSlotSuffix(source_slot_suffix);
+    uint32_t target_slot = (source_slot == 0) ? 1 : 0;
+
     // Attempt to detect re-flashing on each partition.
     // - If all partitions are re-flashed, we can proceed to cancel the whole update.
     // - If only some of the partitions are re-flashed, snapshots for re-flashed partitions are
     //   deleted. Caller is responsible for merging the rest of the snapshots.
     // - If none of the partitions are re-flashed, caller is responsible for merging the snapshots.
-    auto metadata = ReadCurrentMetadata();
-    if (!metadata) return false;
-    bool all_snapshot_cancelled = true;
+    //
+    // Note that we use target slot metadata, since if an OTA has been applied
+    // to the target slot, we can detect the UPDATED flag. Any kind of flash
+    // operation against dynamic partitions ensures that all copies of the
+    // metadata are in sync, so flashing all partitions on the source slot will
+    // remove the UPDATED flag on the target slot as well.
+    const auto& opener = device_->GetPartitionOpener();
+    auto super_device = device_->GetSuperDevice(target_slot);
+    auto metadata = android::fs_mgr::ReadMetadata(opener, super_device, target_slot);
+    if (!metadata) {
+        return false;
+    }
+
+    bool all_snapshots_cancelled = true;
     for (const auto& snapshot_name : snapshots) {
         if (GetMetadataPartitionState(*metadata, snapshot_name) ==
             MetadataPartitionState::Updated) {
-            LOG(WARNING) << "Cannot cancel update because snapshot" << snapshot_name
-                         << " is in use.";
-            all_snapshot_cancelled = false;
+            all_snapshots_cancelled = false;
             continue;
         }
         // Delete snapshots for partitions that are re-flashed after the update.
-        LOG(INFO) << "Detected re-flashing of partition " << snapshot_name << ".";
-        if (!DeleteSnapshot(lock, snapshot_name)) {
-            // This is an error, but it is okay to leave the snapshot in the short term.
-            // However, if all_snapshot_cancelled == false after exiting the loop, caller may
-            // initiate merge for this unused snapshot, which is likely to fail.
-            LOG(WARNING) << "Failed to delete snapshot for re-flashed partition " << snapshot_name;
-        }
+        LOG(WARNING) << "Detected re-flashing of partition " << snapshot_name << ".";
     }
-    if (!all_snapshot_cancelled) return false;
-
-    LOG(INFO) << "All partitions are re-flashed after update, removing all update states.";
-    return true;
+    if (all_snapshots_cancelled) {
+        LOG(WARNING) << "All partitions are re-flashed after update, removing all update states.";
+    }
+    return all_snapshots_cancelled;
 }
 
 bool SnapshotManager::RemoveAllSnapshots(LockedFile* lock) {
@@ -1344,7 +1371,16 @@
     // the reason be clearer? Because the indicator file still exists, and
     // if this was FATAL, reverting to the old slot would be broken.
     auto slot = GetCurrentSlot();
+
     if (slot != Slot::Target) {
+        if (slot == Slot::Source && !device_->IsRecovery()) {
+            // Device is rebooting into the original slot, so mark this as a
+            // rollback.
+            auto path = GetRollbackIndicatorPath();
+            if (!android::base::WriteStringToFile("1", path)) {
+                PLOG(ERROR) << "Unable to write rollback indicator: " << path;
+            }
+        }
         LOG(INFO) << "Not booting from new slot. Will not mount snapshots.";
         return false;
     }
@@ -2370,6 +2406,10 @@
         return state;
     }
     if (state == UpdateState::Unverified) {
+        if (GetCurrentSlot() != Slot::Target) {
+            LOG(INFO) << "Cannot merge until device reboots.";
+            return state;
+        }
         if (!InitiateMerge()) {
             LOG(ERROR) << "Failed to initiate merge.";
             return state;
diff --git a/fs_mgr/libsnapshot/snapshot_test.cpp b/fs_mgr/libsnapshot/snapshot_test.cpp
index c5ad44c..d87274d 100644
--- a/fs_mgr/libsnapshot/snapshot_test.cpp
+++ b/fs_mgr/libsnapshot/snapshot_test.cpp
@@ -447,6 +447,9 @@
     auto sm = SnapshotManager::NewForFirstStageMount(info);
     ASSERT_NE(sm, nullptr);
     ASSERT_FALSE(sm->NeedSnapshotsInFirstStageMount());
+
+    auto indicator = sm->GetRollbackIndicatorPath();
+    ASSERT_EQ(access(indicator.c_str(), R_OK), 0);
 }
 
 TEST_F(SnapshotTest, Merge) {
@@ -503,6 +506,9 @@
 }
 
 TEST_F(SnapshotTest, FirstStageMountAndMerge) {
+#ifdef SKIP_TEST_IN_PRESUBMIT
+    GTEST_SKIP() << "WIP failure b/148889015";
+#endif
     ASSERT_TRUE(AcquireLock());
 
     static const uint64_t kDeviceSize = 1024 * 1024;
@@ -559,6 +565,9 @@
 }
 
 TEST_F(SnapshotTest, FlashSuperDuringMerge) {
+#ifdef SKIP_TEST_IN_PRESUBMIT
+    GTEST_SKIP() << "WIP failure b/148889015";
+#endif
     ASSERT_TRUE(AcquireLock());
 
     static const uint64_t kDeviceSize = 1024 * 1024;
@@ -970,6 +979,9 @@
 // Also test UnmapUpdateSnapshot unmaps everything.
 // Also test first stage mount and merge after this.
 TEST_F(SnapshotUpdateTest, FullUpdateFlow) {
+#ifdef SKIP_TEST_IN_PRESUBMIT
+    GTEST_SKIP() << "WIP failure b/148889015";
+#endif
     // OTA client blindly unmaps all partitions that are possibly mapped.
     for (const auto& name : {"sys_b", "vnd_b", "prd_b"}) {
         ASSERT_TRUE(sm->UnmapUpdateSnapshot(name));
@@ -1015,6 +1027,9 @@
     ASSERT_TRUE(init->NeedSnapshotsInFirstStageMount());
     ASSERT_TRUE(init->CreateLogicalAndSnapshotPartitions("super", snapshot_timeout_));
 
+    auto indicator = sm->GetRollbackIndicatorPath();
+    ASSERT_NE(access(indicator.c_str(), R_OK), 0);
+
     // Check that the target partitions have the same content.
     for (const auto& name : {"sys_b", "vnd_b", "prd_b"}) {
         ASSERT_TRUE(IsPartitionUnchanged(name));
@@ -1114,6 +1129,9 @@
 
 // Test that the old partitions are not modified.
 TEST_F(SnapshotUpdateTest, TestRollback) {
+#ifdef SKIP_TEST_IN_PRESUBMIT
+    GTEST_SKIP() << "WIP failure b/148889015";
+#endif
     // Execute the update.
     ASSERT_TRUE(sm->BeginUpdate());
     ASSERT_TRUE(sm->UnmapUpdateSnapshot("sys_b"));
@@ -1291,6 +1309,9 @@
 }
 
 TEST_F(SnapshotUpdateTest, MergeCannotRemoveCow) {
+#ifdef SKIP_TEST_IN_PRESUBMIT
+    GTEST_SKIP() << "WIP failure b/148889015";
+#endif
     // Make source partitions as big as possible to force COW image to be created.
     SetSize(sys_, 5_MiB);
     SetSize(vnd_, 5_MiB);
@@ -1565,6 +1586,9 @@
 }
 
 TEST_F(SnapshotUpdateTest, WaitForMerge) {
+#ifdef SKIP_TEST_IN_PRESUBMIT
+    GTEST_SKIP() << "WIP failure b/148889015";
+#endif
     AddOperationForPartitions();
 
     // Execute the update.
@@ -1681,9 +1705,11 @@
     ASSERT_NE(nullptr, flashed_builder->FindPartition("prd" + flashed_slot_suffix));
     flashed_builder->RemovePartition("prd" + flashed_slot_suffix);
 
+    // Note that fastbootd always updates the partition table of both slots.
     auto flashed_metadata = flashed_builder->Export();
     ASSERT_NE(nullptr, flashed_metadata);
-    ASSERT_TRUE(UpdatePartitionTable(*opener_, "super", *flashed_metadata, flashed_slot));
+    ASSERT_TRUE(UpdatePartitionTable(*opener_, "super", *flashed_metadata, 0));
+    ASSERT_TRUE(UpdatePartitionTable(*opener_, "super", *flashed_metadata, 1));
 
     std::string path;
     for (const auto& name : {"sys", "vnd"}) {
@@ -1782,6 +1808,10 @@
     std::vector<uint64_t> ret;
     for (uint64_t size = 1_MiB; size <= 512_MiB; size *= 2) {
         ret.push_back(size);
+#ifdef SKIP_TEST_IN_PRESUBMIT
+        // BUG(148889015);
+        break;
+#endif
     }
     return ret;
 }
diff --git a/fs_mgr/libsnapshot/snapshotctl.cpp b/fs_mgr/libsnapshot/snapshotctl.cpp
index 9f23c45..d724be3 100644
--- a/fs_mgr/libsnapshot/snapshotctl.cpp
+++ b/fs_mgr/libsnapshot/snapshotctl.cpp
@@ -112,7 +112,9 @@
 
     auto state = SnapshotManager::New()->InitiateMergeAndWait();
 
-    if (state == UpdateState::None) {
+    // We could wind up in the Unverified state if the device rolled back or
+    // hasn't fully rebooted. Ignore this.
+    if (state == UpdateState::None || state == UpdateState::Unverified) {
         return true;
     }
     if (state == UpdateState::MergeCompleted) {
diff --git a/init/result.h b/init/result.h
index b70dd1b..8c1f91e 100644
--- a/init/result.h
+++ b/init/result.h
@@ -22,8 +22,6 @@
 #include <android-base/result.h>
 
 using android::base::ErrnoError;
-using android::base::ErrnoErrorf;
 using android::base::Error;
-using android::base::Errorf;
 using android::base::Result;
 using android::base::ResultError;
diff --git a/init/service_utils.cpp b/init/service_utils.cpp
index 93cffd8..eec5717 100644
--- a/init/service_utils.cpp
+++ b/init/service_utils.cpp
@@ -249,9 +249,8 @@
 
     for (const auto& rlimit : attr.rlimits) {
         if (setrlimit(rlimit.first, &rlimit.second) == -1) {
-            return ErrnoError() << StringPrintf(
-                           "setrlimit(%d, {rlim_cur=%ld, rlim_max=%ld}) failed", rlimit.first,
-                           rlimit.second.rlim_cur, rlimit.second.rlim_max);
+            return ErrnoErrorf("setrlimit({}, {{rlim_cur={}, rlim_max={}}}) failed", rlimit.first,
+                               rlimit.second.rlim_cur, rlimit.second.rlim_max);
         }
     }
 
diff --git a/liblog/logger_read.cpp b/liblog/logger_read.cpp
index a0c526b..4937042 100644
--- a/liblog/logger_read.cpp
+++ b/liblog/logger_read.cpp
@@ -109,8 +109,8 @@
     return ret;
   }
 
-  if (ret > (int)sizeof(*log_msg)) {
-    ret = sizeof(*log_msg);
+  if (ret > LOGGER_ENTRY_MAX_LEN) {
+    ret = LOGGER_ENTRY_MAX_LEN;
   }
 
   if (ret < static_cast<int>(sizeof(log_msg->entry))) {
@@ -118,7 +118,7 @@
   }
 
   if (log_msg->entry.hdr_size < sizeof(log_msg->entry) ||
-      log_msg->entry.hdr_size >= sizeof(struct log_msg) - sizeof(log_msg->entry)) {
+      log_msg->entry.hdr_size >= LOGGER_ENTRY_MAX_LEN - sizeof(log_msg->entry)) {
     return -EINVAL;
   }
 
@@ -126,6 +126,8 @@
     return -EINVAL;
   }
 
+  log_msg->buf[log_msg->entry.len + log_msg->entry.hdr_size] = '\0';
+
   return ret;
 }