13 files changed, 250 insertions, 263 deletions

diff --git a/applypatch/applypatch.cpp b/applypatch/applypatch.cpp
index e6fd5f6ae..b1f5607a6 100644
--- a/applypatch/applypatch.cpp
+++ b/applypatch/applypatch.cpp
@@ -376,24 +376,26 @@ static int FindMatchingPatch(const uint8_t* sha1, const std::vector<std::string>
   return -1;
 }
 
-int applypatch_check(const char* filename, const std::vector<std::string>& patch_sha1s) {
-  // It's okay to specify no SHA-1s; the check will pass if the LoadFileContents is successful.
-  // (Useful for reading partitions, where the filename encodes the SHA-1s; no need to check them
-  // twice.)
+int applypatch_check(const std::string& filename, const std::vector<std::string>& sha1s) {
+  if (!android::base::StartsWith(filename, "EMMC:")) {
+    return 1;
+  }
+
+  // The check will pass if LoadPartitionContents is successful, because the filename already
+  // encodes the desired SHA-1s.
   FileContents file;
-  if (LoadFileContents(filename, &file) != 0 ||
-      (!patch_sha1s.empty() && FindMatchingPatch(file.sha1, patch_sha1s) < 0)) {
+  if (LoadPartitionContents(filename, &file) != 0) {
     LOG(INFO) << "\"" << filename << "\" doesn't have any of expected SHA-1 sums; checking cache";
 
-    // If the source file is missing or corrupted, it might be because we were killed in the middle
-    // of patching it. A copy should have been made in cache_temp_source. If that file exists and
-    // matches the SHA-1 we're looking for, the check still passes.
+    // If the partition is corrupted, it might be because we were killed in the middle of patching
+    // it. A copy should have been made in cache_temp_source. If that file exists and matches the
+    // SHA-1 we're looking for, the check still passes.
     if (LoadFileContents(Paths::Get().cache_temp_source(), &file) != 0) {
       LOG(ERROR) << "Failed to load cache file";
       return 1;
     }
 
-    if (FindMatchingPatch(file.sha1, patch_sha1s) < 0) {
+    if (FindMatchingPatch(file.sha1, sha1s) < 0) {
       LOG(ERROR) << "The cache bits don't match any SHA-1 for \"" << filename << "\"";
       return 1;
     }
@@ -551,7 +553,7 @@ int applypatch_flash(const char* source_filename, const char* target_filename,
 static int GenerateTarget(const FileContents& source_file, const std::unique_ptr<Value>& patch,
                           const std::string& target_filename,
                           const uint8_t target_sha1[SHA_DIGEST_LENGTH], const Value* bonus_data) {
-  if (patch->type != VAL_BLOB) {
+  if (patch->type != Value::Type::BLOB) {
     LOG(ERROR) << "patch is not a blob";
     return 1;
   }
@@ -622,10 +624,13 @@ static int GenerateTarget(const FileContents& source_file, const std::unique_ptr
     SHA1(reinterpret_cast<const uint8_t*>(patch->data.data()), patch->data.size(), patch_digest);
     LOG(ERROR) << "patch size " << patch->data.size() << " SHA-1 " << short_sha1(patch_digest);
 
-    uint8_t bonus_digest[SHA_DIGEST_LENGTH];
-    SHA1(reinterpret_cast<const uint8_t*>(bonus_data->data.data()), bonus_data->data.size(),
-         bonus_digest);
-    LOG(ERROR) << "bonus size " << bonus_data->data.size() << " SHA-1 " << short_sha1(bonus_digest);
+    if (bonus_data != nullptr) {
+      uint8_t bonus_digest[SHA_DIGEST_LENGTH];
+      SHA1(reinterpret_cast<const uint8_t*>(bonus_data->data.data()), bonus_data->data.size(),
+           bonus_digest);
+      LOG(ERROR) << "bonus size " << bonus_data->data.size() << " SHA-1 "
+                 << short_sha1(bonus_digest);
+    }
 
     // TODO(b/67849209) Remove after debugging the unit test flakiness.
     if (android::base::GetMinimumLogSeverity() <= android::base::LogSeverity::DEBUG) {
diff --git a/applypatch/applypatch_modes.cpp b/applypatch/applypatch_modes.cpp
index 6437e1be6..ec95325fc 100644
--- a/applypatch/applypatch_modes.cpp
+++ b/applypatch/applypatch_modes.cpp
@@ -81,52 +81,51 @@ static int FlashMode(const char* src_filename, const char* tgt_filename,
 }
 
 static int PatchMode(int argc, const char** argv) {
-    FileContents bonusFc;
-    Value bonus(VAL_INVALID, "");
-
-    if (argc >= 3 && strcmp(argv[1], "-b") == 0) {
-        if (LoadFileContents(argv[2], &bonusFc) != 0) {
-          LOG(ERROR) << "Failed to load bonus file " << argv[2];
-          return 1;
-        }
-        bonus.type = VAL_BLOB;
-        bonus.data = std::string(bonusFc.data.cbegin(), bonusFc.data.cend());
-        argc -= 2;
-        argv += 2;
-    }
-
-    if (argc < 4) {
-        return 2;
-    }
-
-    size_t target_size;
-    if (!android::base::ParseUint(argv[4], &target_size) || target_size == 0) {
-      LOG(ERROR) << "Failed to parse \"" << argv[4] << "\" as byte count";
+  std::unique_ptr<Value> bonus;
+  if (argc >= 3 && strcmp(argv[1], "-b") == 0) {
+    FileContents bonus_fc;
+    if (LoadFileContents(argv[2], &bonus_fc) != 0) {
+      LOG(ERROR) << "Failed to load bonus file " << argv[2];
       return 1;
     }
+    bonus = std::make_unique<Value>(Value::Type::BLOB,
+                                    std::string(bonus_fc.data.cbegin(), bonus_fc.data.cend()));
+    argc -= 2;
+    argv += 2;
+  }
 
-    // If no <src-sha1>:<patch> is provided, it is in flash mode.
-    if (argc == 5) {
-        if (bonus.type != VAL_INVALID) {
-          LOG(ERROR) << "bonus file not supported in flash mode";
-          return 1;
-        }
-        return FlashMode(argv[1], argv[2], argv[3], target_size);
-    }
+  if (argc < 4) {
+    return 2;
+  }
 
-    std::vector<std::string> sha1s;
-    std::vector<FileContents> files;
-    if (!ParsePatchArgs(argc-5, argv+5, &sha1s, &files)) {
-      LOG(ERROR) << "Failed to parse patch args";
+  size_t target_size;
+  if (!android::base::ParseUint(argv[4], &target_size) || target_size == 0) {
+    LOG(ERROR) << "Failed to parse \"" << argv[4] << "\" as byte count";
+    return 1;
+  }
+
+  // If no <src-sha1>:<patch> is provided, it is in flash mode.
+  if (argc == 5) {
+    if (bonus) {
+      LOG(ERROR) << "bonus file not supported in flash mode";
       return 1;
     }
+    return FlashMode(argv[1], argv[2], argv[3], target_size);
+  }
 
-    std::vector<std::unique_ptr<Value>> patches;
-    for (size_t i = 0; i < files.size(); ++i) {
-        patches.push_back(std::make_unique<Value>(
-                VAL_BLOB, std::string(files[i].data.cbegin(), files[i].data.cend())));
-    }
-    return applypatch(argv[1], argv[2], argv[3], target_size, sha1s, patches, &bonus);
+  std::vector<std::string> sha1s;
+  std::vector<FileContents> files;
+  if (!ParsePatchArgs(argc - 5, argv + 5, &sha1s, &files)) {
+    LOG(ERROR) << "Failed to parse patch args";
+    return 1;
+  }
+
+  std::vector<std::unique_ptr<Value>> patches;
+  for (const auto& file : files) {
+    patches.push_back(std::make_unique<Value>(Value::Type::BLOB,
+                                              std::string(file.data.cbegin(), file.data.cend())));
+  }
+  return applypatch(argv[1], argv[2], argv[3], target_size, sha1s, patches, bonus.get());
 }
 
 // This program (applypatch) applies binary patches to files in a way that
diff --git a/applypatch/imgpatch.cpp b/applypatch/imgpatch.cpp
index 2f8f4851d..da7569219 100644
--- a/applypatch/imgpatch.cpp
+++ b/applypatch/imgpatch.cpp
@@ -151,7 +151,8 @@ static bool ApplyBSDiffPatchAndStreamOutput(const uint8_t* src_data, size_t src_
 
 int ApplyImagePatch(const unsigned char* old_data, size_t old_size, const unsigned char* patch_data,
                     size_t patch_size, SinkFn sink) {
-  Value patch(VAL_BLOB, std::string(reinterpret_cast<const char*>(patch_data), patch_size));
+  Value patch(Value::Type::BLOB,
+              std::string(reinterpret_cast<const char*>(patch_data), patch_size));
   return ApplyImagePatch(old_data, old_size, patch, sink, nullptr);
 }
 
@@ -246,11 +247,10 @@ int ApplyImagePatch(const unsigned char* old_data, size_t old_size, const Value&
       // Decompress the source data; the chunk header tells us exactly
       // how big we expect it to be when decompressed.
 
-      // Note: expanded_len will include the bonus data size if
-      // the patch was constructed with bonus data.  The
-      // deflation will come up 'bonus_size' bytes short; these
-      // must be appended from the bonus_data value.
-      size_t bonus_size = (i == 1 && bonus_data != NULL) ? bonus_data->data.size() : 0;
+      // Note: expanded_len will include the bonus data size if the patch was constructed with
+      // bonus data. The deflation will come up 'bonus_size' bytes short; these must be appended
+      // from the bonus_data value.
+      size_t bonus_size = (i == 1 && bonus_data != nullptr) ? bonus_data->data.size() : 0;
 
       std::vector<unsigned char> expanded_source(expanded_len);
 
@@ -288,7 +288,7 @@ int ApplyImagePatch(const unsigned char* old_data, size_t old_size, const Value&
         inflateEnd(&strm);
 
         if (bonus_size) {
-          memcpy(expanded_source.data() + (expanded_len - bonus_size), &bonus_data->data[0],
+          memcpy(expanded_source.data() + (expanded_len - bonus_size), bonus_data->data.data(),
                  bonus_size);
         }
       }
diff --git a/applypatch/include/applypatch/applypatch.h b/applypatch/include/applypatch/applypatch.h
index f074e3681..92db59c3a 100644
--- a/applypatch/include/applypatch/applypatch.h
+++ b/applypatch/include/applypatch/applypatch.h
@@ -78,9 +78,10 @@ int applypatch(const char* source_filename, const char* target_filename,
                const std::vector<std::string>& patch_sha1s,
                const std::vector<std::unique_ptr<Value>>& patch_data, const Value* bonus_data);
 
-// Returns 0 if the contents of the file or the cached file match any of the given SHA-1's. Returns
-// nonzero otherwise.
-int applypatch_check(const char* filename, const std::vector<std::string>& patch_sha1s);
+// Returns 0 if the contents of the eMMC target or the cached file match any of the given SHA-1's.
+// Returns nonzero otherwise. 'filename' must refer to an eMMC partition target. It would only use
+// 'sha1s' to find a match on /cache if the hashes embedded in the filename fail to match.
+int applypatch_check(const std::string& filename, const std::vector<std::string>& sha1s);
 
 // Flashes a given image to the target partition. It verifies the target cheksum first, and will
 // return if target already has the desired hash. Otherwise it checks the checksum of the given
diff --git a/edify/expr.cpp b/edify/expr.cpp
index 6823b7339..c090eb28a 100644
--- a/edify/expr.cpp
+++ b/edify/expr.cpp
@@ -51,9 +51,9 @@ bool Evaluate(State* state, const std::unique_ptr<Expr>& expr, std::string* resu
     if (!v) {
         return false;
     }
-    if (v->type != VAL_STRING) {
-        ErrorAbort(state, kArgsParsingFailure, "expecting string, got value type %d", v->type);
-        return false;
+    if (v->type != Value::Type::STRING) {
+      ErrorAbort(state, kArgsParsingFailure, "expecting string, got value type %d", v->type);
+      return false;
     }
 
     *result = v->data;
@@ -68,7 +68,7 @@ Value* StringValue(const char* str) {
     if (str == nullptr) {
         return nullptr;
     }
-    return new Value(VAL_STRING, str);
+    return new Value(Value::Type::STRING, str);
 }
 
 Value* StringValue(const std::string& str) {
diff --git a/edify/include/edify/expr.h b/edify/include/edify/expr.h
index 770d1cf0d..5cbd5e15d 100644
--- a/edify/include/edify/expr.h
+++ b/edify/include/edify/expr.h
@@ -53,19 +53,16 @@ struct State {
   bool is_retry = false;
 };
 
-enum ValueType {
-    VAL_INVALID = -1,
-    VAL_STRING = 1,
-    VAL_BLOB = 2,
-};
-
 struct Value {
-    ValueType type;
-    std::string data;
+  enum class Type {
+    STRING = 1,
+    BLOB = 2,
+  };
+
+  Value(Type type, const std::string& str) : type(type), data(str) {}
 
-    Value(ValueType type, const std::string& str) :
-        type(type),
-        data(str) {}
+  Type type;
+  std::string data;
 };
 
 struct Expr;
@@ -156,6 +153,6 @@ Value* StringValue(const char* str);
 
 Value* StringValue(const std::string& str);
 
-int parse_string(const char* str, std::unique_ptr<Expr>* root, int* error_count);
+int ParseString(const std::string& str, std::unique_ptr<Expr>* root, int* error_count);
 
 #endif  // _EXPRESSION_H
diff --git a/edify/parser.yy b/edify/parser.yy
index bd2e0105f..3a63c37f8 100644
--- a/edify/parser.yy
+++ b/edify/parser.yy
@@ -138,7 +138,7 @@ void yyerror(std::unique_ptr<Expr>* root, int* error_count, const char* s) {
   ++*error_count;
 }
 
-int parse_string(const char* str, std::unique_ptr<Expr>* root, int* error_count) {
-    yy_switch_to_buffer(yy_scan_string(str));
-    return yyparse(root, error_count);
+int ParseString(const std::string& str, std::unique_ptr<Expr>* root, int* error_count) {
+  yy_switch_to_buffer(yy_scan_string(str.c_str()));
+  return yyparse(root, error_count);
 }
diff --git a/tests/component/edify_test.cpp b/tests/component/edify_test.cpp
index 61a1e6b64..8397bd38e 100644
--- a/tests/component/edify_test.cpp
+++ b/tests/component/edify_test.cpp
@@ -21,30 +21,29 @@
 
 #include "edify/expr.h"
 
-static void expect(const char* expr_str, const char* expected) {
-    std::unique_ptr<Expr> e;
-    int error_count = 0;
-    EXPECT_EQ(0, parse_string(expr_str, &e, &error_count));
-    EXPECT_EQ(0, error_count);
-
-    State state(expr_str, nullptr);
-
-    std::string result;
-    bool status = Evaluate(&state, e, &result);
-
-    if (expected == nullptr) {
-        EXPECT_FALSE(status);
-    } else {
-        EXPECT_STREQ(expected, result.c_str());
-    }
-
+static void expect(const std::string& expr_str, const char* expected) {
+  std::unique_ptr<Expr> e;
+  int error_count = 0;
+  EXPECT_EQ(0, ParseString(expr_str, &e, &error_count));
+  EXPECT_EQ(0, error_count);
+
+  State state(expr_str, nullptr);
+
+  std::string result;
+  bool status = Evaluate(&state, e, &result);
+
+  if (expected == nullptr) {
+    EXPECT_FALSE(status);
+  } else {
+    EXPECT_STREQ(expected, result.c_str());
+  }
 }
 
 class EdifyTest : public ::testing::Test {
-  protected:
-    virtual void SetUp() {
-        RegisterBuiltins();
-    }
+ protected:
+  void SetUp() {
+    RegisterBuiltins();
+  }
 };
 
 TEST_F(EdifyTest, parsing) {
@@ -146,25 +145,23 @@ TEST_F(EdifyTest, comparison) {
 }
 
 TEST_F(EdifyTest, big_string) {
-    // big string
-    expect(std::string(8192, 's').c_str(), std::string(8192, 's').c_str());
+  expect(std::string(8192, 's'), std::string(8192, 's').c_str());
 }
 
 TEST_F(EdifyTest, unknown_function) {
-    // unknown function
-    const char* script1 = "unknown_function()";
-    std::unique_ptr<Expr> expr;
-    int error_count = 0;
-    EXPECT_EQ(1, parse_string(script1, &expr, &error_count));
-    EXPECT_EQ(1, error_count);
-
-    const char* script2 = "abc; unknown_function()";
-    error_count = 0;
-    EXPECT_EQ(1, parse_string(script2, &expr, &error_count));
-    EXPECT_EQ(1, error_count);
-
-    const char* script3 = "unknown_function1() || yes";
-    error_count = 0;
-    EXPECT_EQ(1, parse_string(script3, &expr, &error_count));
-    EXPECT_EQ(1, error_count);
+  const char* script1 = "unknown_function()";
+  std::unique_ptr<Expr> expr;
+  int error_count = 0;
+  EXPECT_EQ(1, ParseString(script1, &expr, &error_count));
+  EXPECT_EQ(1, error_count);
+
+  const char* script2 = "abc; unknown_function()";
+  error_count = 0;
+  EXPECT_EQ(1, ParseString(script2, &expr, &error_count));
+  EXPECT_EQ(1, error_count);
+
+  const char* script3 = "unknown_function1() || yes";
+  error_count = 0;
+  EXPECT_EQ(1, ParseString(script3, &expr, &error_count));
+  EXPECT_EQ(1, error_count);
 }
diff --git a/tests/component/updater_test.cpp b/tests/component/updater_test.cpp
index 0b6b96f7c..9fcf17f13 100644
--- a/tests/component/updater_test.cpp
+++ b/tests/component/updater_test.cpp
@@ -51,17 +51,19 @@
 #include "updater/install.h"
 #include "updater/updater.h"
 
+using namespace std::string_literals;
+
 using PackageEntries = std::unordered_map<std::string, std::string>;
 
 static constexpr size_t kTransferListHeaderLines = 4;
 
 struct selabel_handle* sehandle = nullptr;
 
-static void expect(const char* expected, const char* expr_str, CauseCode cause_code,
+static void expect(const char* expected, const std::string& expr_str, CauseCode cause_code,
                    UpdaterInfo* info = nullptr) {
   std::unique_ptr<Expr> e;
   int error_count = 0;
-  ASSERT_EQ(0, parse_string(expr_str, &e, &error_count));
+  ASSERT_EQ(0, ParseString(expr_str, &e, &error_count));
   ASSERT_EQ(0, error_count);
 
   State state(expr_str, info);
@@ -126,7 +128,7 @@ static void RunBlockImageUpdate(bool is_verify, const PackageEntries& entries,
   std::string script = is_verify ? "block_image_verify" : "block_image_update";
   script += R"((")" + image_file + R"(", package_extract_file("transfer_list"), ")" + new_data +
             R"(", "patch_data"))";
-  expect(result.c_str(), script.c_str(), cause_code, &updater_info);
+  expect(result.c_str(), script, cause_code, &updater_info);
 
   ASSERT_EQ(0, fclose(updater_info.cmd_pipe));
   CloseArchive(handle);
@@ -149,11 +151,11 @@ static Value* BlobToString(const char* name, State* state,
     return nullptr;
   }
 
-  if (args[0]->type != VAL_BLOB) {
+  if (args[0]->type != Value::Type::BLOB) {
     return ErrorAbort(state, kArgsParsingFailure, "%s() expects a BLOB argument", name);
   }
 
-  args[0]->type = VAL_STRING;
+  args[0]->type = Value::Type::STRING;
   return args[0].release();
 }
 
@@ -230,7 +232,7 @@ TEST_F(UpdaterTest, apply_patch_check) {
   std::string filename = android::base::Join(
       std::vector<std::string>{ "EMMC", src_file, std::to_string(src_size), src_hash }, ":");
   std::string cmd = "apply_patch_check(\"" + filename + "\")";
-  expect("t", cmd.c_str(), kNoCause);
+  expect("t", cmd, kNoCause);
 
   // EMMC:file:(size-1):sha1:(size+1):sha1 should fail the check.
   std::string filename_bad = android::base::Join(
@@ -238,7 +240,7 @@ TEST_F(UpdaterTest, apply_patch_check) {
                                 std::to_string(src_size + 1), src_hash },
       ":");
   cmd = "apply_patch_check(\"" + filename_bad + "\")";
-  expect("", cmd.c_str(), kNoCause);
+  expect("", cmd, kNoCause);
 
   // EMMC:file:(size-1):sha1:size:sha1:(size+1):sha1 should pass the check.
   filename_bad =
@@ -247,19 +249,21 @@ TEST_F(UpdaterTest, apply_patch_check) {
                                                     std::to_string(src_size + 1), src_hash },
                           ":");
   cmd = "apply_patch_check(\"" + filename_bad + "\")";
-  expect("t", cmd.c_str(), kNoCause);
+  expect("t", cmd, kNoCause);
 
   // Multiple arguments.
+  // As long as it successfully loads the partition specified in filename, it won't check against
+  // any given SHAs.
   cmd = "apply_patch_check(\"" + filename + "\", \"wrong_sha1\", \"wrong_sha2\")";
-  expect("", cmd.c_str(), kNoCause);
+  expect("t", cmd, kNoCause);
 
   cmd = "apply_patch_check(\"" + filename + "\", \"wrong_sha1\", \"" + src_hash +
         "\", \"wrong_sha2\")";
-  expect("t", cmd.c_str(), kNoCause);
+  expect("t", cmd, kNoCause);
 
   cmd = "apply_patch_check(\"" + filename_bad + "\", \"wrong_sha1\", \"" + src_hash +
         "\", \"wrong_sha2\")";
-  expect("t", cmd.c_str(), kNoCause);
+  expect("t", cmd, kNoCause);
 }
 
 TEST_F(UpdaterTest, file_getprop) {
@@ -286,28 +290,28 @@ TEST_F(UpdaterTest, file_getprop) {
 
     std::string script1("file_getprop(\"" + std::string(temp_file2.path) +
                        "\", \"ro.product.name\")");
-    expect("tardis", script1.c_str(), kNoCause);
+    expect("tardis", script1, kNoCause);
 
     std::string script2("file_getprop(\"" + std::string(temp_file2.path) +
                        "\", \"ro.product.board\")");
-    expect("magic", script2.c_str(), kNoCause);
+    expect("magic", script2, kNoCause);
 
     // No match.
     std::string script3("file_getprop(\"" + std::string(temp_file2.path) +
                        "\", \"ro.product.wrong\")");
-    expect("", script3.c_str(), kNoCause);
+    expect("", script3, kNoCause);
 
     std::string script4("file_getprop(\"" + std::string(temp_file2.path) +
                        "\", \"ro.product.name=\")");
-    expect("", script4.c_str(), kNoCause);
+    expect("", script4, kNoCause);
 
     std::string script5("file_getprop(\"" + std::string(temp_file2.path) +
                        "\", \"ro.product.nam\")");
-    expect("", script5.c_str(), kNoCause);
+    expect("", script5, kNoCause);
 
     std::string script6("file_getprop(\"" + std::string(temp_file2.path) +
                        "\", \"ro.product.model\")");
-    expect("", script6.c_str(), kNoCause);
+    expect("", script6, kNoCause);
 }
 
 // TODO: Test extracting to block device.
@@ -327,7 +331,7 @@ TEST_F(UpdaterTest, package_extract_file) {
   // Two-argument version.
   TemporaryFile temp_file1;
   std::string script("package_extract_file(\"a.txt\", \"" + std::string(temp_file1.path) + "\")");
-  expect("t", script.c_str(), kNoCause, &updater_info);
+  expect("t", script, kNoCause, &updater_info);
 
   // Verify the extracted entry.
   std::string data;
@@ -336,34 +340,55 @@ TEST_F(UpdaterTest, package_extract_file) {
 
   // Now extract another entry to the same location, which should overwrite.
   script = "package_extract_file(\"b.txt\", \"" + std::string(temp_file1.path) + "\")";
-  expect("t", script.c_str(), kNoCause, &updater_info);
+  expect("t", script, kNoCause, &updater_info);
 
   ASSERT_TRUE(android::base::ReadFileToString(temp_file1.path, &data));
   ASSERT_EQ(kBTxtContents, data);
 
   // Missing zip entry. The two-argument version doesn't abort.
   script = "package_extract_file(\"doesntexist\", \"" + std::string(temp_file1.path) + "\")";
-  expect("", script.c_str(), kNoCause, &updater_info);
+  expect("", script, kNoCause, &updater_info);
 
   // Extract to /dev/full should fail.
   script = "package_extract_file(\"a.txt\", \"/dev/full\")";
-  expect("", script.c_str(), kNoCause, &updater_info);
+  expect("", script, kNoCause, &updater_info);
 
   // One-argument version. package_extract_file() gives a VAL_BLOB, which needs to be converted to
   // VAL_STRING for equality test.
   script = "blob_to_string(package_extract_file(\"a.txt\")) == \"" + kATxtContents + "\"";
-  expect("t", script.c_str(), kNoCause, &updater_info);
+  expect("t", script, kNoCause, &updater_info);
 
   script = "blob_to_string(package_extract_file(\"b.txt\")) == \"" + kBTxtContents + "\"";
-  expect("t", script.c_str(), kNoCause, &updater_info);
+  expect("t", script, kNoCause, &updater_info);
 
   // Missing entry. The one-argument version aborts the evaluation.
   script = "package_extract_file(\"doesntexist\")";
-  expect(nullptr, script.c_str(), kPackageExtractFileFailure, &updater_info);
+  expect(nullptr, script, kPackageExtractFileFailure, &updater_info);
 
   CloseArchive(handle);
 }
 
+TEST_F(UpdaterTest, read_file) {
+  // read_file() expects one argument.
+  expect(nullptr, "read_file()", kArgsParsingFailure);
+  expect(nullptr, "read_file(\"arg1\", \"arg2\")", kArgsParsingFailure);
+
+  // Write some value to file and read back.
+  TemporaryFile temp_file;
+  std::string script("write_value(\"foo\", \""s + temp_file.path + "\");");
+  expect("t", script, kNoCause);
+
+  script = "read_file(\""s + temp_file.path + "\") == \"foo\"";
+  expect("t", script, kNoCause);
+
+  script = "read_file(\""s + temp_file.path + "\") == \"bar\"";
+  expect("", script, kNoCause);
+
+  // It should fail gracefully when read fails.
+  script = "read_file(\"/doesntexist\")";
+  expect("", script, kNoCause);
+}
+
 TEST_F(UpdaterTest, write_value) {
   // write_value() expects two arguments.
   expect(nullptr, "write_value()", kArgsParsingFailure);
@@ -377,7 +402,7 @@ TEST_F(UpdaterTest, write_value) {
   TemporaryFile temp_file;
   std::string value = "magicvalue";
   std::string script("write_value(\"" + value + "\", \"" + std::string(temp_file.path) + "\")");
-  expect("t", script.c_str(), kNoCause);
+  expect("t", script, kNoCause);
 
   // Verify the content.
   std::string content;
@@ -386,7 +411,7 @@ TEST_F(UpdaterTest, write_value) {
 
   // Allow writing empty string.
   script = "write_value(\"\", \"" + std::string(temp_file.path) + "\")";
-  expect("t", script.c_str(), kNoCause);
+  expect("t", script, kNoCause);
 
   // Verify the content.
   ASSERT_TRUE(android::base::ReadFileToString(temp_file.path, &content));
@@ -394,7 +419,7 @@ TEST_F(UpdaterTest, write_value) {
 
   // It should fail gracefully when write fails.
   script = "write_value(\"value\", \"/proc/0/file1\")";
-  expect("", script.c_str(), kNoCause);
+  expect("", script, kNoCause);
 }
 
 TEST_F(UpdaterTest, get_stage) {
@@ -413,11 +438,11 @@ TEST_F(UpdaterTest, get_stage) {
 
   // Can read the stage value.
   std::string script("get_stage(\"" + temp_file + "\")");
-  expect("2/3", script.c_str(), kNoCause);
+  expect("2/3", script, kNoCause);
 
   // Bad BCB path.
   script = "get_stage(\"doesntexist\")";
-  expect("", script.c_str(), kNoCause);
+  expect("", script, kNoCause);
 }
 
 TEST_F(UpdaterTest, set_stage) {
@@ -437,7 +462,7 @@ TEST_F(UpdaterTest, set_stage) {
 
   // Write with set_stage().
   std::string script("set_stage(\"" + temp_file + "\", \"1/3\")");
-  expect(tf.path, script.c_str(), kNoCause);
+  expect(tf.path, script, kNoCause);
 
   // Verify.
   bootloader_message boot_verify;
@@ -449,10 +474,10 @@ TEST_F(UpdaterTest, set_stage) {
 
   // Bad BCB path.
   script = "set_stage(\"doesntexist\", \"1/3\")";
-  expect("", script.c_str(), kNoCause);
+  expect("", script, kNoCause);
 
   script = "set_stage(\"/dev/full\", \"1/3\")";
-  expect("", script.c_str(), kNoCause);
+  expect("", script, kNoCause);
 }
 
 TEST_F(UpdaterTest, set_progress) {
diff --git a/tests/unit/applypatch_test.cpp b/tests/unit/applypatch_test.cpp
index 4fbdd37fa..5cc03bc7b 100644
--- a/tests/unit/applypatch_test.cpp
+++ b/tests/unit/applypatch_test.cpp
@@ -33,7 +33,6 @@
 #include <android-base/test_utils.h>
 #include <android-base/unique_fd.h>
 #include <gtest/gtest.h>
-#include <openssl/sha.h>
 
 #include "applypatch/applypatch.h"
 #include "common/test_constants.h"
@@ -42,139 +41,102 @@
 
 using namespace std::string_literals;
 
-static void sha1sum(const std::string& fname, std::string* sha1, size_t* fsize = nullptr) {
-  ASSERT_TRUE(sha1 != nullptr);
-
-  std::string data;
-  ASSERT_TRUE(android::base::ReadFileToString(fname, &data));
-
-  if (fsize != nullptr) {
-    *fsize = data.size();
-  }
-
-  uint8_t digest[SHA_DIGEST_LENGTH];
-  SHA1(reinterpret_cast<const uint8_t*>(data.c_str()), data.size(), digest);
-  *sha1 = print_sha1(digest);
-}
-
-static void mangle_file(const std::string& fname) {
-  std::string content(1024, '\0');
-  for (size_t i = 0; i < 1024; i++) {
-    content[i] = rand() % 256;
-  }
-  ASSERT_TRUE(android::base::WriteStringToFile(content, fname));
-}
-
 class ApplyPatchTest : public ::testing::Test {
- public:
+ protected:
   void SetUp() override {
-    // set up files
     old_file = from_testdata_base("old.file");
+    FileContents old_fc;
+    ASSERT_EQ(0, LoadFileContents(old_file, &old_fc));
+    old_sha1 = print_sha1(old_fc.sha1);
+    old_size = old_fc.data.size();
+
     new_file = from_testdata_base("new.file");
-    nonexistent_file = from_testdata_base("nonexistent.file");
+    FileContents new_fc;
+    ASSERT_EQ(0, LoadFileContents(new_file, &new_fc));
+    new_sha1 = print_sha1(new_fc.sha1);
+    new_size = new_fc.data.size();
 
-    // set up SHA constants
-    sha1sum(old_file, &old_sha1, &old_size);
-    sha1sum(new_file, &new_sha1, &new_size);
     srand(time(nullptr));
     bad_sha1_a = android::base::StringPrintf("%040x", rand());
     bad_sha1_b = android::base::StringPrintf("%040x", rand());
+
+    // Reset the cache backup file.
+    Paths::Get().set_cache_temp_source("/cache/saved.file");
   }
 
   std::string old_file;
-  std::string new_file;
-  std::string nonexistent_file;
-
   std::string old_sha1;
+  size_t old_size;
+
+  std::string new_file;
   std::string new_sha1;
+  size_t new_size;
+
   std::string bad_sha1_a;
   std::string bad_sha1_b;
-
-  size_t old_size;
-  size_t new_size;
 };
 
-TEST_F(ApplyPatchTest, CheckModeSkip) {
-  std::vector<std::string> sha1s;
-  ASSERT_EQ(0, applypatch_check(&old_file[0], sha1s));
-}
-
-TEST_F(ApplyPatchTest, CheckModeSingle) {
-  std::vector<std::string> sha1s = { old_sha1 };
-  ASSERT_EQ(0, applypatch_check(&old_file[0], sha1s));
+TEST_F(ApplyPatchTest, CheckMode) {
+  std::string partition = "EMMC:" + old_file + ":" + std::to_string(old_size) + ":" + old_sha1;
+  ASSERT_EQ(0, applypatch_check(partition, {}));
+  ASSERT_EQ(0, applypatch_check(partition, { old_sha1 }));
+  ASSERT_EQ(0, applypatch_check(partition, { bad_sha1_a, bad_sha1_b }));
+  ASSERT_EQ(0, applypatch_check(partition, { bad_sha1_a, old_sha1, bad_sha1_b }));
 }
 
-TEST_F(ApplyPatchTest, CheckModeMultiple) {
-  std::vector<std::string> sha1s = { bad_sha1_a, old_sha1, bad_sha1_b };
-  ASSERT_EQ(0, applypatch_check(&old_file[0], sha1s));
+TEST_F(ApplyPatchTest, CheckMode_NonEmmcTarget) {
+  ASSERT_NE(0, applypatch_check(old_file, {}));
+  ASSERT_NE(0, applypatch_check(old_file, { old_sha1 }));
+  ASSERT_NE(0, applypatch_check(old_file, { bad_sha1_a, bad_sha1_b }));
+  ASSERT_NE(0, applypatch_check(old_file, { bad_sha1_a, old_sha1, bad_sha1_b }));
 }
 
-TEST_F(ApplyPatchTest, CheckModeFailure) {
-  std::vector<std::string> sha1s = { bad_sha1_a, bad_sha1_b };
-  ASSERT_NE(0, applypatch_check(&old_file[0], sha1s));
-}
-
-TEST_F(ApplyPatchTest, CheckModeEmmcTarget) {
+TEST_F(ApplyPatchTest, CheckMode_EmmcTarget) {
   // EMMC:old_file:size:sha1 should pass the check.
   std::string src_file = "EMMC:" + old_file + ":" + std::to_string(old_size) + ":" + old_sha1;
-  std::vector<std::string> sha1s;
-  ASSERT_EQ(0, applypatch_check(src_file.c_str(), sha1s));
+  ASSERT_EQ(0, applypatch_check(src_file, {}));
 
   // EMMC:old_file:(size-1):sha1:(size+1):sha1 should fail the check.
   src_file = "EMMC:" + old_file + ":" + std::to_string(old_size - 1) + ":" + old_sha1 + ":" +
              std::to_string(old_size + 1) + ":" + old_sha1;
-  ASSERT_EQ(1, applypatch_check(src_file.c_str(), sha1s));
+  ASSERT_NE(0, applypatch_check(src_file, {}));
 
   // EMMC:old_file:(size-1):sha1:size:sha1:(size+1):sha1 should pass the check.
   src_file = "EMMC:" + old_file + ":" + std::to_string(old_size - 1) + ":" + old_sha1 + ":" +
              std::to_string(old_size) + ":" + old_sha1 + ":" + std::to_string(old_size + 1) + ":" +
              old_sha1;
-  ASSERT_EQ(0, applypatch_check(src_file.c_str(), sha1s));
+  ASSERT_EQ(0, applypatch_check(src_file, {}));
 
   // EMMC:old_file:(size+1):sha1:(size-1):sha1:size:sha1 should pass the check.
   src_file = "EMMC:" + old_file + ":" + std::to_string(old_size + 1) + ":" + old_sha1 + ":" +
              std::to_string(old_size - 1) + ":" + old_sha1 + ":" + std::to_string(old_size) + ":" +
              old_sha1;
-  ASSERT_EQ(0, applypatch_check(src_file.c_str(), sha1s));
+  ASSERT_EQ(0, applypatch_check(src_file, {}));
 
   // EMMC:new_file:(size+1):old_sha1:(size-1):old_sha1:size:old_sha1:size:new_sha1
   // should pass the check.
   src_file = "EMMC:" + new_file + ":" + std::to_string(old_size + 1) + ":" + old_sha1 + ":" +
              std::to_string(old_size - 1) + ":" + old_sha1 + ":" + std::to_string(old_size) + ":" +
              old_sha1 + ":" + std::to_string(new_size) + ":" + new_sha1;
-  ASSERT_EQ(0, applypatch_check(src_file.c_str(), sha1s));
+  ASSERT_EQ(0, applypatch_check(src_file, {}));
 }
 
-class ApplyPatchCacheTest : public ApplyPatchTest {
- protected:
-  void SetUp() override {
-    ApplyPatchTest::SetUp();
-    Paths::Get().set_cache_temp_source(old_file);
-  }
-};
+TEST_F(ApplyPatchTest, CheckMode_UseBackup) {
+  std::string corrupted = "EMMC:" + old_file + ":" + std::to_string(old_size) + ":" + bad_sha1_a;
+  ASSERT_NE(0, applypatch_check(corrupted, { old_sha1 }));
 
-TEST_F(ApplyPatchCacheTest, CheckCacheCorruptedSourceSingle) {
-  TemporaryFile temp_file;
-  mangle_file(temp_file.path);
-  std::vector<std::string> sha1s_single = { old_sha1 };
-  ASSERT_EQ(0, applypatch_check(temp_file.path, sha1s_single));
-  ASSERT_EQ(0, applypatch_check(nonexistent_file.c_str(), sha1s_single));
+  Paths::Get().set_cache_temp_source(old_file);
+  ASSERT_EQ(0, applypatch_check(corrupted, { old_sha1 }));
+  ASSERT_EQ(0, applypatch_check(corrupted, { bad_sha1_a, old_sha1, bad_sha1_b }));
 }
 
-TEST_F(ApplyPatchCacheTest, CheckCacheCorruptedSourceMultiple) {
-  TemporaryFile temp_file;
-  mangle_file(temp_file.path);
-  std::vector<std::string> sha1s_multiple = { bad_sha1_a, old_sha1, bad_sha1_b };
-  ASSERT_EQ(0, applypatch_check(temp_file.path, sha1s_multiple));
-  ASSERT_EQ(0, applypatch_check(nonexistent_file.c_str(), sha1s_multiple));
-}
+TEST_F(ApplyPatchTest, CheckMode_UseBackup_BothCorrupted) {
+  std::string corrupted = "EMMC:" + old_file + ":" + std::to_string(old_size) + ":" + bad_sha1_a;
+  ASSERT_NE(0, applypatch_check(corrupted, {}));
+  ASSERT_NE(0, applypatch_check(corrupted, { old_sha1 }));
 
-TEST_F(ApplyPatchCacheTest, CheckCacheCorruptedSourceFailure) {
-  TemporaryFile temp_file;
-  mangle_file(temp_file.path);
-  std::vector<std::string> sha1s_failure = { bad_sha1_a, bad_sha1_b };
-  ASSERT_NE(0, applypatch_check(temp_file.path, sha1s_failure));
-  ASSERT_NE(0, applypatch_check(nonexistent_file.c_str(), sha1s_failure));
+  Paths::Get().set_cache_temp_source(old_file);
+  ASSERT_NE(0, applypatch_check(corrupted, { bad_sha1_a, bad_sha1_b }));
 }
 
 class FreeCacheTest : public ::testing::Test {
diff --git a/updater/blockimg.cpp b/updater/blockimg.cpp
index fc713859b..6a6236b1b 100644
--- a/updater/blockimg.cpp
+++ b/updater/blockimg.cpp
@@ -1403,7 +1403,8 @@ static int PerformCommandDiff(CommandParameters& params) {
     if (status == 0) {
       LOG(INFO) << "patching " << blocks << " blocks to " << tgt.blocks();
       Value patch_value(
-          VAL_BLOB, std::string(reinterpret_cast<const char*>(params.patch_start + offset), len));
+          Value::Type::BLOB,
+          std::string(reinterpret_cast<const char*>(params.patch_start + offset), len));
 
       RangeSinkWriter writer(params.fd, tgt);
       if (params.cmdname[0] == 'i') {  // imgdiff
@@ -1531,19 +1532,19 @@ static Value* PerformBlockImageUpdate(const char* name, State* state,
   const std::unique_ptr<Value>& new_data_fn = args[2];
   const std::unique_ptr<Value>& patch_data_fn = args[3];
 
-  if (blockdev_filename->type != VAL_STRING) {
+  if (blockdev_filename->type != Value::Type::STRING) {
     ErrorAbort(state, kArgsParsingFailure, "blockdev_filename argument to %s must be string", name);
     return StringValue("");
   }
-  if (transfer_list_value->type != VAL_BLOB) {
+  if (transfer_list_value->type != Value::Type::BLOB) {
     ErrorAbort(state, kArgsParsingFailure, "transfer_list argument to %s must be blob", name);
     return StringValue("");
   }
-  if (new_data_fn->type != VAL_STRING) {
+  if (new_data_fn->type != Value::Type::STRING) {
     ErrorAbort(state, kArgsParsingFailure, "new_data_fn argument to %s must be string", name);
     return StringValue("");
   }
-  if (patch_data_fn->type != VAL_STRING) {
+  if (patch_data_fn->type != Value::Type::STRING) {
     ErrorAbort(state, kArgsParsingFailure, "patch_data_fn argument to %s must be string", name);
     return StringValue("");
   }
@@ -1944,11 +1945,11 @@ Value* RangeSha1Fn(const char* name, State* state, const std::vector<std::unique
   const std::unique_ptr<Value>& blockdev_filename = args[0];
   const std::unique_ptr<Value>& ranges = args[1];
 
-  if (blockdev_filename->type != VAL_STRING) {
+  if (blockdev_filename->type != Value::Type::STRING) {
     ErrorAbort(state, kArgsParsingFailure, "blockdev_filename argument to %s must be string", name);
     return StringValue("");
   }
-  if (ranges->type != VAL_STRING) {
+  if (ranges->type != Value::Type::STRING) {
     ErrorAbort(state, kArgsParsingFailure, "ranges argument to %s must be string", name);
     return StringValue("");
   }
@@ -2010,7 +2011,7 @@ Value* CheckFirstBlockFn(const char* name, State* state,
 
   const std::unique_ptr<Value>& arg_filename = args[0];
 
-  if (arg_filename->type != VAL_STRING) {
+  if (arg_filename->type != Value::Type::STRING) {
     ErrorAbort(state, kArgsParsingFailure, "filename argument to %s must be string", name);
     return StringValue("");
   }
@@ -2065,11 +2066,11 @@ Value* BlockImageRecoverFn(const char* name, State* state,
   const std::unique_ptr<Value>& filename = args[0];
   const std::unique_ptr<Value>& ranges = args[1];
 
-  if (filename->type != VAL_STRING) {
+  if (filename->type != Value::Type::STRING) {
     ErrorAbort(state, kArgsParsingFailure, "filename argument to %s must be string", name);
     return StringValue("");
   }
-  if (ranges->type != VAL_STRING) {
+  if (ranges->type != Value::Type::STRING) {
     ErrorAbort(state, kArgsParsingFailure, "ranges argument to %s must be string", name);
     return StringValue("");
   }
diff --git a/updater/install.cpp b/updater/install.cpp
index 5ebdab49d..ba7bd55b0 100644
--- a/updater/install.cpp
+++ b/updater/install.cpp
@@ -191,7 +191,7 @@ Value* PackageExtractFileFn(const char* name, State* state,
                         zip_path.c_str(), buffer.size(), ErrorCodeString(ret));
     }
 
-    return new Value(VAL_BLOB, buffer);
+    return new Value(Value::Type::BLOB, buffer);
   }
 }
 
@@ -238,10 +238,10 @@ Value* ApplyPatchFn(const char* name, State* state,
   }
 
   for (int i = 0; i < patchcount; ++i) {
-    if (arg_values[i * 2]->type != VAL_STRING) {
+    if (arg_values[i * 2]->type != Value::Type::STRING) {
       return ErrorAbort(state, kArgsParsingFailure, "%s(): sha-1 #%d is not string", name, i * 2);
     }
-    if (arg_values[i * 2 + 1]->type != VAL_BLOB) {
+    if (arg_values[i * 2 + 1]->type != Value::Type::BLOB) {
       return ErrorAbort(state, kArgsParsingFailure, "%s(): patch #%d is not blob", name, i * 2 + 1);
     }
   }
@@ -739,8 +739,8 @@ Value* RunProgramFn(const char* name, State* state, const std::vector<std::uniqu
   return StringValue(std::to_string(status));
 }
 
-// Read a local file and return its contents (the Value* returned
-// is actually a FileContents*).
+// read_file(filename)
+//   Reads a local file 'filename' and returns its contents as a string Value.
 Value* ReadFileFn(const char* name, State* state, const std::vector<std::unique_ptr<Expr>>& argv) {
   if (argv.size() != 1) {
     return ErrorAbort(state, kArgsParsingFailure, "%s() expects 1 arg, got %zu", name, argv.size());
@@ -748,18 +748,18 @@ Value* ReadFileFn(const char* name, State* state, const std::vector<std::unique_
 
   std::vector<std::string> args;
   if (!ReadArgs(state, argv, &args)) {
-    return ErrorAbort(state, kArgsParsingFailure, "%s() Failed to parse the argument(s)", name);
+    return ErrorAbort(state, kArgsParsingFailure, "%s(): Failed to parse the argument(s)", name);
   }
   const std::string& filename = args[0];
 
-  Value* v = new Value(VAL_INVALID, "");
-
-  FileContents fc;
-  if (LoadFileContents(filename.c_str(), &fc) == 0) {
-    v->type = VAL_BLOB;
-    v->data = std::string(fc.data.begin(), fc.data.end());
+  std::string contents;
+  if (android::base::ReadFileToString(filename, &contents)) {
+    return new Value(Value::Type::STRING, std::move(contents));
   }
-  return v;
+
+  // Leave it to caller to handle the failure.
+  PLOG(ERROR) << name << ": Failed to read " << filename;
+  return StringValue("");
 }
 
 // write_value(value, filename)
diff --git a/updater/updater.cpp b/updater/updater.cpp
index 40e3f1fc1..e06d45355 100644
--- a/updater/updater.cpp
+++ b/updater/updater.cpp
@@ -134,7 +134,7 @@ int main(int argc, char** argv) {
 
   std::unique_ptr<Expr> root;
   int error_count = 0;
-  int error = parse_string(script.c_str(), &root, &error_count);
+  int error = ParseString(script, &root, &error_count);
   if (error != 0 || error_count > 0) {
     LOG(ERROR) << error_count << " parse errors";
     CloseArchive(za);