Revise loopback device handling to be parallelization safe.
Update engine tests were allocating loopback devices in a non-atomic fashion.
When they were run in parallel with other tests that were using loopback
devices, this could lead to two tests using the same device and failing.
This change follows jrbarnette's advice and allocates loopback devices
atomically using "losetup --show -f <file to bind>".
NOTE: integration_unittest.cc is not currently being built or run, and
so the change here has not been verified. I only discovered it wasn't being
built while making this change, and fixing/deleting it seemed out of scope
for the change.
BUG=chromium-os:24975
TEST=Ran unittests
Change-Id: Ica060c8add009cac134e22b46b4e87588d0246e0
Reviewed-on: https://gerrit.chromium.org/gerrit/15128
Reviewed-by: Richard Barnette <jrbarnette@chromium.org>
Reviewed-by: Darin Petkov <petkov@chromium.org>
Tested-by: Don Garrett <dgarrett@chromium.org>
Commit-Ready: Don Garrett <dgarrett@chromium.org>
diff --git a/filesystem_copier_action_unittest.cc b/filesystem_copier_action_unittest.cc
index 7926e22..0e2f6d0 100644
--- a/filesystem_copier_action_unittest.cc
+++ b/filesystem_copier_action_unittest.cc
@@ -128,20 +128,12 @@
EXPECT_TRUE(WriteFileVector(a_loop_file, a_loop_data));
EXPECT_TRUE(WriteFileVector(b_loop_file, b_loop_data));
- // Make loop devices for the files
- string a_dev = GetUnusedLoopDevice();
- EXPECT_FALSE(a_dev.empty());
- EXPECT_EQ(0, System(StringPrintf("losetup %s %s",
- a_dev.c_str(),
- a_loop_file.c_str())));
- ScopedLoopbackDeviceReleaser a_dev_releaser(a_dev);
+ // Attach loop devices to the files
+ string a_dev;
+ string b_dev;
- string b_dev = GetUnusedLoopDevice();
- EXPECT_FALSE(b_dev.empty());
- EXPECT_EQ(0, System(StringPrintf("losetup %s %s",
- b_dev.c_str(),
- b_loop_file.c_str())));
- ScopedLoopbackDeviceReleaser b_dev_releaser(b_dev);
+ ScopedLoopbackDeviceBinder a_dev_releaser(a_loop_file, &a_dev);
+ ScopedLoopbackDeviceBinder b_dev_releaser(b_loop_file, &b_dev);
// Set up the action objects
InstallPlan install_plan;
diff --git a/integration_unittest.cc b/integration_unittest.cc
index c20d810..4c43b35 100644
--- a/integration_unittest.cc
+++ b/integration_unittest.cc
@@ -56,9 +56,7 @@
if (action->Type() == InstallAction::StaticType()) {
InstallAction* install_action = static_cast<InstallAction*>(action);
old_dev_ = install_action->GetOutputObject();
- string dev = GetUnusedLoopDevice();
- string cmd = string("losetup ") + dev + " " + kTestDir + "/dev2";
- EXPECT_EQ(0, system(cmd.c_str()));
+ string dev = BindToUnusedDevice(kTestDir + "/dev2");
install_action->SetOutputObject(dev);
} else if (action->Type() == PostinstallRunnerAction::StaticType()) {
PostinstallRunnerAction* postinstall_runner_action =
diff --git a/postinstall_runner_action_unittest.cc b/postinstall_runner_action_unittest.cc
index 69a5eb0..a5649e0 100644
--- a/postinstall_runner_action_unittest.cc
+++ b/postinstall_runner_action_unittest.cc
@@ -123,24 +123,12 @@
ASSERT_EQ(0, System(string("rm -f ") + cwd + "/postinst_called"));
// get a loop device we can use for the install device
- FILE* find_dev_cmd = popen("losetup -f", "r");
- ASSERT_TRUE(find_dev_cmd);
+ string dev = "/dev/null";
- char dev[100] = {0};
- size_t r = fread(dev, 1, sizeof(dev), find_dev_cmd);
- ASSERT_GT(r, 0);
- ASSERT_LT(r, sizeof(dev));
- ASSERT_TRUE(feof(find_dev_cmd));
- fclose(find_dev_cmd);
-
- // strip trailing newline on dev
- if (dev[strlen(dev) - 1] == '\n')
- dev[strlen(dev) - 1] = '\0';
-
- scoped_ptr<ScopedLoopbackDeviceReleaser> loop_releaser;
+ scoped_ptr<ScopedLoopbackDeviceBinder> loop_releaser;
if (do_losetup) {
- ASSERT_EQ(0, System(string("losetup ") + dev + " " + cwd + "/image.dat"));
- loop_releaser.reset(new ScopedLoopbackDeviceReleaser(dev));
+ loop_releaser.reset(new ScopedLoopbackDeviceBinder(cwd + "/image.dat",
+ &dev));
}
ActionProcessor processor;
diff --git a/test_utils.cc b/test_utils.cc
index 85c73d7..fcaae46 100644
--- a/test_utils.cc
+++ b/test_utils.cc
@@ -136,9 +136,11 @@
return ret;
}
-std::string GetUnusedLoopDevice() {
+string BindToUnusedLoopDevice(const string &filename) {
// get a loop device we can use for the install device
- FILE* find_dev_cmd = popen("losetup -f", "r");
+ string cmd = "losetup --show -f " + filename;
+
+ FILE* find_dev_cmd = popen(cmd.c_str(), "r");
CHECK(find_dev_cmd);
string ret;
@@ -156,6 +158,8 @@
if (*ret.rbegin() == '\n')
ret.resize(ret.size() - 1);
+ // Ensure that the device starts with "/dev/loop"
+ EXPECT_TRUE(StartsWithASCII(ret, "/dev/loop", true));
return ret;
}
@@ -281,16 +285,14 @@
}
}
-ScopedLoopMounter::ScopedLoopMounter(const std::string& file_path,
- std::string* mnt_path,
+ScopedLoopMounter::ScopedLoopMounter(const string& file_path,
+ string* mnt_path,
unsigned long flags) {
EXPECT_TRUE(utils::MakeTempDirectory("/tmp/mnt.XXXXXX", mnt_path));
dir_remover_.reset(new ScopedDirRemover(*mnt_path));
- std::string loop_dev = GetUnusedLoopDevice();
- EXPECT_EQ(0, system(StringPrintf("losetup %s %s", loop_dev.c_str(),
- file_path.c_str()).c_str()));
- loop_releaser_.reset(new ScopedLoopbackDeviceReleaser(loop_dev));
+ string loop_dev;
+ loop_binder_.reset(new ScopedLoopbackDeviceBinder(file_path, &loop_dev));
EXPECT_TRUE(utils::MountFilesystem(loop_dev, *mnt_path, flags));
unmounter_.reset(new ScopedFilesystemUnmounter(*mnt_path));
diff --git a/test_utils.h b/test_utils.h
index b5ac853..86ee030 100644
--- a/test_utils.h
+++ b/test_utils.h
@@ -36,7 +36,7 @@
// the first partition is marked bootable.
std::vector<char> GenerateSampleMbr();
-std::string GetUnusedLoopDevice();
+std::string BindToUnusedLoopDevice(const std::string &filename);
// Returns true iff a == b
bool ExpectVectorsEq(const std::vector<char>& a, const std::vector<char>& b);
@@ -111,10 +111,16 @@
void VerifyAllPaths(const std::string& parent,
std::set<std::string> expected_paths);
-class ScopedLoopbackDeviceReleaser {
+class ScopedLoopbackDeviceBinder {
public:
- explicit ScopedLoopbackDeviceReleaser(const std::string& dev) : dev_(dev) {}
- ~ScopedLoopbackDeviceReleaser() {
+ ScopedLoopbackDeviceBinder(const std::string& file, std::string* dev) {
+ dev_ = BindToUnusedLoopDevice(file);
+
+ if (dev)
+ *dev = dev_;
+ }
+
+ ~ScopedLoopbackDeviceBinder() {
for (int retry = 0; retry < 5; retry++) {
std::vector<std::string> args;
args.push_back("/sbin/losetup");
@@ -129,9 +135,12 @@
}
ADD_FAILURE();
}
+
+ const std::string &dev() { return dev_; }
+
private:
- const std::string dev_;
- DISALLOW_COPY_AND_ASSIGN(ScopedLoopbackDeviceReleaser);
+ std::string dev_;
+ DISALLOW_COPY_AND_ASSIGN(ScopedLoopbackDeviceBinder);
};
class ScopedTempFile {
@@ -227,10 +236,10 @@
private:
// These objects must be destructed in the following order:
// ScopedFilesystemUnmounter (the file system must be unmounted first)
- // ScopedLoopbackDeviceReleaser (then the loop device can be deleted)
+ // ScopedLoopbackDeviceBinder (then the loop device can be deleted)
// ScopedDirRemover (then the mount point can be deleted)
scoped_ptr<ScopedDirRemover> dir_remover_;
- scoped_ptr<ScopedLoopbackDeviceReleaser> loop_releaser_;
+ scoped_ptr<ScopedLoopbackDeviceBinder> loop_binder_;
scoped_ptr<ScopedFilesystemUnmounter> unmounter_;
};