Switch FileWriter::Write to boolean result code.
FileWriter::Write returned either the number of bytes written, or a negative
error code. No clients were doing anything with the result other than checking
for success or failure, and many clients were considering any non-zero result
success.
So, I changed the API to return less information, but just the information the
clients needed. Success or failure.
BUG=chromium-os:8521
TEST=Unittests
Change-Id: I51513d6aa7b704dc27fb90d5fae4dc7118a3f86c
Reviewed-on: https://gerrit.chromium.org/gerrit/11532
Reviewed-by: Andrew de los Reyes <[email protected]>
Tested-by: Don Garrett <[email protected]>
Commit-Ready: Don Garrett <[email protected]>
diff --git a/delta_diff_generator.cc b/delta_diff_generator.cc
index 1c962f1..cc8f352 100644
--- a/delta_diff_generator.cc
+++ b/delta_diff_generator.cc
@@ -337,8 +337,7 @@
// Returns true on success.
bool WriteUint64AsBigEndian(FileWriter* writer, const uint64_t value) {
uint64_t value_be = htobe64(value);
- TEST_AND_RETURN_FALSE(writer->Write(&value_be, sizeof(value_be)) ==
- sizeof(value_be));
+ TEST_AND_RETURN_FALSE(writer->Write(&value_be, sizeof(value_be)));
return true;
}
@@ -1163,8 +1162,7 @@
TEST_AND_RETURN_FALSE(rc == static_cast<ssize_t>(buf.size()));
op->set_data_offset(out_file_size);
- TEST_AND_RETURN_FALSE(writer.Write(&buf[0], buf.size()) ==
- static_cast<ssize_t>(buf.size()));
+ TEST_AND_RETURN_FALSE(writer.Write(&buf[0], buf.size()));
out_file_size += buf.size();
}
return true;
@@ -1486,8 +1484,7 @@
ScopedFileWriterCloser writer_closer(&writer);
// Write header
- TEST_AND_RETURN_FALSE(writer.Write(kDeltaMagic, strlen(kDeltaMagic)) ==
- static_cast<ssize_t>(strlen(kDeltaMagic)));
+ TEST_AND_RETURN_FALSE(writer.Write(kDeltaMagic, strlen(kDeltaMagic)));
// Write version number
TEST_AND_RETURN_FALSE(WriteUint64AsBigEndian(&writer, kVersionNumber));
@@ -1500,8 +1497,7 @@
LOG(INFO) << "Writing final delta file protobuf... "
<< serialized_manifest.size();
TEST_AND_RETURN_FALSE(writer.Write(serialized_manifest.data(),
- serialized_manifest.size()) ==
- static_cast<ssize_t>(serialized_manifest.size()));
+ serialized_manifest.size()));
// Append the data blobs
LOG(INFO) << "Writing final delta file data blobs...";
@@ -1516,7 +1512,7 @@
break;
}
TEST_AND_RETURN_FALSE_ERRNO(rc > 0);
- TEST_AND_RETURN_FALSE(writer.Write(buf, rc) == rc);
+ TEST_AND_RETURN_FALSE(writer.Write(buf, rc));
}
// Write signature blob.
@@ -1528,8 +1524,7 @@
vector<string>(1, private_key_path),
&signature_blob));
TEST_AND_RETURN_FALSE(writer.Write(&signature_blob[0],
- signature_blob.size()) ==
- static_cast<ssize_t>(signature_blob.size()));
+ signature_blob.size()));
}
int64_t manifest_metadata_size =
diff --git a/delta_performer.cc b/delta_performer.cc
index 0e49df7..66c23d4 100644
--- a/delta_performer.cc
+++ b/delta_performer.cc
@@ -232,11 +232,9 @@
}
-// Wrapper around write. Returns bytes written on success or
-// -errno on error.
-// This function performs as many actions as it can, given the amount of
-// data received thus far.
-ssize_t DeltaPerformer::Write(const void* bytes, size_t count) {
+// Wrapper around write. Returns true if all requested bytes
+// were written, or false on any error, reguardless of progress.
+bool DeltaPerformer::Write(const void* bytes, size_t count) {
const char* c_bytes = reinterpret_cast<const char*>(bytes);
buffer_.insert(buffer_.end(), c_bytes, c_bytes + count);
@@ -245,10 +243,10 @@
&manifest_,
&manifest_metadata_size_);
if (result == kMetadataParseError) {
- return -EINVAL;
+ return false;
}
if (result == kMetadataParseInsufficientData) {
- return count;
+ return true;
}
// Remove protobuf and header info from buffer_, so buffer_ contains
// just data blobs
@@ -260,7 +258,7 @@
LogPartitionInfo(manifest_);
if (!PrimeUpdateState()) {
LOG(ERROR) << "Unable to prime the update state.";
- return -EINVAL;
+ return false;
}
}
ssize_t total_operations = manifest_.install_operations_size() +
@@ -289,25 +287,25 @@
if (!PerformReplaceOperation(op, is_kernel_partition)) {
LOG(ERROR) << "Failed to perform replace operation "
<< next_operation_num_;
- return -EINVAL;
+ return false;
}
} else if (op.type() == DeltaArchiveManifest_InstallOperation_Type_MOVE) {
if (!PerformMoveOperation(op, is_kernel_partition)) {
LOG(ERROR) << "Failed to perform move operation "
<< next_operation_num_;
- return -EINVAL;
+ return false;
}
} else if (op.type() == DeltaArchiveManifest_InstallOperation_Type_BSDIFF) {
if (!PerformBsdiffOperation(op, is_kernel_partition)) {
LOG(ERROR) << "Failed to perform bsdiff operation "
<< next_operation_num_;
- return -EINVAL;
+ return false;
}
}
next_operation_num_++;
CheckpointUpdateProgress();
}
- return count;
+ return true;
}
bool DeltaPerformer::CanPerformInstallOperation(
diff --git a/delta_performer.h b/delta_performer.h
index 5c1f3ab..dd8f8d8 100644
--- a/delta_performer.h
+++ b/delta_performer.h
@@ -52,9 +52,9 @@
// Open()ed again.
int Open(const char* path, int flags, mode_t mode);
- // Wrapper around write. Returns bytes written on success or
- // -errno on error.
- ssize_t Write(const void* bytes, size_t count);
+ // Wrapper around write. Returns true if all requested bytes
+ // were written, or false on any error, reguardless of progress.
+ bool Write(const void* bytes, size_t count);
// Wrapper around close. Returns 0 on success or -errno on error.
// Closes both 'path' given to Open() and the kernel path.
diff --git a/delta_performer_unittest.cc b/delta_performer_unittest.cc
index 0983cd3..b820ab8 100644
--- a/delta_performer_unittest.cc
+++ b/delta_performer_unittest.cc
@@ -484,7 +484,7 @@
const size_t kBytesPerWrite = 5;
for (size_t i = 0; i < delta.size(); i += kBytesPerWrite) {
size_t count = min(delta.size() - i, kBytesPerWrite);
- EXPECT_EQ(count, performer.Write(&delta[i], count));
+ EXPECT_TRUE(performer.Write(&delta[i], count));
}
// Wrapper around close. Returns 0 on success or -errno on error.
@@ -586,9 +586,9 @@
DeltaPerformer performer(&prefs);
EXPECT_EQ(0, performer.Open("/dev/null", 0, 0));
EXPECT_TRUE(performer.OpenKernel("/dev/null"));
- EXPECT_EQ(4, performer.Write("junk", 4));
- EXPECT_EQ(8, performer.Write("morejunk", 8));
- EXPECT_LT(performer.Write("morejunk", 8), 0);
+ EXPECT_TRUE(performer.Write("junk", 4));
+ EXPECT_TRUE(performer.Write("morejunk", 8));
+ EXPECT_FALSE(performer.Write("morejunk", 8));
EXPECT_LT(performer.Close(), 0);
}
diff --git a/download_action.cc b/download_action.cc
index 989625d..b08c1fc 100644
--- a/download_action.cc
+++ b/download_action.cc
@@ -96,7 +96,7 @@
bytes_received_ += length;
if (delegate_)
delegate_->BytesReceived(bytes_received_, install_plan_.size);
- if (writer_ && writer_->Write(bytes, length) < 0) {
+ if (writer_ && !writer_->Write(bytes, length)) {
LOG(ERROR) << "Write error -- terminating processing.";
// Don't tell the action processor that the action is complete until we get
// the TransferTerminated callback. Otherwise, this and the HTTP fetcher
diff --git a/download_action_unittest.cc b/download_action_unittest.cc
index bf12326..799d8af 100644
--- a/download_action_unittest.cc
+++ b/download_action_unittest.cc
@@ -81,9 +81,9 @@
TestDirectFileWriter() : fail_write_(0), current_write_(0) {}
void set_fail_write(int fail_write) { fail_write_ = fail_write; }
- virtual ssize_t Write(const void* bytes, size_t count) {
+ virtual bool Write(const void* bytes, size_t count) {
if (++current_write_ == fail_write_) {
- return -EINVAL;
+ return false;
}
return DirectFileWriter::Write(bytes, count);
}
diff --git a/file_writer.cc b/file_writer.cc
index 1a95a75..ad29844 100644
--- a/file_writer.cc
+++ b/file_writer.cc
@@ -15,7 +15,7 @@
return 0;
}
-ssize_t DirectFileWriter::Write(const void* bytes, size_t count) {
+bool DirectFileWriter::Write(const void* bytes, size_t count) {
CHECK_GE(fd_, 0);
const char* char_bytes = reinterpret_cast<const char*>(bytes);
@@ -24,11 +24,11 @@
ssize_t rc = write(fd_, char_bytes + bytes_written,
count - bytes_written);
if (rc < 0)
- return -errno;
+ return false;
bytes_written += rc;
}
CHECK_EQ(bytes_written, count);
- return bytes_written;
+ return bytes_written == count;
}
int DirectFileWriter::Close() {
diff --git a/file_writer.h b/file_writer.h
index a534b96..5fd4c5a 100644
--- a/file_writer.h
+++ b/file_writer.h
@@ -27,9 +27,9 @@
// Wrapper around open. Returns 0 on success or -errno on error.
virtual int Open(const char* path, int flags, mode_t mode) = 0;
- // Wrapper around write. Returns bytes written on success or
- // -errno on error.
- virtual ssize_t Write(const void* bytes, size_t count) = 0;
+ // Wrapper around write. Returns true if all requested bytes
+ // were written, or false on any error, reguardless of progress.
+ virtual bool Write(const void* bytes, size_t count) = 0;
// Wrapper around close. Returns 0 on success or -errno on error.
virtual int Close() = 0;
@@ -47,7 +47,7 @@
virtual ~DirectFileWriter() {}
virtual int Open(const char* path, int flags, mode_t mode);
- virtual ssize_t Write(const void* bytes, size_t count);
+ virtual bool Write(const void* bytes, size_t count);
virtual int Close();
int fd() const { return fd_; }
diff --git a/file_writer_unittest.cc b/file_writer_unittest.cc
index 6e494ec..1b7b1e6 100644
--- a/file_writer_unittest.cc
+++ b/file_writer_unittest.cc
@@ -22,10 +22,10 @@
TEST(FileWriterTest, SimpleTest) {
DirectFileWriter file_writer;
const string path("/tmp/FileWriterTest");
- ASSERT_EQ(0, file_writer.Open(path.c_str(),
+ EXPECT_EQ(0, file_writer.Open(path.c_str(),
O_CREAT | O_LARGEFILE | O_TRUNC | O_WRONLY,
0644));
- ASSERT_EQ(4, file_writer.Write("test", 4));
+ EXPECT_TRUE(file_writer.Write("test", 4));
vector<char> actual_data;
EXPECT_TRUE(utils::ReadFile(path, &actual_data));
@@ -47,7 +47,7 @@
EXPECT_EQ(0, file_writer.Open(path.c_str(),
O_CREAT | O_LARGEFILE | O_TRUNC | O_RDONLY,
0644));
- EXPECT_EQ(-EBADF, file_writer.Write("x", 1));
+ EXPECT_FALSE(file_writer.Write("x", 1));
EXPECT_EQ(0, file_writer.Close());
}
diff --git a/utils.cc b/utils.cc
index 5273347..1c9425a 100644
--- a/utils.cc
+++ b/utils.cc
@@ -89,7 +89,7 @@
O_WRONLY | O_CREAT | O_TRUNC,
0600));
ScopedFileWriterCloser closer(&writer);
- TEST_AND_RETURN_FALSE_ERRNO(data_len == writer.Write(data, data_len));
+ TEST_AND_RETURN_FALSE_ERRNO(writer.Write(data, data_len));
return true;
}
