> On Feb. 25, 2019, 6:06 p.m., Benjamin Mahler wrote: > > 3rdparty/stout/include/stout/os/posix/mktemp.hpp > > Line 47 (original), 47 (patched) > > <https://reviews.apache.org/r/70046/diff/1/?file=2126560#file2126560line47> > > > > Can we just simplify this by returning early? > > > > ``` > > if (fd < 0) { > > return ErrnoError(); > > } > > > > Try<Nothing> close = os::close(fd); > > > > // We propagate `close` failures if creation of the temp file was > > successful. > > if (close.isError()) { > > return Error("Failed to close '" + stringify(fd) + "':" + > > close.error()); > > } > > > > return temp; > > ``` > > > > Seems easier to read as being correct too? > > > > Also, this lets `temp` become just `std::string` without the `Try`.
Much nicer solution, agreed! I posted an additional cleanup as https://reviews.apache.org/r/70053/. - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70046/#review213170 ----------------------------------------------------------- On Feb. 25, 2019, 10:19 p.m., Benjamin Bannier wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70046/ > ----------------------------------------------------------- > > (Updated Feb. 25, 2019, 10:19 p.m.) > > > Review request for mesos, Benjamin Mahler and Chun-Hung Hsiao. > > > Repository: mesos > > > Description > ------- > > During the refactoring of `d838f2958e` we reorganized the code flow to > propagate failures from `os::close` and introduce code which migh > close an invalid file descriptor. While this would produce an `Error` > result from `mktemp` in any case, it would have lead to confusing > error messages in that scenario. This patch prevents closing invalid > file descriptors altogether which is consistent with the > pre-refactoring behavior. > > > Diffs > ----- > > 3rdparty/stout/include/stout/os/posix/mktemp.hpp > 8dab2599f13c3e1dab109423c8a938ec16540aaf > > > Diff: https://reviews.apache.org/r/70046/diff/2/ > > > Testing > ------- > > `make check` > > > Thanks, > > Benjamin Bannier > >
