> 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
> 
>

Reply via email to