update_engine: Use the bsdiff library instead of its executable
Currently we use bsdiff binary to generate BSDIFF and SOURCE_BSDIFF
operations. This involves writing both the source and destination
extents into disk and letting bsdiff read them again into memory. Now
that we actually can use the bsdiff library itself, why not use the
provided bsdiff library functions that can directly read source and
destination data from the memory that we already have. This CL will
reduce the patch generating time and memory footprint.
BUG=none
TEST=cros_workon_make --board=amd64-generic --test update_engine; \
brillo_update_payload generate --payload=payload.delta \
--target_image=veyron-minnie-R62-9804.0.0.bin --source_image=veyron_minnie-R61-9765.13.0.bin
Change-Id: Ib115eb5186c133d56dab91f73a2bde3176f5732d
Reviewed-on: https://chromium-review.googlesource.com/602887
Commit-Ready: Amin Hassani <ahassani@chromium.org>
Tested-by: Amin Hassani <ahassani@chromium.org>
Reviewed-by: Ben Chan <benchan@chromium.org>
diff --git a/Android.mk b/Android.mk
index bf9b678..07d5998 100644
--- a/Android.mk
+++ b/Android.mk
@@ -607,9 +607,10 @@
# server-side code. This is used for delta_generator and unittests but not
# for any client code.
ue_libpayload_generator_exported_static_libraries := \
+ libbsdiff \
libpayload_consumer \
- update_metadata-protos \
liblzma \
+ update_metadata-protos \
$(ue_libpayload_consumer_exported_static_libraries) \
$(ue_update_metadata_protos_exported_static_libraries)
ue_libpayload_generator_exported_shared_libraries := \
@@ -654,9 +655,10 @@
LOCAL_LDFLAGS := $(ue_common_ldflags)
LOCAL_C_INCLUDES := $(ue_common_c_includes)
LOCAL_STATIC_LIBRARIES := \
+ libbsdiff \
libpayload_consumer \
- update_metadata-protos \
liblzma \
+ update_metadata-protos \
$(ue_common_static_libraries) \
$(ue_libpayload_consumer_exported_static_libraries) \
$(ue_update_metadata_protos_exported_static_libraries)
@@ -704,8 +706,6 @@
# Build for the host.
include $(CLEAR_VARS)
LOCAL_MODULE := delta_generator
-LOCAL_REQUIRED_MODULES := \
- bsdiff
LOCAL_MODULE_CLASS := EXECUTABLES
LOCAL_CPP_EXTENSION := .cc
LOCAL_CLANG := true
@@ -848,28 +848,6 @@
test_http_server.cc
include $(BUILD_EXECUTABLE)
-# bsdiff (type: executable)
-# ========================================================
-# We need bsdiff in the update_engine_unittests directory, so we build it here.
-include $(CLEAR_VARS)
-LOCAL_MODULE := ue_unittest_bsdiff
-LOCAL_MODULE_PATH := $(TARGET_OUT_DATA_NATIVE_TESTS)/update_engine_unittests
-LOCAL_MODULE_STEM := bsdiff
-LOCAL_CPP_EXTENSION := .cc
-LOCAL_SRC_FILES := ../../external/bsdiff/bsdiff_main.cc
-LOCAL_CFLAGS := \
- -D_FILE_OFFSET_BITS=64 \
- -Wall \
- -Werror \
- -Wextra \
- -Wno-unused-parameter
-LOCAL_STATIC_LIBRARIES := \
- libbsdiff \
- libbz \
- libdivsufsort64 \
- libdivsufsort
-include $(BUILD_EXECUTABLE)
-
# test_subprocess (type: executable)
# ========================================================
# Test helper subprocess program.
@@ -896,7 +874,6 @@
LOCAL_REQUIRED_MODULES := \
test_http_server \
test_subprocess \
- ue_unittest_bsdiff \
ue_unittest_delta_generator \
ue_unittest_disk_ext2_1k.img \
ue_unittest_disk_ext2_4k.img \
diff --git a/payload_generator/delta_diff_utils.cc b/payload_generator/delta_diff_utils.cc
index 856fecf..2cef589 100644
--- a/payload_generator/delta_diff_utils.cc
+++ b/payload_generator/delta_diff_utils.cc
@@ -34,6 +34,7 @@
#include <base/files/file_util.h>
#include <base/format_macros.h>
#include <base/strings/stringprintf.h>
+#include <bsdiff/bsdiff.h>
#include "update_engine/common/hash_calculator.h"
#include "update_engine/common/subprocess.h"
@@ -52,8 +53,6 @@
namespace chromeos_update_engine {
namespace {
-const char* const kBsdiffPath = "bsdiff";
-
// The maximum destination size allowed for bsdiff. In general, bsdiff should
// work for arbitrary big files, but the payload generation and payload
// application requires a significant amount of RAM. We put a hard-limit of
@@ -613,24 +612,21 @@
? InstallOperation::SOURCE_COPY
: InstallOperation::MOVE);
data_blob = brillo::Blob();
- } else if (bsdiff_allowed || puffdiff_allowed) {
- // If the source file is considered bsdiff safe (no bsdiff bugs
- // triggered), see if BSDIFF encoding is smaller.
- base::FilePath old_chunk;
- TEST_AND_RETURN_FALSE(base::CreateTemporaryFile(&old_chunk));
- ScopedPathUnlinker old_unlinker(old_chunk.value());
- TEST_AND_RETURN_FALSE(utils::WriteFile(
- old_chunk.value().c_str(), old_data.data(), old_data.size()));
- base::FilePath new_chunk;
- TEST_AND_RETURN_FALSE(base::CreateTemporaryFile(&new_chunk));
- ScopedPathUnlinker new_unlinker(new_chunk.value());
- TEST_AND_RETURN_FALSE(utils::WriteFile(
- new_chunk.value().c_str(), new_data.data(), new_data.size()));
-
+ } else {
if (bsdiff_allowed) {
+ base::FilePath patch;
+ TEST_AND_RETURN_FALSE(base::CreateTemporaryFile(&patch));
+ ScopedPathUnlinker unlinker(patch.value());
+
brillo::Blob bsdiff_delta;
- TEST_AND_RETURN_FALSE(DiffFiles(
- kBsdiffPath, old_chunk.value(), new_chunk.value(), &bsdiff_delta));
+ TEST_AND_RETURN_FALSE(0 == bsdiff::bsdiff(old_data.data(),
+ old_data.size(),
+ new_data.data(),
+ new_data.size(),
+ patch.value().c_str(),
+ nullptr));
+
+ TEST_AND_RETURN_FALSE(utils::ReadFile(patch.value(), &bsdiff_delta));
CHECK_GT(bsdiff_delta.size(), static_cast<brillo::Blob::size_type>(0));
if (bsdiff_delta.size() < data_blob.size()) {
operation.set_type(
@@ -672,36 +668,6 @@
return true;
}
-// Runs the bsdiff tool in |diff_path| on two files and returns the
-// resulting delta in |out|. Returns true on success.
-bool DiffFiles(const string& diff_path,
- const string& old_file,
- const string& new_file,
- brillo::Blob* out) {
- const string kPatchFile = "delta.patchXXXXXX";
- string patch_file_path;
-
- TEST_AND_RETURN_FALSE(
- utils::MakeTempFile(kPatchFile, &patch_file_path, nullptr));
-
- vector<string> cmd;
- cmd.push_back(diff_path);
- cmd.push_back(old_file);
- cmd.push_back(new_file);
- cmd.push_back(patch_file_path);
-
- int rc = 1;
- string stdout;
- TEST_AND_RETURN_FALSE(Subprocess::SynchronousExec(cmd, &rc, &stdout));
- if (rc != 0) {
- LOG(ERROR) << diff_path << " returned " << rc << std::endl << stdout;
- return false;
- }
- TEST_AND_RETURN_FALSE(utils::ReadFile(patch_file_path, out));
- unlink(patch_file_path.c_str());
- return true;
-}
-
bool IsAReplaceOperation(InstallOperation_Type op_type) {
return (op_type == InstallOperation::REPLACE ||
op_type == InstallOperation::REPLACE_BZ ||
diff --git a/payload_generator/delta_diff_utils.h b/payload_generator/delta_diff_utils.h
index 9c7481b..f5d5d41 100644
--- a/payload_generator/delta_diff_utils.h
+++ b/payload_generator/delta_diff_utils.h
@@ -103,13 +103,6 @@
brillo::Blob* out_data,
InstallOperation* out_op);
-// Runs the bsdiff tool in |diff_path| on two files and returns the
-// resulting delta in |out|. Returns true on success.
-bool DiffFiles(const std::string& diff_path,
- const std::string& old_file,
- const std::string& new_file,
- brillo::Blob* out);
-
// Generates the best allowed full operation to produce |new_data|. The allowed
// operations are based on |payload_version|. The operation blob will be stored
// in |out_blob| and the resulting operation type in |out_type|. Returns whether
diff --git a/update_engine.gyp b/update_engine.gyp
index 152bcaa..f3b199e 100644
--- a/update_engine.gyp
+++ b/update_engine.gyp
@@ -384,6 +384,9 @@
'<@(exported_deps)',
],
},
+ 'libraries': [
+ '-lbsdiff',
+ ],
},
'sources': [
'payload_generator/ab_generator.cc',