update_engine: Use FileStream in FilesystemVerifierAction.
Instead of dealing manually with the read from the partition while
verifying the filesystem, this patch uses chromeos::FileStream to do
that.
The base::MessageLoopForIO can't wait on regular files because libevent
will fail when trying to watch for such file descriptor. While
chromeos::Stream will still attempt to wait on the file descriptor in
such situation, that will be fixed in chromeos::Streams in a different
CL.
BUG=chromium:499886
TEST=Unittests still pass.
Change-Id: Icd6a031ac1137b1b5746a5c3413506ece79223cc
Reviewed-on: https://chromium-review.googlesource.com/290542
Reviewed-by: Alex Deymo <deymo@chromium.org>
Commit-Queue: Alex Deymo <deymo@chromium.org>
Trybot-Ready: Alex Deymo <deymo@chromium.org>
Tested-by: Alex Deymo <deymo@chromium.org>
diff --git a/filesystem_verifier_action.cc b/filesystem_verifier_action.cc
index 5c352b0..d3e9fe3 100644
--- a/filesystem_verifier_action.cc
+++ b/filesystem_verifier_action.cc
@@ -14,13 +14,12 @@
#include <string>
#include <base/bind.h>
-#include <base/posix/eintr_wrapper.h>
+#include <chromeos/streams/file_stream.h>
#include "update_engine/hardware_interface.h"
#include "update_engine/system_state.h"
#include "update_engine/utils.h"
-using chromeos::MessageLoop;
using std::string;
namespace chromeos_update_engine {
@@ -97,23 +96,23 @@
break;
}
- src_fd_ = HANDLE_EINTR(open(target_path.c_str(), O_RDONLY));
- if (src_fd_ < 0) {
- PLOG(ERROR) << "Unable to open " << target_path << " for reading";
+ chromeos::ErrorPtr error;
+ src_stream_ = chromeos::FileStream::Open(
+ base::FilePath(target_path),
+ chromeos::Stream::AccessMode::READ,
+ chromeos::FileStream::Disposition::OPEN_EXISTING,
+ &error);
+
+ if (!src_stream_) {
+ LOG(ERROR) << "Unable to open " << target_path << " for reading";
return;
}
- DetermineFilesystemSize(src_fd_);
+ DetermineFilesystemSize(target_path);
buffer_.resize(kReadFileBufferSize);
// Start the first read.
- read_task_ = MessageLoop::current()->WatchFileDescriptor(
- FROM_HERE,
- src_fd_,
- MessageLoop::WatchMode::kWatchRead,
- true, // persistent
- base::Bind(&FilesystemVerifierAction::OnReadReadyCallback,
- base::Unretained(this)));
+ ScheduleRead();
abort_action_completer.set_should_complete(false);
}
@@ -124,19 +123,11 @@
}
bool FilesystemVerifierAction::IsCleanupPending() const {
- return (src_fd_ != -1);
+ return src_stream_ != nullptr;
}
void FilesystemVerifierAction::Cleanup(ErrorCode code) {
- MessageLoop::current()->CancelTask(read_task_);
- read_task_ = MessageLoop::kTaskIdNull;
-
- if (src_fd_ != -1) {
- if (IGNORE_EINTR(close(src_fd_)) != 0) {
- PLOG(ERROR) << "Error closing fd " << src_fd_;
- }
- src_fd_ = -1;
- }
+ src_stream_.reset();
// This memory is not used anymore.
buffer_.clear();
@@ -147,74 +138,97 @@
processor_->ActionComplete(this, code);
}
-void FilesystemVerifierAction::OnReadReadyCallback() {
+void FilesystemVerifierAction::ScheduleRead() {
size_t bytes_to_read = std::min(static_cast<int64_t>(buffer_.size()),
remaining_size_);
-
- ssize_t bytes_read = 0;
- if (bytes_to_read) {
- bytes_read = HANDLE_EINTR(read(src_fd_, buffer_.data(), bytes_to_read));
- }
- if (bytes_read < 0) {
- PLOG(ERROR) << "Read failed";
- failed_ = true;
- } else if (bytes_read == 0) {
- read_done_ = true;
- } else {
- remaining_size_ -= bytes_read;
- }
-
- if (bytes_read > 0) {
- CHECK(!read_done_);
- if (!hasher_.Update(buffer_.data(), bytes_read)) {
- LOG(ERROR) << "Unable to update the hash.";
- failed_ = true;
- }
- }
-
- CheckTerminationConditions();
-}
-
-void FilesystemVerifierAction::CheckTerminationConditions() {
- if (failed_ || cancelled_) {
- Cleanup(ErrorCode::kError);
+ if (!bytes_to_read) {
+ OnReadDoneCallback(0);
return;
}
- if (read_done_) {
- // We're done!
- ErrorCode code = ErrorCode::kSuccess;
- if (hasher_.Finalize()) {
- LOG(INFO) << "Hash: " << hasher_.hash();
- switch (partition_type_) {
- case PartitionType::kRootfs:
- if (install_plan_.rootfs_hash != hasher_.raw_hash()) {
- code = ErrorCode::kNewRootfsVerificationError;
- LOG(ERROR) << "New rootfs verification failed.";
- }
- break;
- case PartitionType::kKernel:
- if (install_plan_.kernel_hash != hasher_.raw_hash()) {
- code = ErrorCode::kNewKernelVerificationError;
- LOG(ERROR) << "New kernel verification failed.";
- }
- break;
- case PartitionType::kSourceRootfs:
- install_plan_.source_rootfs_hash = hasher_.raw_hash();
- break;
- case PartitionType::kSourceKernel:
- install_plan_.source_kernel_hash = hasher_.raw_hash();
- break;
- }
- } else {
- LOG(ERROR) << "Unable to finalize the hash.";
- code = ErrorCode::kError;
- }
- Cleanup(code);
+ bool read_async_ok = src_stream_->ReadAsync(
+ buffer_.data(),
+ bytes_to_read,
+ base::Bind(&FilesystemVerifierAction::OnReadDoneCallback,
+ base::Unretained(this)),
+ base::Bind(&FilesystemVerifierAction::OnReadErrorCallback,
+ base::Unretained(this)),
+ nullptr);
+
+ if (!read_async_ok) {
+ LOG(ERROR) << "Unable to schedule an asynchronous read from the stream.";
+ Cleanup(ErrorCode::kError);
}
}
-void FilesystemVerifierAction::DetermineFilesystemSize(int fd) {
+void FilesystemVerifierAction::OnReadDoneCallback(size_t bytes_read) {
+ if (bytes_read == 0) {
+ read_done_ = true;
+ } else {
+ remaining_size_ -= bytes_read;
+ CHECK(!read_done_);
+ if (!hasher_.Update(buffer_.data(), bytes_read)) {
+ LOG(ERROR) << "Unable to update the hash.";
+ Cleanup(ErrorCode::kError);
+ return;
+ }
+ }
+
+ // We either terminate the action or have more data to read.
+ if (!CheckTerminationConditions())
+ ScheduleRead();
+}
+
+void FilesystemVerifierAction::OnReadErrorCallback(
+ const chromeos::Error* error) {
+ // TODO(deymo): Transform the read-error into an specific ErrorCode.
+ LOG(ERROR) << "Asynchronous read failed.";
+ Cleanup(ErrorCode::kError);
+}
+
+bool FilesystemVerifierAction::CheckTerminationConditions() {
+ if (cancelled_) {
+ Cleanup(ErrorCode::kError);
+ return true;
+ }
+
+ if (!read_done_)
+ return false;
+
+ // We're done!
+ ErrorCode code = ErrorCode::kSuccess;
+ if (hasher_.Finalize()) {
+ LOG(INFO) << "Hash: " << hasher_.hash();
+ switch (partition_type_) {
+ case PartitionType::kRootfs:
+ if (install_plan_.rootfs_hash != hasher_.raw_hash()) {
+ code = ErrorCode::kNewRootfsVerificationError;
+ LOG(ERROR) << "New rootfs verification failed.";
+ }
+ break;
+ case PartitionType::kKernel:
+ if (install_plan_.kernel_hash != hasher_.raw_hash()) {
+ code = ErrorCode::kNewKernelVerificationError;
+ LOG(ERROR) << "New kernel verification failed.";
+ }
+ break;
+ case PartitionType::kSourceRootfs:
+ install_plan_.source_rootfs_hash = hasher_.raw_hash();
+ break;
+ case PartitionType::kSourceKernel:
+ install_plan_.source_kernel_hash = hasher_.raw_hash();
+ break;
+ }
+ } else {
+ LOG(ERROR) << "Unable to finalize the hash.";
+ code = ErrorCode::kError;
+ }
+ Cleanup(code);
+ return true;
+}
+
+void FilesystemVerifierAction::DetermineFilesystemSize(
+ const std::string& path) {
switch (partition_type_) {
case PartitionType::kRootfs:
remaining_size_ = install_plan_.rootfs_size;
@@ -227,7 +241,7 @@
case PartitionType::kSourceRootfs:
{
int block_count = 0, block_size = 0;
- if (utils::GetFilesystemSizeFromFD(fd, &block_count, &block_size)) {
+ if (utils::GetFilesystemSize(path, &block_count, &block_size)) {
remaining_size_ = static_cast<int64_t>(block_count) * block_size;
LOG(INFO) << "Filesystem size: " << remaining_size_ << " bytes ("
<< block_count << "x" << block_size << ").";
@@ -237,7 +251,6 @@
default:
break;
}
- return;
}
} // namespace chromeos_update_engine
diff --git a/filesystem_verifier_action.h b/filesystem_verifier_action.h
index 69e4972..731790a 100644
--- a/filesystem_verifier_action.h
+++ b/filesystem_verifier_action.h
@@ -11,7 +11,7 @@
#include <string>
#include <vector>
-#include <chromeos/message_loops/message_loop.h>
+#include <chromeos/streams/stream.h>
#include <gtest/gtest_prod.h> // for FRIEND_TEST
#include "update_engine/action.h"
@@ -56,13 +56,17 @@
FRIEND_TEST(FilesystemVerifierActionTest,
RunAsRootDetermineFilesystemSizeTest);
- // Callback from the main loop when there's data to read from the file
- // descriptor.
- void OnReadReadyCallback();
+ // Schedules the asynchronous read of the filesystem.
+ void ScheduleRead();
- // Based on the state of the read buffers, terminates read process and the
- // action.
- void CheckTerminationConditions();
+ // Called from the main loop when a single read from |src_stream_| succeeds or
+ // fails, calling OnReadDoneCallback() and OnReadErrorCallback() respectively.
+ void OnReadDoneCallback(size_t bytes_read);
+ void OnReadErrorCallback(const chromeos::Error* error);
+
+ // Based on the state of the read buffer, terminates read process and the
+ // action. Return whether the action was terminated.
+ bool CheckTerminationConditions();
// Cleans up all the variables we use for async operations and tells the
// ActionProcessor we're done w/ |code| as passed in. |cancelled_| should be
@@ -72,23 +76,18 @@
// Determine, if possible, the source file system size to avoid copying the
// whole partition. Currently this supports only the root file system assuming
// it's ext3-compatible.
- void DetermineFilesystemSize(int fd);
+ void DetermineFilesystemSize(const std::string& path);
// The type of the partition that we are verifying.
PartitionType partition_type_;
- // If non-null, this is the GUnixInputStream object for the opened source
- // partition.
- int src_fd_{-1};
+ // If not null, the FileStream used to read from the device.
+ chromeos::StreamPtr src_stream_;
// Buffer for storing data we read.
chromeos::Blob buffer_;
- // The task id for the the in-flight async call.
- chromeos::MessageLoop::TaskId read_task_{chromeos::MessageLoop::kTaskIdNull};
-
bool read_done_{false}; // true if reached EOF on the input stream.
- bool failed_{false}; // true if the action has failed.
bool cancelled_{false}; // true if the action has been cancelled.
// The install plan we're passed in via the input pipe.
diff --git a/filesystem_verifier_action_unittest.cc b/filesystem_verifier_action_unittest.cc
index da0fd95..66f8571 100644
--- a/filesystem_verifier_action_unittest.cc
+++ b/filesystem_verifier_action_unittest.cc
@@ -352,6 +352,8 @@
TEST_F(FilesystemVerifierActionTest, RunAsRootTerminateEarlyTest) {
ASSERT_EQ(0, getuid());
EXPECT_TRUE(DoTest(true, false, PartitionType::kKernel));
+ // TerminateEarlyTest may leak some null callbacks from the Stream class.
+ while (loop_.RunOnce(false)) {}
}
TEST_F(FilesystemVerifierActionTest, RunAsRootDetermineFilesystemSizeTest) {
@@ -362,19 +364,18 @@
// Extend the "partition" holding the file system from 10MiB to 20MiB.
EXPECT_EQ(0, truncate(img.c_str(), 20 * 1024 * 1024));
- for (int i = 0; i < 2; ++i) {
- PartitionType fs_type =
- i ? PartitionType::kSourceKernel : PartitionType::kSourceRootfs;
- FilesystemVerifierAction action(&fake_system_state_, fs_type);
+ {
+ FilesystemVerifierAction action(&fake_system_state_,
+ PartitionType::kSourceKernel);
EXPECT_EQ(kint64max, action.remaining_size_);
- {
- int fd = HANDLE_EINTR(open(img.c_str(), O_RDONLY));
- EXPECT_GT(fd, 0);
- ScopedFdCloser fd_closer(&fd);
- action.DetermineFilesystemSize(fd);
- }
- EXPECT_EQ(i ? kint64max : 10 * 1024 * 1024,
- action.remaining_size_);
+ action.DetermineFilesystemSize(img);
+ EXPECT_EQ(kint64max, action.remaining_size_);
+ }
+ {
+ FilesystemVerifierAction action(&fake_system_state_,
+ PartitionType::kSourceRootfs);
+ action.DetermineFilesystemSize(img);
+ EXPECT_EQ(10 * 1024 * 1024, action.remaining_size_);
}
}