> On Nov. 3, 2017, 6:29 p.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. > > Andrew Schwartzmeyer wrote: > 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.
This is a general issue all over the place. Rather than fix that here, I think this should be covered in a sweep of the code where we fix everything in one shot. So I'll drop this for now, but we will plan on doing a sweep to fix up a bunch of things like this. - Jeff ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60621/#review190043 ----------------------------------------------------------- On Nov. 6, 2017, 6:08 p.m., Jeff Coffler wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60621/ > ----------------------------------------------------------- > > (Updated Nov. 6, 2017, 6:08 p.m.) > > > Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, Li > Li, and Michael Park. > > > 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 > >
