mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Li Li <lilyf...@hotmail.com>
Subject Re: Review Request 57926: Windows: Fix return of bad types in stat.hpp.
Date Fri, 24 Mar 2017 23:32:12 GMT


> On March 24, 2017, 10:18 p.m., Li Li wrote:
> > 3rdparty/stout/include/stout/os/windows/stat.hpp
> > Lines 66 (patched)
> > <https://reviews.apache.org/r/57926/diff/1/?file=1674588#file1674588line66>
> >
> >     At this case, should true or false be returned?
> 
> Andrew Schwartzmeyer wrote:
>     In POSIX systems, a symlink is traditionally a file semantically. On Windows, it
is implemented as a reparse point instead of a file, but this MSDN documentation https://msdn.microsoft.com/en-us/library/windows/desktop/aa365680(v=vs.85).aspx
states:
>     
>     > Symbolic links are designed to aid in migration and application compatibility
with UNIX operating systems. Microsoft has implemented its symbolic links to function just
like UNIX links.
>     
>     So I think it makes sense to maintain the POSIX semantics and state that a symlink
is a file (i.e. return true).
>     
>     That said, I double checked our POSIX implementation, and `isdir` nor `isfile` respectively
compare `S_IFDIR` and `S_IFREG`, so they would both return false (just tested `S_IFREG` as
I wasn't sure). I don't _think_ this is correct semantically, unless we intend `isfile` to
mean "is regular file" (i.e. explicitly not a link).
>     
>     I might have just talked myself into returning false here, but would appreciate further
input.

Let's be consistent with the current isdir and isfile POSIX implementations. To me, isfile
is better to tell us if a link is a real file or not. Feel free to file an issue to track
the ambiguity and make it clear. If we want to change the current meaning of isfile, which
means "is regular file", both places(windows and linux) should be changed together later.


- Li


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57926/#review170059
-----------------------------------------------------------


On March 24, 2017, 10:14 p.m., Andrew Schwartzmeyer wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57926/
> -----------------------------------------------------------
> 
> (Updated March 24, 2017, 10:14 p.m.)
> 
> 
> Review request for mesos, John Kordich, James Peach, Joseph Wu, Li Li, and Michael Park.
> 
> 
> Bugs: MESOS-7307
>     https://issues.apache.org/jira/browse/MESOS-7307
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Commit 5f159cdcb introduced `return Error(...)` logic to functions
> which return `bool`, not `Try<bool>`, which broke the Windows build.
> 
> Furthermore, in the instances of `isdir` and `isfile`, erroring when
> asked to not follow a symlink is not correct. The semantics of symlinks
> provide clear answers to `isdir` and `isfile` when the target is a link,
> and is not being followed (it is a file, but not a directory).
> 
> For the functions `mode` and `dev`, which return types wrapped by `Try`,
> we should only error if asked not to follow symlinks, and the target is
> actually a symlink. If it is not a symlink to begin with, we should not
> prematurely error. If it is a symlink, we should error because there is
> no equivalent of `lstat` on Windows to obtain `st_mode` or `st_dev` of a
> symlink itself.
> 
> 
> Diffs
> -----
> 
>   3rdparty/stout/include/stout/os/windows/stat.hpp 8587341282ca2d596a2b6f23f84b84a00053c3d5

> 
> 
> Diff: https://reviews.apache.org/r/57926/diff/1/
> 
> 
> Testing
> -------
> 
> Build on Windows and run stout-tests.exe
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message