You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mesos.apache.org by be...@apache.org on 2013/01/31 06:04:09 UTC
svn commit: r1440846 - in /incubator/mesos/trunk: src/master/master.cpp
src/slave/state.cpp src/tests/stout_tests.cpp
third_party/libprocess/include/stout/os.hpp
Author: benh
Date: Thu Jan 31 05:04:09 2013
New Revision: 1440846
URL: http://svn.apache.org/viewvc?rev=1440846&view=rev
Log:
Fixed of os::read/write to be POSIX compliant (handle 0, EINTR, and
partial reads/writes).
From: Ben Mahler <be...@gmail.com>
Review: https://reviews.apache.org/r/8694
Modified:
incubator/mesos/trunk/src/master/master.cpp
incubator/mesos/trunk/src/slave/state.cpp
incubator/mesos/trunk/src/tests/stout_tests.cpp
incubator/mesos/trunk/third_party/libprocess/include/stout/os.hpp
Modified: incubator/mesos/trunk/src/master/master.cpp
URL: http://svn.apache.org/viewvc/incubator/mesos/trunk/src/master/master.cpp?rev=1440846&r1=1440845&r2=1440846&view=diff
==============================================================================
--- incubator/mesos/trunk/src/master/master.cpp (original)
+++ incubator/mesos/trunk/src/master/master.cpp Thu Jan 31 05:04:09 2013
@@ -85,18 +85,18 @@ protected:
// TODO(vinod): Ensure this read is atomic w.r.t external
// writes/updates to this file.
- Result<string> result = os::read(path.substr(strlen("file://")));
- if (result.isError()) {
- LOG(ERROR) << "Error reading whitelist file "
- << result.error() << ". Retrying";
+ Try<string> read = os::read(path.substr(strlen("file://")));
+ if (read.isError()) {
+ LOG(ERROR) << "Error reading whitelist file: " << read.error() << ". "
+ << "Retrying";
whitelist = lastWhitelist;
- } else if (result.isNone()) {
- LOG(WARNING) << "Empty whitelist file "
- << path << ". No offers will be made!";
+ } else if (read.get().empty()) {
+ LOG(WARNING) << "Empty whitelist file " << path << ". "
+ << "No offers will be made!";
whitelist = Option<hashset<string> >::some(hashset<string>());
} else {
hashset<string> hostnames;
- vector<string> lines = strings::tokenize(result.get(), "\n");
+ vector<string> lines = strings::tokenize(read.get(), "\n");
foreach (const string& hostname, lines) {
hostnames.insert(hostname);
}
Modified: incubator/mesos/trunk/src/slave/state.cpp
URL: http://svn.apache.org/viewvc/incubator/mesos/trunk/src/slave/state.cpp?rev=1440846&r1=1440845&r2=1440846&view=diff
==============================================================================
--- incubator/mesos/trunk/src/slave/state.cpp (original)
+++ incubator/mesos/trunk/src/slave/state.cpp Thu Jan 31 05:04:09 2013
@@ -139,19 +139,19 @@ SlaveID readSlaveID(const string& rootDi
{
const string& path = paths::getSlaveIDPath(rootDir);
- Result<string> result = os::read(path);
+ Try<string> read = os::read(path);
SlaveID slaveId;
- if (!result.isSome()) {
- LOG(WARNING) << "Cannot read slave id from " << path << " because "
- << result.isError() ? result.error() : "empty";
+ if (!read.isSome()) {
+ LOG(WARNING) << "Cannot read slave id from " << path << ": "
+ << read.error();
return slaveId;
}
- LOG(INFO) << "Read slave id " << result.get() << " from " << path;
+ LOG(INFO) << "Read slave id " << read.get() << " from " << path;
- slaveId.set_value(result.get());
+ slaveId.set_value(read.get());
return slaveId;
}
@@ -184,17 +184,17 @@ process::UPID readFrameworkPID(const str
const string& path = paths::getFrameworkPIDPath(metaRootDir, slaveId,
frameworkId);
- Result<string> result = os::read(path);
+ Try<string> read = os::read(path);
- if (!result.isSome()) {
- LOG(WARNING) << "Cannot read framework pid from " << path << " because "
- << result.isError() ? result.error() : "empty";
+ if (!read.isSome()) {
+ LOG(WARNING) << "Cannot read framework pid from " << path << ": "
+ << read.error();
return process::UPID();
}
- LOG(INFO) << "Read framework pid " << result.get() << " from " << path;
+ LOG(INFO) << "Read framework pid " << read.get() << " from " << path;
- return process::UPID(result.get());
+ return process::UPID(read.get());
}
} // namespace state {
Modified: incubator/mesos/trunk/src/tests/stout_tests.cpp
URL: http://svn.apache.org/viewvc/incubator/mesos/trunk/src/tests/stout_tests.cpp?rev=1440846&r1=1440845&r2=1440846&view=diff
==============================================================================
--- incubator/mesos/trunk/src/tests/stout_tests.cpp (original)
+++ incubator/mesos/trunk/src/tests/stout_tests.cpp Thu Jan 31 05:04:09 2013
@@ -395,7 +395,7 @@ TEST_F(StoutUtilsTest, readWriteString)
ASSERT_SOME(os::write(testfile, teststr));
- Result<std::string> readstr = os::read(testfile);
+ Try<std::string> readstr = os::read(testfile);
ASSERT_SOME(readstr);
EXPECT_EQ(teststr, readstr.get());
Modified: incubator/mesos/trunk/third_party/libprocess/include/stout/os.hpp
URL: http://svn.apache.org/viewvc/incubator/mesos/trunk/third_party/libprocess/include/stout/os.hpp?rev=1440846&r1=1440845&r2=1440846&view=diff
==============================================================================
--- incubator/mesos/trunk/third_party/libprocess/include/stout/os.hpp (original)
+++ incubator/mesos/trunk/third_party/libprocess/include/stout/os.hpp Thu Jan 31 05:04:09 2013
@@ -208,16 +208,22 @@ inline Try<std::string> mktemp(const std
// Write out the string to the file at the current fd position.
inline Try<Nothing> write(int fd, const std::string& message)
{
- ssize_t length = ::write(fd, message.data(), message.length());
+ size_t offset = 0;
- if (length == -1) {
- // TODO(bmahler): Handle EINTR by retrying.
- return Try<Nothing>::error(strerror(errno));
- }
+ while (offset < message.length()) {
+ ssize_t length =
+ ::write(fd, message.data() + offset, message.length() - offset);
+
+ if (length < 0) {
+ // TODO(benh): Handle a non-blocking fd? (EAGAIN, EWOULDBLOCK)
+ if (errno == EINTR) {
+ continue;
+ }
+ return Try<Nothing>::error(strerror(errno));
+ }
- CHECK(length > 0);
- // TODO(bmahler): Handle partial writes with a write loop.
- CHECK(static_cast<size_t>(length) == message.length());
+ offset += length;
+ }
return Nothing();
}
@@ -246,61 +252,93 @@ inline Try<Nothing> write(const std::str
}
-// Read the contents of the file from its current offset
-// and return it as a string.
-// TODO(bmahler): Change this to a Try<std::string> since none()
-// is equivalent to an empty string.
-inline Result<std::string> read(int fd)
-{
- // Get the size of the file.
- off_t offset = lseek(fd, 0, SEEK_CUR);
- if (offset == -1) {
- return Result<std::string>::error("Error seeking to SEEK_CUR");
+// Reads 'size' bytes from a file from its current offset.
+// If EOF is encountered before reading size bytes, then the offset
+// is restored and none is returned.
+inline Result<std::string> read(int fd, size_t size)
+{
+ // Save the current offset.
+ off_t current = lseek(fd, 0, SEEK_CUR);
+ if (current == -1) {
+ return Result<std::string>::error(
+ "Failed to lseek to SEEK_CUR: " + std::string(strerror(errno)));
}
- off_t size = lseek(fd, 0, SEEK_END);
- if (size == -1) {
- return Result<std::string>::error("Error seeking to SEEK_END");
- }
+ char* buffer = new char[size];
+ size_t offset = 0;
- if (lseek(fd, offset, SEEK_SET) == -1) {
- return Result<std::string>::error("Error seeking to SEEK_SET");
+ while (offset < size) {
+ ssize_t length = ::read(fd, buffer + offset, size - offset);
+
+ if (length < 0) {
+ // TODO(bmahler): Handle a non-blocking fd? (EAGAIN, EWOULDBLOCK)
+ if (errno == EINTR) {
+ continue;
+ }
+ // Attempt to restore the original offset.
+ lseek(fd, current, SEEK_SET);
+ return Result<std::string>::error(strerror(errno));
+ } else if (length == 0) {
+ // Reached EOF before expected! Restore the offset.
+ lseek(fd, current, SEEK_SET);
+ return Result<std::string>::none();
+ }
+
+ offset += length;
}
- // Allocate memory.
- char* buffer = new char[size];
+ return std::string(buffer, size);
+}
- ssize_t length = ::read(fd, buffer, size);
- if (length == 0) {
- return Result<std::string>::none();
- } else if (length == -1) {
- // TODO(bmahler): Handle EINTR by retrying.
- // Save the error, reset the file offset, and return the error.
- return Result<std::string>::error(strerror(errno));
- } else if (length != size) {
- // TODO(bmahler): Handle partial reads with a read loop.
- return Result<std::string>::error("Couldn't read the entire file");
+// Returns the contents of the file starting from its current offset.
+// If an error occurs, this will attempt to recover the file offset.
+inline Try<std::string> read(int fd)
+{
+ // Save the current offset.
+ off_t current = lseek(fd, 0, SEEK_CUR);
+ if (current == -1) {
+ return Try<std::string>::error(
+ "Failed to lseek to SEEK_CUR: " + std::string(strerror(errno)));
}
- std::string result(buffer, size);
+ // Get the size of the file from the offset.
+ off_t size = lseek(fd, current, SEEK_END);
+ if (size == -1) {
+ return Try<std::string>::error(
+ "Failed to lseek to SEEK_END: " + std::string(strerror(errno)));
+ }
- return result;
+ // Restore the offset.
+ if (lseek(fd, current, SEEK_SET) == -1) {
+ return Try<std::string>::error(
+ "Failed to lseek with SEEK_SET: " + std::string(strerror(errno)));
+ }
+
+ Result<std::string> result = read(fd, size);
+ if (result.isNone()) {
+ // Hit EOF before reading size bytes.
+ return Try<std::string>::error("The file size was modified while reading");
+ } else if (result.isError()) {
+ return Try<std::string>::error(result.error());
+ }
+
+ return result.get();
}
// A wrapper function that wraps the above read() with
// open and closing the file.
-inline Result<std::string> read(const std::string& path)
+inline Try<std::string> read(const std::string& path)
{
Try<int> fd = os::open(path, O_RDONLY,
S_IRUSR | S_IWUSR | S_IRGRP | S_IRWXO);
if (fd.isError()) {
- return Result<std::string>::error("Failed to open file " + path);
+ return Try<std::string>::error("Failed to open file " + path);
}
- Result<std::string> result = read(fd.get());
+ Try<std::string> result = read(fd.get());
// NOTE: We ignore the return value of close(). This is because users calling
// this function are interested in the return value of read(). Also an