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 11:42:21 GMT


> On Nov. 4, 2015, 6:08 p.m., Michael Hopcroft wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp,
line 32
> > <https://reviews.apache.org/r/39803/diff/2/?file=1113682#file1113682line32>
> >
> >     I wonder if there is any good way of protecting ourselves from breaking changes
associated with _REPARSE_DATA_BUFFER. Perhaps you should consider setting WINVER in windows.hpp.
Then you could add a static_assert on the value of WINVER in this header.
> >     
> >     http://stackoverflow.com/questions/10112051/c-compile-time-macros-to-detect-windows-os

This is a really interesting suggestion, but because this is in the DDK, I think we would
have to set `NTDDI_VERSION`[1] (which also happens to be in `Sdkddkver.h`).

That said, I don't have the ability to test this on multiple platforms right now, so I added
a JIRA ticket: MESOS-4270.

[1] https://msdn.microsoft.com/en-us/library/windows/hardware/ff554695(v=vs.85).aspx


> On Nov. 4, 2015, 6:08 p.m., Michael Hopcroft wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp,
line 49
> > <https://reviews.apache.org/r/39803/diff/2/?file=1113682#file1113682line49>
> >
> >     Are you sure these buffers of size 1 are correct? I'm assuming that the remainder
of the buffer follows the _REPARSE_DATA_BUFFER in the block of memory.
> >     
> >     I would add a comment explaining this.

The documentation[1] defines this field as:

> First character of the path string. This is followed in memory by the remainder of the
string.

I've added a comment mentioning this.

[1] https://msdn.microsoft.com/en-us/library/windows/hardware/ff552012(v=vs.85).aspx


> On Nov. 4, 2015, 6:08 p.m., Michael Hopcroft wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp,
line 71
> > <https://reviews.apache.org/r/39803/diff/2/?file=1113682#file1113682line71>
> >
> >     Who actually uses this struct. The reason I ask is that it has wstrings inside
instead of strings. Will the user be forced to do the conversion? Perhaps you should be doing
the conversion for them here.

It is purely and specifically for internal APIs only, as a replacement for `REPARSE_DATA_BUFFER`,
which is really inelegant to use. This struct is simple and much easier to reason about.

I've added a comment explaining this.


> On Nov. 4, 2015, 6:08 p.m., Michael Hopcroft wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp,
line 73
> > <https://reviews.apache.org/r/39803/diff/2/?file=1113682#file1113682line73>
> >
> >     Be aware the std::wstring can throw exceptions. If stout has a strong no-exception
guarantee, you should review the code paths to make sure that this exception is caught somewhere
inside of stout.

This datastructure is meant only to be a very simple easy-to-reason-about replacement for
the massively inelegant `REPARSE_DATA_BUFFER`. We don't do any serious computation on any
of the fields, so I expect exceptions to not be an issue here.


> On Nov. 4, 2015, 6:08 p.m., Michael Hopcroft wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp,
line 75
> > <https://reviews.apache.org/r/39803/diff/2/?file=1113682#file1113682line75>
> >
> >     If this struct is part of the API, it should have better documentation. One
cannot know what flags is without reading the implementation in this file.

It's part of the DDK API, yes. I've added a comment to clarify.


> On Nov. 4, 2015, 6:08 p.m., Michael Hopcroft wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp,
line 84
> > <https://reviews.apache.org/r/39803/diff/2/?file=1113682#file1113682line84>
> >
> >     Should check for return value of  INVALID_FILE_ATTRIBUTES or add an explanatory
comment if the code on line 85 correctly handles this case.
> >     
> >     In general, your logic should not make any assumptions on what INVALID_FILE_ATTRIBUTES
actually equals.
> >     
> >     ------
> >     Oh - I see - you check for INVALID_FILE_ATTRIBUTES on line 89. In general line
85 is bad form because reparseBitSet is only valid when attributes != INVALID_FILE_ATTRIBUTES.
Recommend writing code so that reparseBitSet is valid during its entire lifetime.

Good points.


> On Nov. 4, 2015, 6:08 p.m., Michael Hopcroft wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp,
line 104
> > <https://reviews.apache.org/r/39803/diff/2/?file=1113682#file1113682line104>
> >
> >     Divide by sizeof(WCHAR). Same goes for lines 106, 111, and 113.

Good call.


> On Nov. 4, 2015, 6:08 p.m., Michael Hopcroft wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp,
line 147
> > <https://reviews.apache.org/r/39803/diff/2/?file=1113682#file1113682line147>
> >
> >     Could you track down a more definitive source to quote?
> >     
> >     Also, perhaps you could get Microsoft to create a work item for updating MSDN.

I really tried hard to find a better source. I could not find one.


> On Nov. 4, 2015, 6:08 p.m., Michael Hopcroft wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp,
line 199
> > <https://reviews.apache.org/r/39803/diff/2/?file=1113682#file1113682line199>
> >
> >     Recommend using RAII pattern (e.g. std::unique_ptr) to eliminate leaking of
reparsePointData in the presense of exceptions or logic errors.
> >     
> >     Since this is C++ code, recommend using new [] instead of malloc/free.

I am somewhat embarrassed to admit that, being a C++ noob, I need you to double-check that
I got this right: we want to do `new REPARSE_DATA_BUFFER[MAXIMUM_REPARSE_DATA_BUFFER_SIZE]`
to allocate a union of size `MAXIMUM_REPARSE_DATA_BUFFER_SIZE`, right?

I'm keeping this issue open so you can confirm. I was not even aware we could do this in C++,
and after some Googling, I did not find specific confirmation that this is the semantics of
this expression.


> On Nov. 4, 2015, 6:08 p.m., Michael Hopcroft wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/stat.hpp, line 127
> > <https://reviews.apache.org/r/39803/diff/2/?file=1113684#file1113684line127>
> >
> >     It is probably good practice to include information about the site of the error.
First idea is to just adopt a convention of prefixing each method with "functionName:". Better
idea is to modify error.hpp so that ErrnoError() becomes a macro that prepends the file name
and line number.
> >     
> >     This comment applies to all of your functions.

These are copied directly from the POSIX versions of this function. For posterity, we agreed
about the need for this to change and proposed a solution to the dev@ list.


- Alex


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


On Jan. 4, 2016, 11:33 a.m., Alex Clemmer wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39803/
> -----------------------------------------------------------
> 
> (Updated Jan. 4, 2016, 11:33 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 b2dea9b93adfa3ea0415a0b5c81a369dd29b6cfe

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

>   3rdparty/libprocess/3rdparty/stout/include/stout/windows.hpp d46e262e0fd1c2de36f3bf19d8bd693c23bf58cd

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