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

Reply via email to