Fixes to utility / unit test related code.
* Eliminated bugs related to reading content from pipes/files, including
general cleanup/refactoring of these code pieces and API.
* Eliminated bugs related binding/unbinding of loopback devices, which
are used in unit testing.
BUG=chromium-os:31082
TEST=Builds and runs unit tests
CQ-DEPEND=Ib7b3552e98ca40b6141688e2dea5a1407db12b2a
Change-Id: Ifaab8697602a35ce7d7fb9384fdcb1ca64b72515
Reviewed-on: https://gerrit.chromium.org/gerrit/27911
Reviewed-by: Don Garrett <dgarrett@chromium.org>
Tested-by: Gilad Arnold <garnold@chromium.org>
Commit-Ready: Gilad Arnold <garnold@chromium.org>
diff --git a/delta_diff_generator_unittest.cc b/delta_diff_generator_unittest.cc
index 1a963da..6d989a1 100644
--- a/delta_diff_generator_unittest.cc
+++ b/delta_diff_generator_unittest.cc
@@ -427,7 +427,7 @@
new_blobs));
string new_data;
- EXPECT_TRUE(utils::ReadFileToString(new_blobs, &new_data));
+ EXPECT_TRUE(utils::ReadFile(new_blobs, &new_data));
EXPECT_EQ("bcda", new_data);
EXPECT_EQ(2, manifest.install_operations_size());
EXPECT_EQ(0, manifest.install_operations(0).data_offset());
diff --git a/integration_unittest.cc b/integration_unittest.cc
index 4c43b35..f42b89e 100644
--- a/integration_unittest.cc
+++ b/integration_unittest.cc
@@ -56,7 +56,8 @@
if (action->Type() == InstallAction::StaticType()) {
InstallAction* install_action = static_cast<InstallAction*>(action);
old_dev_ = install_action->GetOutputObject();
- string dev = BindToUnusedDevice(kTestDir + "/dev2");
+ string dev;
+ BindToUnusedDevice(kTestDir + "/dev2", &dev);
install_action->SetOutputObject(dev);
} else if (action->Type() == PostinstallRunnerAction::StaticType()) {
PostinstallRunnerAction* postinstall_runner_action =
@@ -178,7 +179,7 @@
ASSERT_EQ(0, lstat("/tmp/update_engine_test_postinst_out.txt", &stbuf));
EXPECT_TRUE(S_ISREG(stbuf.st_mode));
string file_data;
- EXPECT_TRUE(utils::ReadFileToString(
+ EXPECT_TRUE(utils::ReadFile(
"/tmp/update_engine_test_postinst_out.txt",
&file_data));
EXPECT_EQ("POSTINST_DONE\n", file_data);
diff --git a/omaha_request_params.cc b/omaha_request_params.cc
index 3733f10..e240c86 100644
--- a/omaha_request_params.cc
+++ b/omaha_request_params.cc
@@ -147,7 +147,7 @@
// TODO(adlr): make sure files checked are owned as root (and all their
// parents are recursively, too).
string file_data;
- if (!utils::ReadFileToString(root_ + *it, &file_data))
+ if (!utils::ReadFile(root_ + *it, &file_data))
continue;
map<string, string> data = simple_key_value_store::ParseString(file_data);
diff --git a/omaha_request_params_unittest.cc b/omaha_request_params_unittest.cc
index 090c514..ecaa682 100644
--- a/omaha_request_params_unittest.cc
+++ b/omaha_request_params_unittest.cc
@@ -55,22 +55,14 @@
namespace {
string GetMachineType() {
- FILE* fp = popen("uname -m", "r");
- if (!fp)
+ string machine_type;
+ if (!utils::ReadPipe("uname -m", &machine_type))
return "";
- string ret;
- for (;;) {
- char buffer[10];
- size_t r = fread(buffer, 1, sizeof(buffer), fp);
- if (r == 0)
- break;
- ret.insert(ret.begin(), buffer, buffer + r);
- }
- // strip trailing '\n' if it exists
- if ((*ret.rbegin()) == '\n')
- ret.resize(ret.size() - 1);
- fclose(fp);
- return ret;
+ // Strip anything from the first newline char.
+ size_t newline_pos = machine_type.find('\n');
+ if (newline_pos != string::npos)
+ machine_type.erase(newline_pos);
+ return machine_type;
}
} // namespace {}
diff --git a/omaha_response_handler_action_unittest.cc b/omaha_response_handler_action_unittest.cc
index 87fa1e8..9da48a6 100644
--- a/omaha_response_handler_action_unittest.cc
+++ b/omaha_response_handler_action_unittest.cc
@@ -107,7 +107,7 @@
EXPECT_EQ(in.hash, install_plan.download_hash);
EXPECT_EQ("/dev/sda5", install_plan.install_path);
string deadline;
- EXPECT_TRUE(utils::ReadFileToString(
+ EXPECT_TRUE(utils::ReadFile(
OmahaResponseHandlerAction::kDeadlineFile,
&deadline));
EXPECT_EQ("20101020", deadline);
@@ -133,7 +133,7 @@
EXPECT_EQ(in.hash, install_plan.download_hash);
EXPECT_EQ("/dev/sda3", install_plan.install_path);
string deadline;
- EXPECT_TRUE(utils::ReadFileToString(
+ EXPECT_TRUE(utils::ReadFile(
OmahaResponseHandlerAction::kDeadlineFile,
&deadline) && deadline.empty());
}
@@ -154,7 +154,7 @@
EXPECT_EQ(in.hash, install_plan.download_hash);
EXPECT_EQ("/dev/sda5", install_plan.install_path);
string deadline;
- EXPECT_TRUE(utils::ReadFileToString(
+ EXPECT_TRUE(utils::ReadFile(
OmahaResponseHandlerAction::kDeadlineFile,
&deadline));
EXPECT_EQ("some-deadline", deadline);
diff --git a/test_utils.cc b/test_utils.cc
index 40d300c..fddd2d1 100644
--- a/test_utils.cc
+++ b/test_utils.cc
@@ -138,31 +138,27 @@
return ret;
}
-string BindToUnusedLoopDevice(const string &filename) {
- // get a loop device we can use for the install device
- string cmd = "losetup --show -f " + filename;
+// Binds provided |filename| to an unused loopback device, whose name is written
+// to the string pointed to by |lo_dev_name_p|. Returns true on success, false
+// otherwise (along with corresponding test failures), in which case the content
+// of |lo_dev_name_p| is unknown.
+bool BindToUnusedLoopDevice(const string& filename, string* lo_dev_name_p) {
+ CHECK(lo_dev_name_p);
- FILE* find_dev_cmd = popen(cmd.c_str(), "r");
- CHECK(find_dev_cmd);
-
- string ret;
- char dev[100] = {0};
- size_t r;
- while ((r = fread(dev, 1, sizeof(dev - 1), find_dev_cmd)) > 0) {
- EXPECT_LT(r, sizeof(dev));
- ret.insert(ret.end(), dev, dev + r);
+ // Bind to an unused loopback device, sanity check the device name.
+ lo_dev_name_p->clear();
+ if (!(utils::ReadPipe("losetup --show -f " + filename, lo_dev_name_p) &&
+ StartsWithASCII(*lo_dev_name_p, "/dev/loop", true))) {
+ ADD_FAILURE();
+ return false;
}
- EXPECT_EQ(r, 0);
- EXPECT_TRUE(feof(find_dev_cmd));
- fclose(find_dev_cmd);
- // strip trailing \n on dev
- if (*ret.rbegin() == '\n')
- ret.resize(ret.size() - 1);
+ // Strip anything from the first newline char.
+ size_t newline_pos = lo_dev_name_p->find('\n');
+ if (newline_pos != string::npos)
+ lo_dev_name_p->erase(newline_pos);
- // Ensure that the device starts with "/dev/loop"
- EXPECT_TRUE(StartsWithASCII(ret, "/dev/loop", true));
- return ret;
+ return true;
}
bool ExpectVectorsEq(const vector<char>& expected, const vector<char>& actual) {
diff --git a/test_utils.h b/test_utils.h
index 86ee030..6d85157 100644
--- a/test_utils.h
+++ b/test_utils.h
@@ -36,7 +36,8 @@
// the first partition is marked bootable.
std::vector<char> GenerateSampleMbr();
-std::string BindToUnusedLoopDevice(const std::string &filename);
+bool BindToUnusedLoopDevice(const std::string &filename,
+ std::string* lo_dev_name_ptr);
// Returns true iff a == b
bool ExpectVectorsEq(const std::vector<char>& a, const std::vector<char>& b);
@@ -114,13 +115,17 @@
class ScopedLoopbackDeviceBinder {
public:
ScopedLoopbackDeviceBinder(const std::string& file, std::string* dev) {
- dev_ = BindToUnusedLoopDevice(file);
+ is_bound_ = BindToUnusedLoopDevice(file, &dev_);
+ EXPECT_TRUE(is_bound_);
- if (dev)
+ if (is_bound_ && dev)
*dev = dev_;
}
~ScopedLoopbackDeviceBinder() {
+ if (!is_bound_)
+ return;
+
for (int retry = 0; retry < 5; retry++) {
std::vector<std::string> args;
args.push_back("/sbin/losetup");
@@ -136,10 +141,14 @@
ADD_FAILURE();
}
- const std::string &dev() { return dev_; }
+ const std::string &dev() {
+ EXPECT_TRUE(is_bound_);
+ return dev_;
+ }
private:
std::string dev_;
+ bool is_bound_;
DISALLOW_COPY_AND_ASSIGN(ScopedLoopbackDeviceBinder);
};
diff --git a/utils.cc b/utils.cc
index 9cb8053..e0d9244 100644
--- a/utils.cc
+++ b/utils.cc
@@ -141,33 +141,70 @@
}
-bool ReadFile(const std::string& path, std::vector<char>* out) {
- CHECK(out);
+// Append |nbytes| of content from |buf| to the vector pointed to by either
+// |vec_p| or |str_p|.
+static void AppendBytes(const char* buf, size_t nbytes,
+ std::vector<char>* vec_p) {
+ CHECK(buf);
+ CHECK(vec_p);
+ vec_p->insert(vec_p->end(), buf, buf + nbytes);
+}
+static void AppendBytes(const char* buf, size_t nbytes,
+ std::string* str_p) {
+ CHECK(buf);
+ CHECK(str_p);
+ str_p->append(buf, nbytes);
+}
+
+// Reads from an open file |fp|, appending the read content to the container
+// pointer to by |out_p|. Returns true upon successful reading all of the
+// file's content, false otherwise.
+template <class T>
+static bool Read(FILE* fp, T* out_p) {
+ CHECK(fp);
+ char buf[1024];
+ while (size_t nbytes = fread(buf, 1, sizeof(buf), fp))
+ AppendBytes(buf, nbytes, out_p);
+ return feof(fp) && !ferror(fp);
+}
+
+// Opens a file |path| for reading, then uses |append_func| to append its
+// content to a container |out_p|.
+template <class T>
+static bool ReadFileAndAppend(const std::string& path, T* out_p) {
FILE* fp = fopen(path.c_str(), "r");
if (!fp)
return false;
- const size_t kChunkSize = 1024;
- size_t read_size;
- do {
- char buf[kChunkSize];
- read_size = fread(buf, 1, kChunkSize, fp);
- if (read_size == 0)
- break;
- out->insert(out->end(), buf, buf + read_size);
- } while (read_size == kChunkSize);
- bool success = !ferror(fp);
- TEST_AND_RETURN_FALSE_ERRNO(fclose(fp) == 0);
- return success;
+ bool success = Read(fp, out_p);
+ return (success && !fclose(fp));
}
-bool ReadFileToString(const std::string& path, std::string* out) {
- vector<char> data;
- bool success = ReadFile(path, &data);
- if (!success) {
+// Invokes a pipe |cmd|, then uses |append_func| to append its stdout to a
+// container |out_p|.
+template <class T>
+static bool ReadPipeAndAppend(const std::string& cmd, T* out_p) {
+ FILE* fp = popen(cmd.c_str(), "r");
+ if (!fp)
return false;
- }
- (*out) = string(&data[0], data.size());
- return true;
+ bool success = Read(fp, out_p);
+ return (success && pclose(fp) >= 0);
+}
+
+
+bool ReadFile(const std::string& path, std::vector<char>* out_p) {
+ return ReadFileAndAppend(path, out_p);
+}
+
+bool ReadFile(const std::string& path, std::string* out_p) {
+ return ReadFileAndAppend(path, out_p);
+}
+
+bool ReadPipe(const std::string& cmd, std::vector<char>* out_p) {
+ return ReadPipeAndAppend(cmd, out_p);
+}
+
+bool ReadPipe(const std::string& cmd, std::string* out_p) {
+ return ReadPipeAndAppend(cmd, out_p);
}
off_t FileSize(const string& path) {
diff --git a/utils.h b/utils.h
index 67e8062..1b3095b 100644
--- a/utils.h
+++ b/utils.h
@@ -48,9 +48,18 @@
bool PReadAll(int fd, void* buf, size_t count, off_t offset,
ssize_t* out_bytes_read);
-// Returns the entire contents of the file at path. Returns true on success.
-bool ReadFile(const std::string& path, std::vector<char>* out);
-bool ReadFileToString(const std::string& path, std::string* out);
+// Opens |path| for reading and appends its entire content to the container
+// pointed to by |out_p|. Returns true upon successfully reading all of the
+// file's content, false otherwise, in which case the state of the output
+// container is unknown.
+bool ReadFile(const std::string& path, std::vector<char>* out_p);
+bool ReadFile(const std::string& path, std::string* out_p);
+
+// Invokes |cmd| in a pipe and appends its stdout to the container pointed to by
+// |out_p|. Returns true upon successfully reading all of the output, false
+// otherwise, in which case the state of the output container is unknown.
+bool ReadPipe(const std::string& cmd, std::vector<char>* out_p);
+bool ReadPipe(const std::string& cmd, std::string* out_p);
// Returns the size of the file at path. If the file doesn't exist or some
// error occurrs, -1 is returned.