Advertising

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