> On Dec. 15, 2015, 3:54 a.m., Alex Naparu wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/rmdir.hpp, line > > 39 > > <https://reviews.apache.org/r/39584/diff/7/?file=1125983#file1125983line39> > > > > What if the path ends with multiple '\' chars to begin with?
Good question. In general, our externally-facing API (like `os::rmdir`) will take any path and normalize that before processing, but our internal functions assume that paths are already normalized. Practically speaking, this simplifies our code quite a bit, and it's an easy rule to remember. In this case, `os::realpath` will strip the trailing extraneous slashes from the filename (I've verified this), and since `os::rmdir` preforms this normalization before `recursiveRemoveDirectory` is called, I believe this code is correct. So the short answer is, "we should never encounter such a situation", assuming there is no normalized path with multiple > On Dec. 15, 2015, 3:54 a.m., Alex Naparu wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/rmdir.hpp, line > > 47 > > <https://reviews.apache.org/r/39584/diff/7/?file=1125983#file1125983line47> > > > > What happens if the input path already contains a wildcard? Might be a > > good idea to check that "path" is a directory before moving forward. Also > > might be a good idea to use realpath here. The short answer is that paths end with a star yield correct behavior -- _i.e._, it will treat the path ending in a wildcard as an absolute path, and since that path doesn't exist, it will then return an error. So, a call to `isdir` is not necessary here, unless I'm missing something else. The longer answer is: we assume the path passed into the function is a directory, and append either a `*` or a `*` to the end of the path (depending on whether it already has a trailing `\ `), and ask `FindFirstFile` to find the first file that matches that pattern. Since this will result in a path like `C:\example**`, clearly this is not a directory, and the Windows API it will fail to find any directory entries matching the pattern, returning an error. For the normalization issue, as noted above, we do assume that the path is normalized when passed into this function. > On Dec. 15, 2015, 3:54 a.m., Alex Naparu wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/rmdir.hpp, line > > 62 > > <https://reviews.apache.org/r/39584/diff/7/?file=1125983#file1125983line62> > > > > Nit: tou're not reusing these, so might as well inline the callse. I'd like to keep these actually, unless you really are opposed to them. I chose them because they make the code self-documenting. > On Dec. 15, 2015, 3:54 a.m., Alex Naparu wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/rmdir.hpp, line > > 73 > > <https://reviews.apache.org/r/39584/diff/7/?file=1125983#file1125983line73> > > > > Attributes should already be available in WIN32_FIND_DATA (see > > https://msdn.microsoft.com/en-us/library/windows/desktop/aa365740(v=vs.85).aspx), > > you might not need to call GetFileAttribiutes here. If you do need to get > > the attributes, consider using CreateFile and GetFileInformationByHandle, > > which will always give the up-to-date information even on NTFS (see > > https://msdn.microsoft.com/en-us/library/windows/desktop/aa364952(v=vs.85).aspx > > for details) This code can probably actually transition to `stat::isdir`; it wasn't written to use that code because I hadn't written that function yet. Now that I have, we can reverse the order of these commits, and simply use that function. > On Dec. 15, 2015, 3:54 a.m., Alex Naparu wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/rmdir.hpp, line > > 77 > > <https://reviews.apache.org/r/39584/diff/7/?file=1125983#file1125983line77> > > > > Consider using a smart pointer for closing the handle. Per our conversation some months ago, we'll transition to a new type, `shared_handle`, which is a typedef to `shared_ptr<void>`. This looks the same but avoids returning pointer to void everywhere, which is gross. > On Dec. 15, 2015, 3:54 a.m., Alex Naparu wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/rmdir.hpp, line > > 97 > > <https://reviews.apache.org/r/39584/diff/7/?file=1125983#file1125983line97> > > > > Failure of ::remove alone might not be reason enough for aborting the > > entire call. For instance, if ::remove fails with "file not found", we're > > still good to go. I believe the POSIX spec actually says that you should stop after you fail to delete one of the things you tried to delete. So I think that we should keep this behavior, unless you think I missed something else? > On Dec. 15, 2015, 3:54 a.m., Alex Naparu wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/rmdir.hpp, line > > 138 > > <https://reviews.apache.org/r/39584/diff/7/?file=1125983#file1125983line138> > > > > Consider using ::RemoveDirectory here, which will delete the directory > > when the last handle is closed. Unless that's not the desired behavior... I believe `rmdir` does the same thing, actually. In fact, I remember being unaware of this fact until I spent a coule hours trying to debug why things weren't deleting. - Alex ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39584/#review110385 ----------------------------------------------------------- On Nov. 16, 2015, 9:13 a.m., Alex Clemmer wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39584/ > ----------------------------------------------------------- > > (Updated Nov. 16, 2015, 9:13 a.m.) > > > Review request for mesos, Artem Harutyunyan, Joris Van Remoortere, and Joseph > Wu. > > > Repository: mesos > > > Description > ------- > > Windows: Implemented `os::rmdir.hpp`. > > > Diffs > ----- > > 3rdparty/libprocess/3rdparty/stout/include/Makefile.am > a8c35c086ecae21701f6a720f25231c1b0d4e329 > 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp > 5c1df81193b4b888d2ed5c7dbfa0b5e2fae48467 > 3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/rmdir.hpp > PRE-CREATION > 3rdparty/libprocess/3rdparty/stout/include/stout/os/rmdir.hpp PRE-CREATION > 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/rmdir.hpp > PRE-CREATION > 3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp > e26df59d9b837e1f4a4b92577f0a3de4b9076cb4 > 3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp > edf17d5ad8efbc988e909bfb8ffa5a015ecdc89d > > Diff: https://reviews.apache.org/r/39584/diff/ > > > Testing > ------- > > `make check` from autotools on Ubuntu 15. > `make check` from CMake on OS X 10.10. > Ran `check` project in VS on Windows 10. > > > Thanks, > > Alex Clemmer > >
