mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alex Clemmer" <clemmer.alexan...@gmail.com>
Subject Re: Review Request 39803: Windows: Implemented stout/os/stat.hpp`.
Date Mon, 04 Jan 2016 07:49:37 GMT


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


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