> On Nov. 3, 2017, 11:29 a.m., Benjamin Mahler wrote: > > 3rdparty/stout/include/stout/os/windows/copyfile.hpp > > Lines 56-57 (patched) > > <https://reviews.apache.org/r/60621/diff/6/?file=1864337#file1864337line56> > > > > The general pattern is to just include the reason for an error, and to > > not include any information the caller already has, because otherwise the > > callers will double log: > > > > ``` > > Try<Nothing> copy = copyfile(source, destination); > > > > if (copy.isError()) { > > return ("Failed to copy '" + source + "'" > > " to '" + destination + "': " + copy.error(); > > } > > ``` > > > > The current code would log: > > > > "Failed to copy 's' to 'd': Failed to copy 's' to 'd': No disk space > > left" > > > > Can you guys do a sweep for this issue in the windows related code that > > has been added? > > Andrew Schwartzmeyer wrote: > I'm not sure I follow. Are you saying the _caller_ should always write > the actual error message, versus the _callee_ preparing it here? > > I guess in your example, I don't get why the user of `os::copyfile` would > add `"Failed to copy..."` instead of just doing: > > ``` > if (copy.isError()) { > return Error(copy.error()); > } > ``` > > And then the error message is only written once, in `os::copyfile`. > > Benjamin Mahler wrote: > What is the "actual" error message? :) > > An error message consists of several parts, much like an exception: the > "reason" for the error, and multiple "stacks" of context. If you're referring > to the "reason" when you said "actual", either approach (the one we use, or > the one used in this patch) includes the reason in their returned error > message. The distinction lies in where the "stacks" of context get included. > > The decision we took some time ago was to have the "owner" of the context > be responsible for including it. So if I call `os::copyfile` I know which > function I'm calling and which source and destination I'm passing into it. > This matches posix-style programming which I why I think we chose this > approach: > > ``` > int main() > { > int fd = open("/file"); > > if (fd == -1) { > // Caller logs the thing it was doing, and gets the reason for the > error: > LOG(ERROR) << "Failed to initialize: Failed to open '/file': " << > strerror(errno); > } > } > > vs > > int main() > { > Try<int> fd = open("/file"); > > if (fd.isError()) { > // Caller logs the thing it was doing, and gets the reason for the > error: > LOG(ERROR) << "Failed to initialize: Failed to open '/file': " << > fd.error(); > } > } > ``` > > Now of course, if you use the alternative approach to have the leaf > include all the information it has, then you have to compose differently: > > ``` > int main() > { > Try<int> fd = os::open("/file"); > > if (fd.isError()) { > // Caller knows that no additional context needs to be added because > callee has all of it. > LOG(ERROR) << "Failed to initialize: " << fd.error(); > } > } > ``` > > The approach we chose was to treat the error as just the "reason" (like > strerror), so if you want to add context to it you can. > > Both approaches work, but we have to pick one and apply it consistently > as best we can. In retrospect, I actually think the other approach would have > been a better choice because it fits more easily into Future::then style > composition. But we would have to discuss the change of approach and do a > sweep, because most of the code follows the first pattern shown.
Ah, thank you for the detailed explanation. Unfortunately, the existing error handling code was using the leaf approach, and we kept it consistent, but consistently wrong. We'll have to do a sweep to fix it. This explanation was really good, do we have something like it documented? If not, let's get it added to the style guide. - Andrew ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60621/#review190043 ----------------------------------------------------------- On Oct. 19, 2017, 2:32 p.m., Jeff Coffler wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60621/ > ----------------------------------------------------------- > > (Updated Oct. 19, 2017, 2:32 p.m.) > > > Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, and > Li Li. > > > Bugs: MESOS-6705 > https://issues.apache.org/jira/browse/MESOS-6705 > > > Repository: mesos > > > Description > ------- > > The os::copyfile() method provides an API to copy a file from one > location to another. Note that copying of directories are not > supported, and that the destination directory must exist before a > file can be copied into a directory. > > > Diffs > ----- > > 3rdparty/stout/Makefile.am 4386017acd6ca465be3f735460c11d50b440ccc5 > 3rdparty/stout/include/Makefile.am bdd3e9908ebfc682458a3babc34cbee36ad3f751 > 3rdparty/stout/include/stout/os/copyfile.hpp PRE-CREATION > 3rdparty/stout/include/stout/os/posix/copyfile.hpp PRE-CREATION > 3rdparty/stout/include/stout/os/windows/copyfile.hpp PRE-CREATION > 3rdparty/stout/tests/CMakeLists.txt > 6e5773f1e03671de7ac007caf49edd0f1cd7aedd > 3rdparty/stout/tests/os/copyfile_tests.cpp PRE-CREATION > > > Diff: https://reviews.apache.org/r/60621/diff/6/ > > > Testing > ------- > > See upstream > > Note that Joe made some changes to this, I ended up taking his changes as is. > > > Thanks, > > Jeff Coffler > >
