vts_libsnapshot_test: Fix test flakiness.
This patch fixes a few lingering issues in vts_libsnapshot_test.
The most important fix is a crash in snapuserd when handler deletion
races with the merge monitor thread. Since tests issue lots of
snapshot-related requests in rapid succession, this was easy to hit in
presubmit, and resulted in a null-pointer deref.
SnapuserdClient's CloseConnection does the same thing as the destructor,
but leaves SnapuserdClient in an unusable state. This method is removed
in favor of RAII.
Fix a bug in SnapshotManager where CloseConnection could be called
without zapping snapuserd_client_.
Fix a bug where POLLHUP was checked before calling recv().
Add test name logging so presubmit failures can be diagnosed via logcat
dumps.
Bug: N/A
Test: vts_libsnapshot_test on cuttlefish
Change-Id: I8f22a45e537c24a3c6d327ac47bf8b1352108706
diff --git a/fs_mgr/libsnapshot/snapshot.cpp b/fs_mgr/libsnapshot/snapshot.cpp
index bc3efd9..59abd6f 100644
--- a/fs_mgr/libsnapshot/snapshot.cpp
+++ b/fs_mgr/libsnapshot/snapshot.cpp
@@ -1498,7 +1498,6 @@
if (UpdateUsesUserSnapshots(lock) && !device()->IsTestDevice()) {
if (snapuserd_client_) {
snapuserd_client_->DetachSnapuserd();
- snapuserd_client_->CloseConnection();
snapuserd_client_ = nullptr;
}
}
@@ -2794,7 +2793,6 @@
if (snapuserd_client_) {
LOG(INFO) << "Shutdown snapuserd daemon";
snapuserd_client_->DetachSnapuserd();
- snapuserd_client_->CloseConnection();
snapuserd_client_ = nullptr;
}
@@ -3317,7 +3315,7 @@
}
if (snapuserd_client) {
snapuserd_client->DetachSnapuserd();
- snapuserd_client->CloseConnection();
+ snapuserd_client = nullptr;
}
}
diff --git a/fs_mgr/libsnapshot/snapshot_test.cpp b/fs_mgr/libsnapshot/snapshot_test.cpp
index 2233a38..2c01cf6 100644
--- a/fs_mgr/libsnapshot/snapshot_test.cpp
+++ b/fs_mgr/libsnapshot/snapshot_test.cpp
@@ -107,6 +107,12 @@
protected:
void SetUp() override {
+ const testing::TestInfo* const test_info =
+ testing::UnitTest::GetInstance()->current_test_info();
+ test_name_ = test_info->test_suite_name() + "/"s + test_info->name();
+
+ LOG(INFO) << "Starting test: " << test_name_;
+
SKIP_IF_NON_VIRTUAL_AB();
SetupProperties();
@@ -152,10 +158,14 @@
void TearDown() override {
RETURN_IF_NON_VIRTUAL_AB();
+ LOG(INFO) << "Tearing down SnapshotTest test: " << test_name_;
+
lock_ = nullptr;
CleanupTestArtifacts();
SnapshotTestPropertyFetcher::TearDown();
+
+ LOG(INFO) << "Teardown complete for test: " << test_name_;
}
void InitializeState() {
@@ -487,6 +497,7 @@
android::fiemap::IImageManager* image_manager_ = nullptr;
std::string fake_super_;
bool snapuserd_required_ = false;
+ std::string test_name_;
};
TEST_F(SnapshotTest, CreateSnapshot) {
@@ -1003,6 +1014,8 @@
void TearDown() override {
RETURN_IF_NON_VIRTUAL_AB();
+ LOG(INFO) << "Tearing down SnapshotUpdateTest test: " << test_name_;
+
Cleanup();
SnapshotTest::TearDown();
}
@@ -2797,7 +2810,6 @@
return;
}
snapuserd_client->DetachSnapuserd();
- snapuserd_client->CloseConnection();
}
} // namespace snapshot
diff --git a/fs_mgr/libsnapshot/snapuserd/include/snapuserd/snapuserd_client.h b/fs_mgr/libsnapshot/snapuserd/include/snapuserd/snapuserd_client.h
index 9a69d58..4b62b20 100644
--- a/fs_mgr/libsnapshot/snapuserd/include/snapuserd/snapuserd_client.h
+++ b/fs_mgr/libsnapshot/snapuserd/include/snapuserd/snapuserd_client.h
@@ -71,8 +71,6 @@
// must ONLY be called if the control device has already been deleted.
bool WaitForDeviceDelete(const std::string& control_device);
- void CloseConnection() { sockfd_ = {}; }
-
// Detach snapuserd. This shuts down the listener socket, and will cause
// snapuserd to gracefully exit once all handler threads have terminated.
// This should only be used on first-stage instances of snapuserd.
diff --git a/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_server.cpp b/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_server.cpp
index 1bf33c8..d437d32 100644
--- a/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_server.cpp
+++ b/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_server.cpp
@@ -462,15 +462,14 @@
}
bool UserSnapshotServer::HandleClient(android::base::borrowed_fd fd, int revents) {
- if (revents & POLLHUP) {
- LOG(DEBUG) << "Snapuserd client disconnected";
- return false;
- }
-
std::string str;
if (!Recv(fd, &str)) {
return false;
}
+ if (str.empty() && (revents & POLLHUP)) {
+ LOG(DEBUG) << "Snapuserd client disconnected";
+ return false;
+ }
if (!Receivemsg(fd, str)) {
LOG(ERROR) << "Encountered error handling client message, revents: " << revents;
return false;
@@ -650,6 +649,12 @@
while (active_merge_threads_ < kMaxMergeThreads && merge_handlers_.size() > 0) {
auto handler = merge_handlers_.front();
merge_handlers_.pop();
+
+ if (!handler->snapuserd()) {
+ LOG(INFO) << "MonitorMerge: skipping deleted handler: " << handler->misc_name();
+ continue;
+ }
+
LOG(INFO) << "Starting merge for partition: "
<< handler->snapuserd()->GetMiscName();
handler->snapuserd()->InitiateMerge();