update_engine: Remove libgio usage from FilesystemVerifierAction.
The FilesystemVerifierAction used gio to read from the partition it
is verifying. This patch makes it use plain file descriptors and watch
them using chromeos::MessageLoop instead.
BUG=chromium:499886
TEST=updated unittests.
Change-Id: I5230399d5ac4777522bd5f53d4f4ade0a29a6c9f
Reviewed-on: https://chromium-review.googlesource.com/284648
Tested-by: Alex Deymo <deymo@chromium.org>
Reviewed-by: Alex Vakulenko <avakulenko@chromium.org>
Commit-Queue: Alex Deymo <deymo@chromium.org>
Trybot-Ready: Alex Deymo <deymo@chromium.org>
diff --git a/filesystem_verifier_action.cc b/filesystem_verifier_action.cc
index 8810763..70156d9 100644
--- a/filesystem_verifier_action.cc
+++ b/filesystem_verifier_action.cc
@@ -13,33 +13,27 @@
#include <cstdlib>
#include <string>
-#include <gio/gio.h>
-#include <gio/gunixinputstream.h>
-#include <glib.h>
+#include <base/bind.h>
+#include <base/posix/eintr_wrapper.h>
-#include "update_engine/glib_utils.h"
#include "update_engine/hardware_interface.h"
#include "update_engine/subprocess.h"
#include "update_engine/system_state.h"
#include "update_engine/utils.h"
+using chromeos::MessageLoop;
using std::string;
namespace chromeos_update_engine {
namespace {
-const off_t kCopyFileBufferSize = 128 * 1024;
+const off_t kReadFileBufferSize = 128 * 1024;
} // namespace
FilesystemVerifierAction::FilesystemVerifierAction(
SystemState* system_state,
PartitionType partition_type)
: partition_type_(partition_type),
- src_stream_(nullptr),
- canceller_(nullptr),
- read_done_(false),
- failed_(false),
- cancelled_(false),
remaining_size_(kint64max),
system_state_(system_state) {}
@@ -104,39 +98,49 @@
break;
}
- int src_fd = open(target_path.c_str(), O_RDONLY);
- if (src_fd < 0) {
- PLOG(ERROR) << "Unable to open " << target_path << " for reading:";
+ src_fd_ = HANDLE_EINTR(open(target_path.c_str(), O_RDONLY));
+ if (src_fd_ < 0) {
+ PLOG(ERROR) << "Unable to open " << target_path << " for reading";
return;
}
- DetermineFilesystemSize(src_fd);
- src_stream_ = g_unix_input_stream_new(src_fd, TRUE);
-
- buffer_.resize(kCopyFileBufferSize);
- canceller_ = g_cancellable_new();
+ DetermineFilesystemSize(src_fd_);
+ buffer_.resize(kReadFileBufferSize);
// Start the first read.
- SpawnAsyncActions();
+ read_task_ = MessageLoop::current()->WatchFileDescriptor(
+ FROM_HERE,
+ src_fd_,
+ MessageLoop::WatchMode::kWatchRead,
+ true, // persistent
+ base::Bind(&FilesystemVerifierAction::OnReadReadyCallback,
+ base::Unretained(this)));
abort_action_completer.set_should_complete(false);
}
void FilesystemVerifierAction::TerminateProcessing() {
- if (canceller_) {
- g_cancellable_cancel(canceller_);
- }
+ cancelled_ = true;
+ Cleanup(ErrorCode::kSuccess); // error code is ignored if canceled_ is true.
}
bool FilesystemVerifierAction::IsCleanupPending() const {
- return (src_stream_ != nullptr);
+ return (src_fd_ != -1);
}
void FilesystemVerifierAction::Cleanup(ErrorCode code) {
- g_object_unref(canceller_);
- canceller_ = nullptr;
- g_object_unref(src_stream_);
- src_stream_ = nullptr;
+ 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;
+ }
+ // This memory is not used anymore.
+ buffer_.clear();
+
if (cancelled_)
return;
if (code == ErrorCode::kSuccess && HasOutputPipe())
@@ -144,16 +148,16 @@
processor_->ActionComplete(this, code);
}
-void FilesystemVerifierAction::AsyncReadReadyCallback(GObject *source_object,
- GAsyncResult *res) {
- GError* error = nullptr;
- CHECK(canceller_);
- cancelled_ = g_cancellable_is_cancelled(canceller_) == TRUE;
+void FilesystemVerifierAction::OnReadReadyCallback() {
+ size_t bytes_to_read = std::min(static_cast<int64_t>(buffer_.size()),
+ remaining_size_);
- ssize_t bytes_read = g_input_stream_read_finish(src_stream_, res, &error);
-
+ 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) {
- LOG(ERROR) << "Read failed: " << utils::GetAndFreeGError(&error);
+ PLOG(ERROR) << "Read failed";
failed_ = true;
} else if (bytes_read == 0) {
read_done_ = true;
@@ -162,43 +166,23 @@
}
if (bytes_read > 0) {
- // If read_done_ is set, SpawnAsyncActions may finalize the hash so the hash
- // update below would happen too late.
CHECK(!read_done_);
if (!hasher_.Update(buffer_.data(), bytes_read)) {
LOG(ERROR) << "Unable to update the hash.";
failed_ = true;
}
}
- SpawnAsyncActions();
+
+ CheckTerminationConditions();
}
-void FilesystemVerifierAction::StaticAsyncReadReadyCallback(
- GObject *source_object,
- GAsyncResult *res,
- gpointer user_data) {
- reinterpret_cast<FilesystemVerifierAction*>(user_data)->
- AsyncReadReadyCallback(source_object, res);
-}
-
-void FilesystemVerifierAction::SpawnAsyncActions() {
+void FilesystemVerifierAction::CheckTerminationConditions() {
if (failed_ || cancelled_) {
Cleanup(ErrorCode::kError);
return;
}
- if (!read_done_) {
- int64_t bytes_to_read = std::min(static_cast<int64_t>(buffer_.size()),
- remaining_size_);
- g_input_stream_read_async(
- src_stream_,
- buffer_.data(),
- bytes_to_read,
- G_PRIORITY_DEFAULT,
- canceller_,
- &FilesystemVerifierAction::StaticAsyncReadReadyCallback,
- this);
- } else {
+ if (read_done_) {
// We're done!
ErrorCode code = ErrorCode::kSuccess;
if (hasher_.Finalize()) {
diff --git a/filesystem_verifier_action.h b/filesystem_verifier_action.h
index f934ab4..69e4972 100644
--- a/filesystem_verifier_action.h
+++ b/filesystem_verifier_action.h
@@ -11,8 +11,7 @@
#include <string>
#include <vector>
-#include <gio/gio.h>
-#include <glib.h>
+#include <chromeos/message_loops/message_loop.h>
#include <gtest/gtest_prod.h> // for FRIEND_TEST
#include "update_engine/action.h"
@@ -57,15 +56,13 @@
FRIEND_TEST(FilesystemVerifierActionTest,
RunAsRootDetermineFilesystemSizeTest);
- // Callbacks from glib when the read operation is done.
- void AsyncReadReadyCallback(GObject *source_object, GAsyncResult *res);
- static void StaticAsyncReadReadyCallback(GObject *source_object,
- GAsyncResult *res,
- gpointer user_data);
+ // Callback from the main loop when there's data to read from the file
+ // descriptor.
+ void OnReadReadyCallback();
- // Based on the state of the ping-pong buffers spawns appropriate read
- // actions asynchronously.
- void SpawnAsyncActions();
+ // Based on the state of the read buffers, terminates read process and the
+ // action.
+ void 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
@@ -82,17 +79,17 @@
// If non-null, this is the GUnixInputStream object for the opened source
// partition.
- GInputStream* src_stream_;
+ int src_fd_{-1};
// Buffer for storing data we read.
chromeos::Blob buffer_;
- // The cancellable object for the in-flight async calls.
- GCancellable* canceller_;
+ // The task id for the the in-flight async call.
+ chromeos::MessageLoop::TaskId read_task_{chromeos::MessageLoop::kTaskIdNull};
- bool read_done_; // true if reached EOF on the input stream.
- bool failed_; // true if the action has failed.
- bool cancelled_; // true if the action has been cancelled.
+ 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.
InstallPlan install_plan_;
diff --git a/filesystem_verifier_action_unittest.cc b/filesystem_verifier_action_unittest.cc
index a1842e8..da0fd95 100644
--- a/filesystem_verifier_action_unittest.cc
+++ b/filesystem_verifier_action_unittest.cc
@@ -10,10 +10,12 @@
#include <string>
#include <vector>
+#include <base/bind.h>
#include <base/posix/eintr_wrapper.h>
#include <base/strings/string_util.h>
#include <base/strings/stringprintf.h>
-#include <glib.h>
+#include <chromeos/message_loops/glib_message_loop.h>
+#include <chromeos/message_loops/message_loop_utils.h>
#include <gmock/gmock.h>
#include <gtest/gtest.h>
@@ -23,6 +25,7 @@
#include "update_engine/test_utils.h"
#include "update_engine/utils.h"
+using chromeos::MessageLoop;
using std::set;
using std::string;
using std::vector;
@@ -31,30 +34,40 @@
class FilesystemVerifierActionTest : public ::testing::Test {
protected:
+ void SetUp() override {
+ loop_.SetAsCurrent();
+ }
+
+ void TearDown() override {
+ EXPECT_EQ(0, chromeos::MessageLoopRunMaxIterations(&loop_, 1));
+ }
+
// Returns true iff test has completed successfully.
bool DoTest(bool terminate_early,
bool hash_fail,
PartitionType partition_type);
+ chromeos::GlibMessageLoop loop_;
FakeSystemState fake_system_state_;
};
class FilesystemVerifierActionTestDelegate : public ActionProcessorDelegate {
public:
- FilesystemVerifierActionTestDelegate(GMainLoop* loop,
- FilesystemVerifierAction* action)
- : loop_(loop), action_(action), ran_(false), code_(ErrorCode::kError) {}
+ explicit FilesystemVerifierActionTestDelegate(
+ FilesystemVerifierAction* action)
+ : action_(action), ran_(false), code_(ErrorCode::kError) {}
void ExitMainLoop() {
- GMainContext* context = g_main_loop_get_context(loop_);
- // We cannot use g_main_context_pending() alone to determine if it is safe
- // to quit the main loop here because g_main_context_pending() may return
- // FALSE when g_input_stream_read_async() in FilesystemVerifierAction has
- // been cancelled but the callback has not yet been invoked.
- while (g_main_context_pending(context) || action_->IsCleanupPending()) {
- g_main_context_iteration(context, false);
- g_usleep(100);
+ // We need to wait for the Action to call Cleanup.
+ if (action_->IsCleanupPending()) {
+ LOG(INFO) << "Waiting for Cleanup() to be called.";
+ MessageLoop::current()->PostDelayedTask(
+ FROM_HERE,
+ base::Bind(&FilesystemVerifierActionTestDelegate::ExitMainLoop,
+ base::Unretained(this)),
+ base::TimeDelta::FromMilliseconds(100));
+ } else {
+ MessageLoop::current()->BreakLoop();
}
- g_main_loop_quit(loop_);
}
void ProcessingDone(const ActionProcessor* processor, ErrorCode code) {
ExitMainLoop();
@@ -74,28 +87,19 @@
ErrorCode code() const { return code_; }
private:
- GMainLoop* loop_;
FilesystemVerifierAction* action_;
bool ran_;
ErrorCode code_;
};
-struct StartProcessorCallbackArgs {
- ActionProcessor* processor;
- FilesystemVerifierAction* filesystem_copier_action;
- bool terminate_early;
-};
-
-gboolean StartProcessorInRunLoop(gpointer data) {
- StartProcessorCallbackArgs* args =
- reinterpret_cast<StartProcessorCallbackArgs*>(data);
- ActionProcessor* processor = args->processor;
+void StartProcessorInRunLoop(ActionProcessor* processor,
+ FilesystemVerifierAction* filesystem_copier_action,
+ bool terminate_early) {
processor->StartProcessing();
- if (args->terminate_early) {
- EXPECT_TRUE(args->filesystem_copier_action);
- args->processor->StopProcessing();
+ if (terminate_early) {
+ EXPECT_NE(nullptr, filesystem_copier_action);
+ processor->StopProcessing();
}
- return FALSE;
}
// TODO(garnold) Temporarily disabling this test, see chromium-os:31082 for
@@ -120,8 +124,6 @@
testing::NiceMock<MockHardware> mock_hardware;
fake_system_state_.set_hardware(&mock_hardware);
- GMainLoop *loop = g_main_loop_new(g_main_context_default(), FALSE);
-
string a_loop_file;
if (!(utils::MakeTempFile("a_loop_file.XXXXXX", &a_loop_file, nullptr))) {
@@ -130,7 +132,7 @@
}
ScopedPathUnlinker a_loop_file_unlinker(a_loop_file);
- // Make random data for a, zero filled data for b.
+ // Make random data for a.
const size_t kLoopFileSize = 10 * 1024 * 1024 + 512;
chromeos::Blob a_loop_data(kLoopFileSize);
test_utils::FillWithData(&a_loop_data);
@@ -206,7 +208,7 @@
BondActions(&feeder_action, &copier_action);
BondActions(&copier_action, &collector_action);
- FilesystemVerifierActionTestDelegate delegate(loop, &copier_action);
+ FilesystemVerifierActionTestDelegate delegate(&copier_action);
processor.set_delegate(&delegate);
processor.EnqueueAction(&feeder_action);
processor.EnqueueAction(&copier_action);
@@ -214,14 +216,11 @@
feeder_action.set_obj(install_plan);
- StartProcessorCallbackArgs start_callback_args;
- start_callback_args.processor = &processor;
- start_callback_args.filesystem_copier_action = &copier_action;
- start_callback_args.terminate_early = terminate_early;
-
- g_timeout_add(0, &StartProcessorInRunLoop, &start_callback_args);
- g_main_loop_run(loop);
- g_main_loop_unref(loop);
+ loop_.PostTask(FROM_HERE, base::Bind(&StartProcessorInRunLoop,
+ &processor,
+ &copier_action,
+ terminate_early));
+ loop_.Run();
if (!terminate_early) {
bool is_delegate_ran = delegate.ran();
@@ -277,7 +276,6 @@
code_ = code;
}
}
- GMainLoop *loop_;
bool ran_;
ErrorCode code_;
};
@@ -362,10 +360,7 @@
ScopedPathUnlinker img_unlinker(img);
test_utils::CreateExtImageAtPath(img, nullptr);
// Extend the "partition" holding the file system from 10MiB to 20MiB.
- EXPECT_EQ(0, test_utils::System(base::StringPrintf(
- "dd if=/dev/zero of=%s seek=20971519 bs=1 count=1 status=none",
- img.c_str())));
- EXPECT_EQ(20 * 1024 * 1024, utils::FileSize(img));
+ EXPECT_EQ(0, truncate(img.c_str(), 20 * 1024 * 1024));
for (int i = 0; i < 2; ++i) {
PartitionType fs_type =
diff --git a/update_engine.gyp b/update_engine.gyp
index 6df8e7e..d3232a3 100644
--- a/update_engine.gyp
+++ b/update_engine.gyp
@@ -83,8 +83,6 @@
'exported_deps': [
'dbus-1',
'dbus-glib-1',
- 'gio-2.0',
- 'gio-unix-2.0',
'glib-2.0',
'gthread-2.0',
'libchrome-<(libbase_ver)',