Re: Review Request 39803: Windows: Implemented stout/os/stat.hpp`.

[Alex Clemmer <cl...@gmail.com>]

> On Nov. 4, 2015, 1:02 a.m., Michael Hopcroft wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/symlink.hpp, line 14
> > <https://reviews.apache.org/r/39803/diff/2/?file=1113683#file1113683line14>
> >
> >     Recommend #pragma once. This is supported by VS. GCC supports it since version 3.4 (https://en.wikipedia.org/wiki/Pragma_once), but this shouldn't matter since we're discussing a Windows file. In general #pragma once is preferred over the #ifndef include guard mechanism, so the main question would pertain to consistency with the rest of the codebase. My recommendation is to use #pragma once for all new code and gradually migrate old code as it is edited.
> >     
> >     I notice that the Google Style Guide recommends the #ifndef version. This may be a factor in deciding.

Per our proposal to the dev@ list, let's defer this until we're in SF with the Mesosphere folks, and we can just sweep over the entire codebase and do them all at once.


> On Nov. 4, 2015, 1:02 a.m., Michael Hopcroft wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/symlink.hpp, line 64
> > <https://reviews.apache.org/r/39803/diff/2/?file=1113683#file1113683line64>
> >
> >     Google Style Guide isn't really clear on this one. I would recommend indenting line 66 to align with the open paren on line 65.
> 
> Joseph Wu wrote:
>     Generally, we would go for this:
>     ```
>     return Error(
>         "Reparse point attribute is not set for path '" +
>         absolutePath.get() + "', and therefore it is not a sybolic link");
>     ```
>     Note: no period at the end of the message.

I agree with Joseph, as that does seem to be the style we use in Stout.


> On Nov. 4, 2015, 1:02 a.m., Michael Hopcroft wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/symlink.hpp, line 55
> > <https://reviews.apache.org/r/39803/diff/2/?file=1113683#file1113683line55>
> >
> >     Google Style Guide allows auto if it help readability? Is it a good or bad idea here?
> 
> Joseph Wu wrote:
>     In general, we don't use `auto` unless it's completely obvious from the right-side what the type is.  In this case, you might expect `os::realpath` to return a `std::string`, `Try<std::string>`, `Option<str::string>`, `Path`, etc.

Joseph is correct. I think we should keep it as is.


> On Nov. 4, 2015, 1:02 a.m., Michael Hopcroft wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/symlink.hpp, line 69
> > <https://reviews.apache.org/r/39803/diff/2/?file=1113683#file1113683line69>
> >
> >     I'd put a shorter comment here, something like "Open `HANDLE` for the symlink isself." In general I feel it is error prone to include a verbatem copy of the function's documentation here as it won't get updated, but could be confused for the function's documentation.

I chose to remove this and instead rename the variable to make it clear what the `HANDLE` points at.


> On Nov. 4, 2015, 1:02 a.m., Michael Hopcroft wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/symlink.hpp, line 81
> > <https://reviews.apache.org/r/39803/diff/2/?file=1113683#file1113683line81>
> >
> >     Is it possible to leak a handle in the presence of an error? I guess if getSymbolicLinkData() can never throw you will always execute line 81.
> >     
> >     Still, I would rather see RAII pattern involving any code with handles, just to be safe. This also future proofs the code against edits that make the logic more complex.

In this revision, I propose making a `typedef shared_ptr<void> shared_handle` as a stand-in for `HANDLE`. Semantically, it's just a reference-counted `HANDLE`. It's slow but we don't care for this code path.


> On Nov. 4, 2015, 1:02 a.m., Michael Hopcroft wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/stat.hpp, line 64
> > <https://reviews.apache.org/r/39803/diff/2/?file=1113684#file1113684line64>
> >
> >     If you always return symlink.isSome(), then why use Try<>? Is it that other users of querySymbolicLinkData() actually care about getting an error back?
> >     
> >     Even if you keep Try<> in the API, is it ok to silently ignore an error?

Yep, it's a `Try` because other places we call `querySymbolicLinkData` care about the error.

Whether it is ok to silently ignore an error depends on what you're trying to do. In this case, if for any reason we are unable to retrieve information about the symlink (via the `Symlink` struct), then we report that it is not a symlink. So it is reasonable to ignore the error, because we don't care what the error was.

Right?


> On Nov. 4, 2015, 1:02 a.m., Michael Hopcroft wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/stat.hpp, line 77
> > <https://reviews.apache.org/r/39803/diff/2/?file=1113684#file1113684line77>
> >
> >     When you say the size of the file system entry, I assume you mean the size of the file pointed to, not the number of bytes in the file system record. Might want to reword the comment to make it more clear.
> >     
> >     Also, what happens if the path is a directory?

The short answer is that calling `stat` or `_stat` on a path pointing at a directory will return a stat object that (among other things) reports the number of bytes allocated on disk to represent that directory. This convention comes from the POSIX spec.


[1] https://en.wikipedia.org/wiki/Unix_file_types
[2] http://pubs.opengroup.org/onlinepubs/009695399/basedefs/sys/stat.h.html


> On Nov. 4, 2015, 1:02 a.m., Michael Hopcroft wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/windows.hpp, line 94
> > <https://reviews.apache.org/r/39803/diff/2/?file=1113685#file1113685line94>
> >
> >     Why make these macros instead of functions?
> >     
> >     Also, shouldn't these be in a header under internal? Why does anyone else outside of stout need to see them?

We made them macros just because they originally were macros. There isn't a compelling reason to not make them functions, though.

The header should be under internal, but as a project we tend to be against moving things and changing them at the same time. So let's split that into a different review.


> On Nov. 4, 2015, 1:02 a.m., Michael Hopcroft wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/symlink.hpp, line 22
> > <https://reviews.apache.org/r/39803/diff/2/?file=1113683#file1113683line22>
> >
> >     Not sure how lines 22 and 24 fit with the #include file ordering scheme.

Historically we alphabetize by namespace, excepting the `#ifdef`'d windows includes, which historically go at the bottom. Initially I had though that maybe this should apply to all the Windows stuff, but I've changed my mind and reversed the order, so that `internal/windows/reparsepoint.hpp` comes first.


