From c88bc006caf816bd05201a5a7158e38a8f728534 Mon Sep 17 00:00:00 2001 From: HeYuchen <377710264@qq.com> Date: Thu, 4 Jun 2020 18:31:51 +0800 Subject: [PATCH] refactor: move function verify_file to file_system (#491) --- include/dsn/utility/filesystem.h | 5 ++++ src/core/core/filesystem.cpp | 30 +++++++++++++++++++ src/core/tests/file_system_test.cpp | 30 +++++++++++++++++++ .../block_service/block_service_manager.cpp | 26 ---------------- .../block_service/block_service_manager.h | 3 -- .../test/block_service_manager_test.cpp | 21 ------------- .../lib/bulk_load/replica_bulk_loader.cpp | 5 +++- 7 files changed, 69 insertions(+), 51 deletions(-) create mode 100644 src/core/tests/file_system_test.cpp diff --git a/include/dsn/utility/filesystem.h b/include/dsn/utility/filesystem.h index 85524e727a..66f735e80a 100644 --- a/include/dsn/utility/filesystem.h +++ b/include/dsn/utility/filesystem.h @@ -131,6 +131,11 @@ std::pair is_directory_empty(const std::string &dirname); error_code read_file(const std::string &fname, /*out*/ std::string &buf); +// compare file metadata calculated by fname with expected md5 and file_size +bool verify_file(const std::string &fname, + const std::string &expected_md5, + const int64_t &expected_fsize); + } // namespace filesystem } // namespace utils } // namespace dsn diff --git a/src/core/core/filesystem.cpp b/src/core/core/filesystem.cpp index 5e658a9958..14c9c5c87a 100644 --- a/src/core/core/filesystem.cpp +++ b/src/core/core/filesystem.cpp @@ -796,6 +796,36 @@ error_code read_file(const std::string &fname, std::string &buf) return ERR_OK; } +bool verify_file(const std::string &fname, + const std::string &expected_md5, + const int64_t &expected_fsize) +{ + if (!file_exists(fname)) { + derror_f("file({}) is not existed", fname); + return false; + } + int64_t f_size = 0; + if (!file_size(fname, f_size)) { + derror_f("verify file({}) failed, becaused failed to get file size", fname); + return false; + } + std::string md5; + if (md5sum(fname, md5) != ERR_OK) { + derror_f("verify file({}) failed, becaused failed to get file md5", fname); + return false; + } + if (f_size != expected_fsize || md5 != expected_md5) { + derror_f("verify file({}) failed, because file damaged, size: {} VS {}, md5: {} VS {}", + fname, + f_size, + expected_fsize, + md5, + expected_md5); + return false; + } + return true; +} + } // namespace filesystem } // namespace utils } // namespace dsn diff --git a/src/core/tests/file_system_test.cpp b/src/core/tests/file_system_test.cpp new file mode 100644 index 0000000000..77c56466bc --- /dev/null +++ b/src/core/tests/file_system_test.cpp @@ -0,0 +1,30 @@ +// Copyright (c) 2017-present, Xiaomi, Inc. All rights reserved. +// This source code is licensed under the Apache License Version 2.0, which +// can be found in the LICENSE file in the root directory of this source tree. + +#include +#include + +namespace dsn { +namespace utils { +namespace filesystem { + +TEST(verify_file, verify_file_test) +{ + const std::string &fname = "test_file"; + std::string expected_md5; + int64_t expected_fsize; + create_file(fname); + md5sum(fname, expected_md5); + file_size(fname, expected_fsize); + + ASSERT_TRUE(verify_file(fname, expected_md5, expected_fsize)); + ASSERT_FALSE(verify_file(fname, "wrong_md5", 10086)); + ASSERT_FALSE(verify_file("file_not_exists", "wrong_md5", 10086)); + + remove_path(fname); +} + +} // namespace filesystem +} // namespace utils +} // namespace dsn diff --git a/src/dist/block_service/block_service_manager.cpp b/src/dist/block_service/block_service_manager.cpp index c0a2f41429..efa7066d87 100644 --- a/src/dist/block_service/block_service_manager.cpp +++ b/src/dist/block_service/block_service_manager.cpp @@ -214,32 +214,6 @@ error_code block_service_manager::download_file(const std::string &remote_dir, return download_err; } -bool block_service_manager::verify_file(const replication::file_meta &f_meta, - const std::string &local_dir) -{ - const std::string local_file = utils::filesystem::path_combine(local_dir, f_meta.name); - int64_t f_size = 0; - if (!utils::filesystem::file_size(local_file, f_size)) { - derror_f("verify file({}) failed, becaused failed to get file size", local_file); - return false; - } - std::string md5; - if (utils::filesystem::md5sum(local_file, md5) != ERR_OK) { - derror_f("verify file({}) failed, becaused failed to get file md5", local_file); - return false; - } - if (f_size != f_meta.size || md5 != f_meta.md5) { - derror_f("verify file({}) failed, because file damaged, size: {} VS {}, md5: {} VS {}", - local_file, - f_size, - f_meta.size, - md5, - f_meta.md5); - return false; - } - return true; -} - } // namespace block_service } // namespace dist } // namespace dsn diff --git a/src/dist/block_service/block_service_manager.h b/src/dist/block_service/block_service_manager.h index 4ebab38757..cb7d5e578f 100644 --- a/src/dist/block_service/block_service_manager.h +++ b/src/dist/block_service/block_service_manager.h @@ -39,9 +39,6 @@ class block_service_manager block_filesystem *fs, /*out*/ uint64_t &download_file_size); - // compare file metadata calculated by file and file_meta parsed from metadata file - bool verify_file(const replication::file_meta &f_meta, const std::string &local_dir); - private: block_service_registry &_registry_holder; diff --git a/src/dist/block_service/test/block_service_manager_test.cpp b/src/dist/block_service/test/block_service_manager_test.cpp index adb4f25d2c..7272aa6ab1 100644 --- a/src/dist/block_service/test/block_service_manager_test.cpp +++ b/src/dist/block_service/test/block_service_manager_test.cpp @@ -33,14 +33,6 @@ class block_service_manager_test : public ::testing::Test PROVIDER, LOCAL_DIR, FILE_NAME, _fs.get(), download_size); } - bool test_verify_file(int64_t size, const std::string &md5) - { - _file_meta.name = FILE_NAME; - _file_meta.size = size; - _file_meta.md5 = md5; - return _block_service_manager.verify_file(_file_meta, LOCAL_DIR); - } - void create_local_file(const std::string &file_name) { std::string whole_name = utils::filesystem::path_combine(LOCAL_DIR, file_name); @@ -103,19 +95,6 @@ TEST_F(block_service_manager_test, do_download_succeed) ASSERT_EQ(test_download_file(), ERR_OK); } -// verify_file unit tests -TEST_F(block_service_manager_test, verify_file_failed) -{ - create_local_file(FILE_NAME); - ASSERT_FALSE(test_verify_file(_file_meta.size, "wrong_md5")); -} - -TEST_F(block_service_manager_test, verify_file_succeed) -{ - create_local_file(FILE_NAME); - ASSERT_TRUE(test_verify_file(_file_meta.size, _file_meta.md5)); -} - } // namespace block_service } // namespace dist } // namespace dsn diff --git a/src/dist/replication/lib/bulk_load/replica_bulk_loader.cpp b/src/dist/replication/lib/bulk_load/replica_bulk_loader.cpp index 3a7cbba1ee..9164fd5a46 100644 --- a/src/dist/replication/lib/bulk_load/replica_bulk_loader.cpp +++ b/src/dist/replication/lib/bulk_load/replica_bulk_loader.cpp @@ -284,7 +284,10 @@ error_code replica_bulk_loader::download_sst_files(const std::string &app_name, uint64_t f_size = 0; error_code ec = _stub->_block_service_manager.download_file( remote_dir, local_dir, f_meta.name, fs, f_size); - if (ec == ERR_OK && !_stub->_block_service_manager.verify_file(f_meta, local_dir)) { + const std::string &file_name = + utils::filesystem::path_combine(local_dir, f_meta.name); + if (ec == ERR_OK && + !utils::filesystem::verify_file(file_name, f_meta.md5, f_meta.size)) { ec = ERR_CORRUPTION; } if (ec != ERR_OK) {