This is an automated email from the ASF dual-hosted git repository.
bbannier pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/mesos.git
The following commit(s) were added to refs/heads/master by this push:
new d838f29 Correctly propagated `close` failures in some instances.
d838f29 is described below
commit d838f2958e18c1a75594ca4f10df132670fcd11e
Author: Benjamin Bannier <benjamin.bann...@mesosphere.io>
AuthorDate: Thu Jan 24 10:44:49 2019 +0100
Correctly propagated `close` failures in some instances.
When e.g., writing to disk, errors from write might only manifest at
::close time. We update some instances to correctly propagate these
errors instead of dropping them silently. We only propagate the
::close error if the write operation succeeded, otherwise we just
propagate the error from the write operation.
Review: https://reviews.apache.org/r/69082/
---
3rdparty/stout/include/stout/os/posix/mktemp.hpp | 26 ++++++++--------
3rdparty/stout/include/stout/os/write.hpp | 19 +++++++-----
3rdparty/stout/include/stout/protobuf.hpp | 38 ++++++++++++++----------
3 files changed, 47 insertions(+), 36 deletions(-)
diff --git a/3rdparty/stout/include/stout/os/posix/mktemp.hpp
b/3rdparty/stout/include/stout/os/posix/mktemp.hpp
index 63b3d1a..8dab259 100644
--- a/3rdparty/stout/include/stout/os/posix/mktemp.hpp
+++ b/3rdparty/stout/include/stout/os/posix/mktemp.hpp
@@ -36,23 +36,25 @@ namespace os {
inline Try<std::string> mktemp(
const std::string& path = path::join(os::temp(), "XXXXXX"))
{
- char* temp = new char[path.size() + 1];
- ::memcpy(temp, path.c_str(), path.size() + 1);
+ char* _path = new char[path.size() + 1];
+ ::memcpy(_path, path.c_str(), path.size() + 1);
+
+ int_fd fd = ::mkstemp(_path);
+ Try<std::string> temp = std::string(_path);
+ delete[] _path;
- int_fd fd = ::mkstemp(temp);
if (fd < 0) {
- delete[] temp;
- return ErrnoError();
+ temp = ErrnoError();
}
- // We ignore the return value of close(). This is because users
- // calling this function are interested in the return value of
- // mkstemp(). Also an unsuccessful close() doesn't affect the file.
- os::close(fd);
+ Try<Nothing> close = os::close(fd);
+
+ // We propagate `close` failures if creation of the temp file was successful.
+ if (temp.isSome() && close.isError()) {
+ return Error("Failed to close '" + stringify(fd) + "':" + close.error());
+ }
- std::string result(temp);
- delete[] temp;
- return result;
+ return temp;
}
} // namespace os {
diff --git a/3rdparty/stout/include/stout/os/write.hpp
b/3rdparty/stout/include/stout/os/write.hpp
index f7538f9..89ca7b2 100644
--- a/3rdparty/stout/include/stout/os/write.hpp
+++ b/3rdparty/stout/include/stout/os/write.hpp
@@ -126,21 +126,24 @@ inline Try<Nothing> write(
return Error(fd.error());
}
- Try<Nothing> result = write(fd.get(), message);
+ Try<Nothing> write = os::write(fd.get(), message);
- if (sync && result.isSome()) {
+ if (sync && write.isSome()) {
// We call `fsync()` before closing the file instead of opening it with the
// `O_SYNC` flag for better performance. See:
// http://lkml.iu.edu/hypermail/linux/kernel/0105.3/0353.html
- result = os::fsync(fd.get());
+ write = os::fsync(fd.get());
}
- // We ignore the return value of `close()` because users calling this
function
- // are interested in the return value of `write()`, or `fsync()` if `sync` is
- // set to true. Also an unsuccessful `close()` doesn't affect the write.
- os::close(fd.get());
+ Try<Nothing> close = os::close(fd.get());
- return result;
+ // We propagate `close` failures if `write` on the file was successful.
+ if (write.isSome() && close.isError()) {
+ write =
+ Error("Failed to close '" + stringify(fd.get()) + "':" + close.error());
+ }
+
+ return write;
}
diff --git a/3rdparty/stout/include/stout/protobuf.hpp
b/3rdparty/stout/include/stout/protobuf.hpp
index eb4adef..4b3db7e 100644
--- a/3rdparty/stout/include/stout/protobuf.hpp
+++ b/3rdparty/stout/include/stout/protobuf.hpp
@@ -147,21 +147,24 @@ Try<Nothing> write(const std::string& path, const T& t,
bool sync = false)
return Error("Failed to open file '" + path + "': " + fd.error());
}
- Try<Nothing> result = write(fd.get(), t);
+ Try<Nothing> write = ::protobuf::write(fd.get(), t);
- if (sync && result.isSome()) {
+ if (sync && write.isSome()) {
// We call `fsync()` before closing the file instead of opening it with the
// `O_SYNC` flag for better performance. See:
// http://lkml.iu.edu/hypermail/linux/kernel/0105.3/0353.html
- result = os::fsync(fd.get());
+ write = os::fsync(fd.get());
}
- // We ignore the return value of `close()` because users calling this
function
- // are interested in the return value of `write()`, or `fsync()` if `sync` is
- // set to true. Also an unsuccessful `close()` doesn't affect the write.
- os::close(fd.get());
+ Try<Nothing> close = os::close(fd.get());
- return result;
+ // We propagate `close` failures if `write` on the file was successful.
+ if (write.isSome() && close.isError()) {
+ return Error(
+ "Failed to close '" + stringify(fd.get()) + "':" + close.error());
+ }
+
+ return write;
}
@@ -181,20 +184,23 @@ inline Try<Nothing> append(
return Error("Failed to open file '" + path + "': " + fd.error());
}
- Try<Nothing> result = write(fd.get(), message);
+ Try<Nothing> write = protobuf::write(fd.get(), message);
- if (sync && result.isSome()) {
+ if (sync && write.isSome()) {
// We call `fsync()` before closing the file instead of opening it with the
// `O_SYNC` flag for better performance.
- result = os::fsync(fd.get());
+ write = os::fsync(fd.get());
}
- // NOTE: We ignore the return value of close(). This is because
- // users calling this function are interested in the return value of
- // write(). Also an unsuccessful close() doesn't affect the write.
- os::close(fd.get());
+ Try<Nothing> close = os::close(fd.get());
- return result;
+ // We propagate `close` failures if `write` on the file was successful.
+ if (write.isSome() && close.isError()) {
+ return Error(
+ "Failed to close '" + stringify(fd.get()) + "':" + close.error());
+ }
+
+ return write;
}