> On Nov. 4, 2015, 1:02 a.m., Michael Hopcroft wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/symlink.hpp, line 61
> > <https://reviews.apache.org/r/39803/diff/2/?file=1113683#file1113683line61>
> >
> >     Recommend opening a work item for Windows team to add this API. Who knows what version of Windows would actually ship it, but the world would be a slightly better place.
> >     
> >     Also wonder if you should add your code to a StackOverflow answer.

Who do you think we should contact about this?


> On Nov. 4, 2015, 1:02 a.m., Michael Hopcroft wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/stat.hpp, line 40
> > <https://reviews.apache.org/r/39803/diff/2/?file=1113684#file1113684line40>
> >
> >     Might add a comment explaining that when ::stat() returns a value less than zero the error can only be ENOENT or EINVAL. ENOENT means the path doesn't exist, so returning false is correct. EINVAL should be impossible, based on inspection of the code.

Just so we're all on the same page, let me state my understanding. Unless I'm reading the POSIX spec[1] and the Windows documentation[2], there isn't an error case where we'd want to return true, right? So it's not clear to me that adding a comment actually helps understanding. The argument I can see in favor of adding the comment is that the Windows implementation returns only two types of error codes, which is definitely less meaningful than the POSIX spec, but on balance, if the POSIX spec returns many more error codes (minus the weird Windows use of `EINVAL`), and it's still clear to readers, than maybe that means we don't need the comment.

What do you think?


[1] http://pubs.opengroup.org/onlinepubs/009695399/functions/stat.html
[2] https://msdn.microsoft.com/en-us/library/14h5k7ff.aspx


> On Nov. 4, 2015, 1:02 a.m., Michael Hopcroft wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/windows.hpp, line 95
> > <https://reviews.apache.org/r/39803/diff/2/?file=1113685#file1113685line95>
> >
> >     Are you sure this is the correct meaning of S_IFREG? Is it possible for both S_IFDIR and S_IFREG to be set? The windows documentation states, "the _S_IFREG bit is set if path specifies an ordinary file or a device." What happens if the path is "c:\". This is a device and also a directory.

I believe that `S_IFREG` is meant to return true only if it's a _file_ (as in a regular file or a device file). So, unless I'm misreading the documentaiton, `S_IFDIR` and `S_IFREG` can't both be set, because something is either a file or a directory.

Hence, `C:\` is considered a directory, not a regular file or device file.


- Alex


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


On Nov. 16, 2015, 9:14 a.m., Alex Clemmer wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39803/
> -----------------------------------------------------------
> 
> (Updated Nov. 16, 2015, 9:14 a.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Michael Hopcroft, Joris Van Remoortere, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Windows: Implemented stout/os/stat.hpp`.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/3rdparty/stout/include/Makefile.am a8c35c086ecae21701f6a720f25231c1b0d4e329 
>   3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp PRE-CREATION 
>   3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/symlink.hpp PRE-CREATION 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/stat.hpp 675b2e712358a55b3580026936890eaf80e5af71 
>   3rdparty/libprocess/3rdparty/stout/include/stout/windows.hpp 1a7037d64afeedc340258c92067e95d1d3caa027 
> 
> Diff: https://reviews.apache.org/r/39803/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
> 
>