You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Andrew Schwartzmeyer <an...@schwartzmeyer.com> on 2016/12/05 17:38:13 UTC
Review Request 54335: Add os::var() to stout.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54335/
-----------------------------------------------------------
Review request for mesos and Alex Clemmer.
Bugs: MESOS-6677
https://issues.apache.org/jira/browse/MESOS-6677
Repository: mesos
Description
-------
Returns `/var` on POSIX and (usually) `C:\ProgramData` on Windows.
Uses Windows API to look up correct location for persistent,
app-local variable data. Returns standard location on POSIX.
Diffs
-----
3rdparty/stout/include/stout/posix/os.hpp c37e64db662ba3cee83d2f55de0f9d71ad72c038
3rdparty/stout/include/stout/windows/os.hpp de9b04ad82443038a0f4408bc72cae1540a1beaf
Diff: https://reviews.apache.org/r/54335/diff/
Testing
-------
Thanks,
Andrew Schwartzmeyer
Re: Review Request 54335: Add os::var() to stout.
Posted by Alex Clemmer <cl...@gmail.com>.
> On Dec. 5, 2016, 6:31 p.m., Jie Yu wrote:
> > Flying by. I am checking https://www.gnu.org/prep/standards/html_node/Directory-Variables.html
> >
> > Looks like, in retrospect, we should call the current `runtime_dir` `runstate_dir` instead. So we probably should use `os::runstatedir`?
>
> Alex Clemmer wrote:
> +1, thanks for the helpful suggestion Jie. We were debating what to call this anyway. :)
But, actually, I think I spoke too soon. The idea is actually to use this for all the places we use a directory rooted at `/var`, _i.e._, for all places we're dealing with variable data. I think the final picture of what the disk isolators and persistent volumes stuff will end up looking like is not yet fully developed, but the idea here is that we will want all of the places those things manage variable data to be managed out of a sensible `/var` on Windows, too. This is also why I think it's important to consider the implication of choosing user-specific directories (as I say below).
Thoughts?
- Alex
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54335/#review158030
-----------------------------------------------------------
On Dec. 5, 2016, 5:38 p.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54335/
> -----------------------------------------------------------
>
> (Updated Dec. 5, 2016, 5:38 p.m.)
>
>
> Review request for mesos and Alex Clemmer.
>
>
> Bugs: MESOS-6677
> https://issues.apache.org/jira/browse/MESOS-6677
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Returns `/var` on POSIX and (usually) `C:\ProgramData` on Windows.
> Uses Windows API to look up correct location for persistent,
> app-local variable data. Returns standard location on POSIX.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/posix/os.hpp c37e64db662ba3cee83d2f55de0f9d71ad72c038
> 3rdparty/stout/include/stout/windows/os.hpp de9b04ad82443038a0f4408bc72cae1540a1beaf
>
> Diff: https://reviews.apache.org/r/54335/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 54335: Add os::var() to stout.
Posted by Alex Clemmer <cl...@gmail.com>.
> On Dec. 5, 2016, 6:31 p.m., Jie Yu wrote:
> > Flying by. I am checking https://www.gnu.org/prep/standards/html_node/Directory-Variables.html
> >
> > Looks like, in retrospect, we should call the current `runtime_dir` `runstate_dir` instead. So we probably should use `os::runstatedir`?
>
> Alex Clemmer wrote:
> +1, thanks for the helpful suggestion Jie. We were debating what to call this anyway. :)
>
> Alex Clemmer wrote:
> But, actually, I think I spoke too soon. The idea is actually to use this for all the places we use a directory rooted at `/var`, _i.e._, for all places we're dealing with variable data. I think the final picture of what the disk isolators and persistent volumes stuff will end up looking like is not yet fully developed, but the idea here is that we will want all of the places those things manage variable data to be managed out of a sensible `/var` on Windows, too. This is also why I think it's important to consider the implication of choosing user-specific directories (as I say below).
>
> Thoughts?
>
> Jie Yu wrote:
> Looks like according to GNU standard, this should be `localstatedir` (e.g., /var or /usr/local/var). And `runstatedir` is under `localstatedir` (i.e., `$(localstatedir)/run`). And for volumes which normally under `/var/lib`, should be called `libdir` according to GNU standard.
>
> Note that `/var/run` will normally cleared by OS upon reboot, while `/var/lib` contains persisent information across reboot.
Ok, those are good points. To make this stuff work in Windows, I think we will still need an `os::var`, but your suggestion to implement a series of Stout functions following the form `os::runstatedir`, _etc_. make sense and IMO are worth adopting. Does that sound good to you?
- Alex
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54335/#review158030
-----------------------------------------------------------
On Dec. 5, 2016, 5:38 p.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54335/
> -----------------------------------------------------------
>
> (Updated Dec. 5, 2016, 5:38 p.m.)
>
>
> Review request for mesos and Alex Clemmer.
>
>
> Bugs: MESOS-6677
> https://issues.apache.org/jira/browse/MESOS-6677
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Returns `/var` on POSIX and (usually) `C:\ProgramData` on Windows.
> Uses Windows API to look up correct location for persistent,
> app-local variable data. Returns standard location on POSIX.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/posix/os.hpp c37e64db662ba3cee83d2f55de0f9d71ad72c038
> 3rdparty/stout/include/stout/windows/os.hpp de9b04ad82443038a0f4408bc72cae1540a1beaf
>
> Diff: https://reviews.apache.org/r/54335/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 54335: Add os::var() to stout.
Posted by Alex Clemmer <cl...@gmail.com>.
> On Dec. 5, 2016, 6:31 p.m., Jie Yu wrote:
> > Flying by. I am checking https://www.gnu.org/prep/standards/html_node/Directory-Variables.html
> >
> > Looks like, in retrospect, we should call the current `runtime_dir` `runstate_dir` instead. So we probably should use `os::runstatedir`?
+1, thanks for the helpful suggestion Jie. We were debating what to call this anyway. :)
- Alex
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54335/#review158030
-----------------------------------------------------------
On Dec. 5, 2016, 5:38 p.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54335/
> -----------------------------------------------------------
>
> (Updated Dec. 5, 2016, 5:38 p.m.)
>
>
> Review request for mesos and Alex Clemmer.
>
>
> Bugs: MESOS-6677
> https://issues.apache.org/jira/browse/MESOS-6677
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Returns `/var` on POSIX and (usually) `C:\ProgramData` on Windows.
> Uses Windows API to look up correct location for persistent,
> app-local variable data. Returns standard location on POSIX.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/posix/os.hpp c37e64db662ba3cee83d2f55de0f9d71ad72c038
> 3rdparty/stout/include/stout/windows/os.hpp de9b04ad82443038a0f4408bc72cae1540a1beaf
>
> Diff: https://reviews.apache.org/r/54335/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 54335: Add os::var() to stout.
Posted by Jie Yu <yu...@gmail.com>.
> On Dec. 5, 2016, 6:31 p.m., Jie Yu wrote:
> > Flying by. I am checking https://www.gnu.org/prep/standards/html_node/Directory-Variables.html
> >
> > Looks like, in retrospect, we should call the current `runtime_dir` `runstate_dir` instead. So we probably should use `os::runstatedir`?
>
> Alex Clemmer wrote:
> +1, thanks for the helpful suggestion Jie. We were debating what to call this anyway. :)
>
> Alex Clemmer wrote:
> But, actually, I think I spoke too soon. The idea is actually to use this for all the places we use a directory rooted at `/var`, _i.e._, for all places we're dealing with variable data. I think the final picture of what the disk isolators and persistent volumes stuff will end up looking like is not yet fully developed, but the idea here is that we will want all of the places those things manage variable data to be managed out of a sensible `/var` on Windows, too. This is also why I think it's important to consider the implication of choosing user-specific directories (as I say below).
>
> Thoughts?
Looks like according to GNU standard, this should be `localstatedir` (e.g., /var or /usr/local/var). And `runstatedir` is under `localstatedir` (i.e., `$(localstatedir)/run`). And for volumes which normally under `/var/lib`, should be called `libdir` according to GNU standard.
Note that `/var/run` will normally cleared by OS upon reboot, while `/var/lib` contains persisent information across reboot.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54335/#review158030
-----------------------------------------------------------
On Dec. 5, 2016, 5:38 p.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54335/
> -----------------------------------------------------------
>
> (Updated Dec. 5, 2016, 5:38 p.m.)
>
>
> Review request for mesos and Alex Clemmer.
>
>
> Bugs: MESOS-6677
> https://issues.apache.org/jira/browse/MESOS-6677
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Returns `/var` on POSIX and (usually) `C:\ProgramData` on Windows.
> Uses Windows API to look up correct location for persistent,
> app-local variable data. Returns standard location on POSIX.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/posix/os.hpp c37e64db662ba3cee83d2f55de0f9d71ad72c038
> 3rdparty/stout/include/stout/windows/os.hpp de9b04ad82443038a0f4408bc72cae1540a1beaf
>
> Diff: https://reviews.apache.org/r/54335/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 54335: Add os::var() to stout.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54335/#review158030
-----------------------------------------------------------
Flying by. I am checking https://www.gnu.org/prep/standards/html_node/Directory-Variables.html
Looks like, in retrospect, we should call the current `runtime_dir` `runstate_dir` instead. So we probably should use `os::runstatedir`?
- Jie Yu
On Dec. 5, 2016, 5:38 p.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54335/
> -----------------------------------------------------------
>
> (Updated Dec. 5, 2016, 5:38 p.m.)
>
>
> Review request for mesos and Alex Clemmer.
>
>
> Bugs: MESOS-6677
> https://issues.apache.org/jira/browse/MESOS-6677
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Returns `/var` on POSIX and (usually) `C:\ProgramData` on Windows.
> Uses Windows API to look up correct location for persistent,
> app-local variable data. Returns standard location on POSIX.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/posix/os.hpp c37e64db662ba3cee83d2f55de0f9d71ad72c038
> 3rdparty/stout/include/stout/windows/os.hpp de9b04ad82443038a0f4408bc72cae1540a1beaf
>
> Diff: https://reviews.apache.org/r/54335/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 54335: Add `os::var()` to Stout.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54335/
-----------------------------------------------------------
(Updated Dec. 8, 2016, 8:15 p.m.)
Review request for mesos and Alex Clemmer.
Changes
-------
Fix formatting of description.
Bugs: MESOS-6677 and MESOS-6722
https://issues.apache.org/jira/browse/MESOS-6677
https://issues.apache.org/jira/browse/MESOS-6722
Repository: mesos
Description (updated)
-------
Returns `/var` on POSIX and (usually) `C:\ProgramData` on Windows. Uses Windows
COM API to look up correct location for persistent, app-local (but not per user)
variable data. Returns standard location on POSIX.
The addition of `os::var()` is a continuation of the fix in #54336. The correct
place for variable runtime data on Windows is not necessarily in `os::temp()`, but
in the analogous location `ProgramData`. Thus we need a platform-agnostic way to
refer to var.
The call to `ShGetKnownFolder` is not RAII because it is a C API, and the ATL
`CComHeapPtr` class is not used in this commit due to Windows header issues. Thus
the buffer allocated by the C API is freed immediately after the data is copied
into a `std::wstring`. Because the Windows API returns a UTF-16 string, and
Unicode characters are valid in Windows path names, we have to correctly convert
it to UTF-8 using `<codecvt>`.
Diffs
-----
3rdparty/stout/include/stout/posix/os.hpp 8443aa0cf0a8d8d52e36282611c2ab15ca4dd354
3rdparty/stout/include/stout/windows.hpp d89c70902cf60544441608c2cb290b0727cbb45c
3rdparty/stout/include/stout/windows/os.hpp 2f20ccc64e255a60a1b7f33d684969942f12e45f
Diff: https://reviews.apache.org/r/54335/diff/
Testing
-------
make && make check on Linux: no failures.
msbuild and attach to a master on Windows: no failures.
Thanks,
Andrew Schwartzmeyer
Re: Review Request 54335: Add `os::var()` to Stout.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54335/
-----------------------------------------------------------
(Updated Dec. 8, 2016, 8:07 p.m.)
Review request for mesos and Alex Clemmer.
Changes
-------
Put back updated description.
Bugs: MESOS-6677 and MESOS-6722
https://issues.apache.org/jira/browse/MESOS-6677
https://issues.apache.org/jira/browse/MESOS-6722
Repository: mesos
Description (updated)
-------
Returns `/var` on POSIX and (usually) `C:\ProgramData` on Windows. Uses Windows
COM API to look up correct location for persistent, app-local (but not per user)
variable data. Returns standard location on POSIX.
The addition of os::var() is a continuation of the fix in #54336. The correct
place for variable runtime data on Windows is not necessarily in os::temp(), but
in the analogous location ProgramData. Thus we need a platform-agnostic way to
refer to var.
The call to ShGetKnownFolder is not RAII because it is a C API, and the ATL
CComHeapPtr class is not used in this commit due to Windows header issues. Thus
the buffer allocated by the C API is freed immediately after the data is copied
into a std::wstring. Because the Windows API returns a UTF-16 string, and
Unicode characters are valid in Windows path names, we have to correctly convert
it to UTF-8 using <codecvt>.
Diffs
-----
3rdparty/stout/include/stout/posix/os.hpp 8443aa0cf0a8d8d52e36282611c2ab15ca4dd354
3rdparty/stout/include/stout/windows.hpp d89c70902cf60544441608c2cb290b0727cbb45c
3rdparty/stout/include/stout/windows/os.hpp 2f20ccc64e255a60a1b7f33d684969942f12e45f
Diff: https://reviews.apache.org/r/54335/diff/
Testing
-------
make && make check on Linux: no failures.
msbuild and attach to a master on Windows: no failures.
Thanks,
Andrew Schwartzmeyer
Re: Review Request 54335: Add `os::var()` to Stout.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54335/
-----------------------------------------------------------
(Updated Dec. 8, 2016, 8:02 p.m.)
Review request for mesos and Alex Clemmer.
Bugs: MESOS-6677 and MESOS-6722
https://issues.apache.org/jira/browse/MESOS-6677
https://issues.apache.org/jira/browse/MESOS-6722
Repository: mesos
Description (updated)
-------
Returns `/var` on POSIX and (usually) `C:\ProgramData` on Windows.
Uses Windows COM API to look up correct location for persistent,
app-local (but not per user) variable data.
Returns standard location on POSIX.
Diffs (updated)
-----
3rdparty/stout/include/stout/posix/os.hpp 8443aa0cf0a8d8d52e36282611c2ab15ca4dd354
3rdparty/stout/include/stout/windows.hpp d89c70902cf60544441608c2cb290b0727cbb45c
3rdparty/stout/include/stout/windows/os.hpp 2f20ccc64e255a60a1b7f33d684969942f12e45f
Diff: https://reviews.apache.org/r/54335/diff/
Testing
-------
make && make check on Linux: no failures.
msbuild and attach to a master on Windows: no failures.
Thanks,
Andrew Schwartzmeyer
Re: Review Request 54335: Add `os::var()` to Stout.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
> On Dec. 8, 2016, 6:14 a.m., Alex Clemmer wrote:
> > 3rdparty/stout/include/stout/windows/os.hpp, line 20
> > <https://reviews.apache.org/r/54335/diff/1-2/?file=1575119#file1575119line20>
> >
> > Tiny nit: while you're messing around with the headers, could we alphabetize them?
No problem.
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54335/#review158500
-----------------------------------------------------------
On Dec. 8, 2016, 1:49 a.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54335/
> -----------------------------------------------------------
>
> (Updated Dec. 8, 2016, 1:49 a.m.)
>
>
> Review request for mesos and Alex Clemmer.
>
>
> Bugs: MESOS-6677 and MESOS-6722
> https://issues.apache.org/jira/browse/MESOS-6677
> https://issues.apache.org/jira/browse/MESOS-6722
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Returns `/var` on POSIX and (usually) `C:\ProgramData` on Windows.
> Uses Windows COM API to look up correct location for persistent,
> app-local (but not per user) variable data. Returns standard location on POSIX.
>
> The addition of `os::var()` is a continuation of the fix in #54336.
> The correct place for variable runtime data on Windows is not
> necessarily in `os::temp()`, but in the analogous location `ProgramData`.
> Thus we need a platform-agnostic way to refer to `var`.
>
> The call to `ShGetKnownFolder` is not RAII because it is a C API,
> and the ATL `CComHeapPtr` class is not used in this commit
> due to Windows header issues.
> Thus the buffer allocated by the C API is freed immediately after
> the data is copied into a `std::wstring`.
> Because the Windows API returns a UTF-16 string,
> and Unicode characters are valid in Windows path names,
> we have to correctly convert it to UTF-8 using `<codecvt>`.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/posix/os.hpp 8443aa0cf0a8d8d52e36282611c2ab15ca4dd354
> 3rdparty/stout/include/stout/windows/os.hpp 2f20ccc64e255a60a1b7f33d684969942f12e45f
>
> Diff: https://reviews.apache.org/r/54335/diff/
>
>
> Testing
> -------
>
> make && make check on Linux: no failures.
> msbuild and attach to a master on Windows: no failures.
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 54335: Add `os::var()` to Stout.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
> On Dec. 8, 2016, 6:14 a.m., Alex Clemmer wrote:
> > 3rdparty/stout/include/stout/windows/os.hpp, line 772
> > <https://reviews.apache.org/r/54335/diff/1-2/?file=1575119#file1575119line772>
> >
> > I think it's worth mentioning specifically something like "will not cause us to return different directories for different users." I think when I read this in x years it will not be super clear to me what you mean when you say "will not change the result." :) Change the result, with respect to what?
Ah, yeah that could be more clear.
> On Dec. 8, 2016, 6:14 a.m., Alex Clemmer wrote:
> > 3rdparty/stout/include/stout/windows/os.hpp, line 754
> > <https://reviews.apache.org/r/54335/diff/2/?file=1579535#file1579535line754>
> >
> > Mesos style is to use `wchar_t* var_folder` rather than `wchar_t *var_folder`.
> >
> > Also, smaller question: is there a particular reason we switched from `PWSTR` to `wchar_t`? If not, it seems to me that if the win32 API is going to go through the trouble of using opaque typedefs, we should use those when we call their functions, too. (And convert to something more appropriate to Mesos before returning any data.) Here and elsewhere. Or, alternatively, we might consider putting a `static_assert` into Windows to make sure these are the same type. (Perhaps we should do this anyway since I believe the `wstring` constructor below doesn't take `PWSTR`.)
Who puts the pointers on the types?!? Google Style Guide says either works (I did check :). I'll fix it, thanks.
`PWSTR` to `wchar_t` because who on earth wants Hungarian notation in this code. I like your idea of a `static_assert`, that's the best solution here. `PWSTR` *should be* pointer-to-wide-c-string, but since we require that in our use, we should ensure it.
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54335/#review158500
-----------------------------------------------------------
On Dec. 8, 2016, 1:49 a.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54335/
> -----------------------------------------------------------
>
> (Updated Dec. 8, 2016, 1:49 a.m.)
>
>
> Review request for mesos and Alex Clemmer.
>
>
> Bugs: MESOS-6677 and MESOS-6722
> https://issues.apache.org/jira/browse/MESOS-6677
> https://issues.apache.org/jira/browse/MESOS-6722
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Returns `/var` on POSIX and (usually) `C:\ProgramData` on Windows.
> Uses Windows COM API to look up correct location for persistent,
> app-local (but not per user) variable data. Returns standard location on POSIX.
>
> The addition of `os::var()` is a continuation of the fix in #54336.
> The correct place for variable runtime data on Windows is not
> necessarily in `os::temp()`, but in the analogous location `ProgramData`.
> Thus we need a platform-agnostic way to refer to `var`.
>
> The call to `ShGetKnownFolder` is not RAII because it is a C API,
> and the ATL `CComHeapPtr` class is not used in this commit
> due to Windows header issues.
> Thus the buffer allocated by the C API is freed immediately after
> the data is copied into a `std::wstring`.
> Because the Windows API returns a UTF-16 string,
> and Unicode characters are valid in Windows path names,
> we have to correctly convert it to UTF-8 using `<codecvt>`.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/posix/os.hpp 8443aa0cf0a8d8d52e36282611c2ab15ca4dd354
> 3rdparty/stout/include/stout/windows/os.hpp 2f20ccc64e255a60a1b7f33d684969942f12e45f
>
> Diff: https://reviews.apache.org/r/54335/diff/
>
>
> Testing
> -------
>
> make && make check on Linux: no failures.
> msbuild and attach to a master on Windows: no failures.
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 54335: Add `os::var()` to Stout.
Posted by Alex Clemmer <cl...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54335/#review158500
-----------------------------------------------------------
3rdparty/stout/include/stout/windows/os.hpp (line 20)
<https://reviews.apache.org/r/54335/#comment229265>
Tiny nit: while you're messing around with the headers, could we alphabetize them?
3rdparty/stout/include/stout/windows/os.hpp (line 756)
<https://reviews.apache.org/r/54335/#comment229266>
I think it's worth mentioning specifically something like "will not cause us to return different directories for different users." I think when I read this in x years it will not be super clear to me what you mean when you say "will not change the result." :) Change the result, with respect to what?
3rdparty/stout/include/stout/windows/os.hpp (line 754)
<https://reviews.apache.org/r/54335/#comment229264>
Mesos style is to use `wchar_t* var_folder` rather than `wchar_t *var_folder`.
Also, smaller question: is there a particular reason we switched from `PWSTR` to `wchar_t`? If not, it seems to me that if the win32 API is going to go through the trouble of using opaque typedefs, we should use those when we call their functions, too. (And convert to something more appropriate to Mesos before returning any data.) Here and elsewhere. Or, alternatively, we might consider putting a `static_assert` into Windows to make sure these are the same type. (Perhaps we should do this anyway since I believe the `wstring` constructor below doesn't take `PWSTR`.)
3rdparty/stout/include/stout/windows/os.hpp (lines 757 - 760)
<https://reviews.apache.org/r/54335/#comment229263>
I actually forget whether this is style compliant. We definitely do this in other parts of the codebase, (_e.g._, [1]), but I have occasionally been told in cases like these to put the first argument on the next line, and to indent those lines 4 spaces. I prefer this way, because it makes it easier to read the `!= S_OK`.
I'm not opening this as an issue, but I'm still leaving this as a comment to give Joseph the opportunity to set me straight here. :)
[1] https://github.com/apache/mesos/blob/master/3rdparty/libprocess/src/io.cpp#L91-L97
- Alex Clemmer
On Dec. 8, 2016, 1:49 a.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54335/
> -----------------------------------------------------------
>
> (Updated Dec. 8, 2016, 1:49 a.m.)
>
>
> Review request for mesos and Alex Clemmer.
>
>
> Bugs: MESOS-6677 and MESOS-6722
> https://issues.apache.org/jira/browse/MESOS-6677
> https://issues.apache.org/jira/browse/MESOS-6722
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Returns `/var` on POSIX and (usually) `C:\ProgramData` on Windows.
> Uses Windows COM API to look up correct location for persistent,
> app-local (but not per user) variable data. Returns standard location on POSIX.
>
> The addition of `os::var()` is a continuation of the fix in #54336.
> The correct place for variable runtime data on Windows is not
> necessarily in `os::temp()`, but in the analogous location `ProgramData`.
> Thus we need a platform-agnostic way to refer to `var`.
>
> The call to `ShGetKnownFolder` is not RAII because it is a C API,
> and the ATL `CComHeapPtr` class is not used in this commit
> due to Windows header issues.
> Thus the buffer allocated by the C API is freed immediately after
> the data is copied into a `std::wstring`.
> Because the Windows API returns a UTF-16 string,
> and Unicode characters are valid in Windows path names,
> we have to correctly convert it to UTF-8 using `<codecvt>`.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/posix/os.hpp 8443aa0cf0a8d8d52e36282611c2ab15ca4dd354
> 3rdparty/stout/include/stout/windows/os.hpp 2f20ccc64e255a60a1b7f33d684969942f12e45f
>
> Diff: https://reviews.apache.org/r/54335/diff/
>
>
> Testing
> -------
>
> make && make check on Linux: no failures.
> msbuild and attach to a master on Windows: no failures.
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 54335: Add `os::var()` to Stout.
Posted by Alex Clemmer <cl...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54335/#review158503
-----------------------------------------------------------
Ship it!
Oops, sorry, meant to mark this as "Fix it, then ship it!" My bad.
- Alex Clemmer
On Dec. 8, 2016, 1:49 a.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54335/
> -----------------------------------------------------------
>
> (Updated Dec. 8, 2016, 1:49 a.m.)
>
>
> Review request for mesos and Alex Clemmer.
>
>
> Bugs: MESOS-6677 and MESOS-6722
> https://issues.apache.org/jira/browse/MESOS-6677
> https://issues.apache.org/jira/browse/MESOS-6722
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Returns `/var` on POSIX and (usually) `C:\ProgramData` on Windows.
> Uses Windows COM API to look up correct location for persistent,
> app-local (but not per user) variable data. Returns standard location on POSIX.
>
> The addition of `os::var()` is a continuation of the fix in #54336.
> The correct place for variable runtime data on Windows is not
> necessarily in `os::temp()`, but in the analogous location `ProgramData`.
> Thus we need a platform-agnostic way to refer to `var`.
>
> The call to `ShGetKnownFolder` is not RAII because it is a C API,
> and the ATL `CComHeapPtr` class is not used in this commit
> due to Windows header issues.
> Thus the buffer allocated by the C API is freed immediately after
> the data is copied into a `std::wstring`.
> Because the Windows API returns a UTF-16 string,
> and Unicode characters are valid in Windows path names,
> we have to correctly convert it to UTF-8 using `<codecvt>`.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/posix/os.hpp 8443aa0cf0a8d8d52e36282611c2ab15ca4dd354
> 3rdparty/stout/include/stout/windows/os.hpp 2f20ccc64e255a60a1b7f33d684969942f12e45f
>
> Diff: https://reviews.apache.org/r/54335/diff/
>
>
> Testing
> -------
>
> make && make check on Linux: no failures.
> msbuild and attach to a master on Windows: no failures.
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 54335: Add `os::var()` to Stout.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54335/
-----------------------------------------------------------
(Updated Dec. 8, 2016, 1:49 a.m.)
Review request for mesos and Alex Clemmer.
Bugs: MESOS-6677 and MESOS-6722
https://issues.apache.org/jira/browse/MESOS-6677
https://issues.apache.org/jira/browse/MESOS-6722
Repository: mesos
Description (updated)
-------
Returns `/var` on POSIX and (usually) `C:\ProgramData` on Windows.
Uses Windows COM API to look up correct location for persistent,
app-local (but not per user) variable data. Returns standard location on POSIX.
The addition of `os::var()` is a continuation of the fix in #54336.
The correct place for variable runtime data on Windows is not
necessarily in `os::temp()`, but in the analogous location `ProgramData`.
Thus we need a platform-agnostic way to refer to `var`.
The call to `ShGetKnownFolder` is not RAII because it is a C API,
and the ATL `CComHeapPtr` class is not used in this commit
due to Windows header issues.
Thus the buffer allocated by the C API is freed immediately after
the data is copied into a `std::wstring`.
Because the Windows API returns a UTF-16 string,
and Unicode characters are valid in Windows path names,
we have to correctly convert it to UTF-8 using `<codecvt>`.
Diffs
-----
3rdparty/stout/include/stout/posix/os.hpp 8443aa0cf0a8d8d52e36282611c2ab15ca4dd354
3rdparty/stout/include/stout/windows/os.hpp 2f20ccc64e255a60a1b7f33d684969942f12e45f
Diff: https://reviews.apache.org/r/54335/diff/
Testing (updated)
-------
make && make check on Linux: no failures.
msbuild and attach to a master on Windows: no failures.
Thanks,
Andrew Schwartzmeyer
Re: Review Request 54335: Add `os::var()` to Stout.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54335/
-----------------------------------------------------------
(Updated Dec. 8, 2016, 12:39 a.m.)
Review request for mesos and Alex Clemmer.
Summary (updated)
-----------------
Add `os::var()` to Stout.
Bugs: MESOS-6677 and MESOS-6722
https://issues.apache.org/jira/browse/MESOS-6677
https://issues.apache.org/jira/browse/MESOS-6722
Repository: mesos
Description (updated)
-------
Returns `/var` on POSIX and (usually) `C:\ProgramData` on Windows.
Uses Windows COM API to look up correct location for persistent,
app-local (but not per user) variable data.
Returns standard location on POSIX.
Diffs (updated)
-----
3rdparty/stout/include/stout/posix/os.hpp 8443aa0cf0a8d8d52e36282611c2ab15ca4dd354
3rdparty/stout/include/stout/windows/os.hpp 2f20ccc64e255a60a1b7f33d684969942f12e45f
Diff: https://reviews.apache.org/r/54335/diff/
Testing
-------
Thanks,
Andrew Schwartzmeyer
Re: Review Request 54335: Add os::var() to Stout.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54335/
-----------------------------------------------------------
(Updated Dec. 8, 2016, 12:21 a.m.)
Review request for mesos and Alex Clemmer.
Summary (updated)
-----------------
Add os::var() to Stout.
Bugs: MESOS-6677 and MESOS-6722
https://issues.apache.org/jira/browse/MESOS-6677
https://issues.apache.org/jira/browse/MESOS-6722
Repository: mesos
Description
-------
Returns `/var` on POSIX and (usually) `C:\ProgramData` on Windows.
Uses Windows API to look up correct location for persistent,
app-local variable data. Returns standard location on POSIX.
Diffs
-----
3rdparty/stout/include/stout/posix/os.hpp c37e64db662ba3cee83d2f55de0f9d71ad72c038
3rdparty/stout/include/stout/windows/os.hpp de9b04ad82443038a0f4408bc72cae1540a1beaf
Diff: https://reviews.apache.org/r/54335/diff/
Testing
-------
Thanks,
Andrew Schwartzmeyer
Re: Review Request 54335: Add os::var() to stout.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54335/#review158122
-----------------------------------------------------------
Patch looks great!
Reviews applied: [54335]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh
- Mesos ReviewBot
On Dec. 5, 2016, 5:38 p.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54335/
> -----------------------------------------------------------
>
> (Updated Dec. 5, 2016, 5:38 p.m.)
>
>
> Review request for mesos and Alex Clemmer.
>
>
> Bugs: MESOS-6677
> https://issues.apache.org/jira/browse/MESOS-6677
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Returns `/var` on POSIX and (usually) `C:\ProgramData` on Windows.
> Uses Windows API to look up correct location for persistent,
> app-local variable data. Returns standard location on POSIX.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/posix/os.hpp c37e64db662ba3cee83d2f55de0f9d71ad72c038
> 3rdparty/stout/include/stout/windows/os.hpp de9b04ad82443038a0f4408bc72cae1540a1beaf
>
> Diff: https://reviews.apache.org/r/54335/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 54335: Add os::var() to Stout.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
> On Dec. 5, 2016, 7:30 p.m., Alex Clemmer wrote:
> > 3rdparty/stout/include/stout/windows/os.hpp, line 782
> > <https://reviews.apache.org/r/54335/diff/1/?file=1575119#file1575119line782>
> >
> > I think it's worth considering whether this should be canonized and encoded as its own function in `stout/strings.hpp` or something. It seems like we don't want to be hand-rolling a unicode conversion in every place we need to convert from `wchar` -> `char`, as it's super error prone. Also, it will make it easier to manage locale changes later.
>
> Andrew Schwartzmeyer wrote:
> Absolutely it should be. The other problem with this is that it's assuming none of the characters in the original `wstring` were actually multi-byte. While this is *probably* true, it is **not** guaranteed per [MSDN](https://msdn.microsoft.com/en-us/library/windows/desktop/aa365247(v=vs.85).aspx), a path may:
> > Use any character in the current code page for a name, including Unicode characters and characters in the extended character set (128\u2013255)
>
> So I'll need to fix this.
This actually turned out not to be too bad. In the new version, it's:
```
// Convert UTF-16 `wstring` to UTF-8 `string`.
std::wstring_convert<std::codecvt_utf8_utf16<wchar_t>, wchar_t> converter;
return converter.to_bytes(wvar_folder.c_str());
```
It's the canonical C++ way to convert UTF-16 to UTF-8 (and back if need be). If we start to use it more we could wrap it to make it a bit cleaner looking.
> On Dec. 5, 2016, 7:30 p.m., Alex Clemmer wrote:
> > 3rdparty/stout/include/stout/windows/os.hpp, line 769
> > <https://reviews.apache.org/r/54335/diff/1/?file=1575119#file1575119line769>
> >
> > Although cumbersome, I would personally like to see this RAII'd somehow, perhaps with a `unique_ptr` or even something incidentally RAII'd, like a `vector`.
> >
> > My reason for suggesting this is not just safety, but also a matter of reasoning about the code: I tend to find it easier to reason about code when it's clear when something is done initializing, and when it's clear what the spec for destruction is, and the easiest way to accomplish this is to use RAII.
>
> Andrew Schwartzmeyer wrote:
> This is going to be interesting as the API handles the initialization... I'll see what I can do.
Unfortunately, while `CComHeapPtr` from the ATL would be the way to go, including the ATL headers proved to be an exercise in frustration due to our definition of `NOGDI` and a conflicing definition of `_WINGDI_`. The alternative approach of removing `NOGDI` and fixing the root problem it was addressing (the double definition of `ERROR`) was also fruitless, as `ERROR` isn't the only double definition (next up was `DEBUG`). While this should be done *at some point*, I don't think it should be done here. So I'm left with having to free the allocation done by the C API.
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54335/#review158032
-----------------------------------------------------------
On Dec. 8, 2016, 12:21 a.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54335/
> -----------------------------------------------------------
>
> (Updated Dec. 8, 2016, 12:21 a.m.)
>
>
> Review request for mesos and Alex Clemmer.
>
>
> Bugs: MESOS-6677 and MESOS-6722
> https://issues.apache.org/jira/browse/MESOS-6677
> https://issues.apache.org/jira/browse/MESOS-6722
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Returns `/var` on POSIX and (usually) `C:\ProgramData` on Windows.
> Uses Windows API to look up correct location for persistent,
> app-local variable data. Returns standard location on POSIX.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/posix/os.hpp c37e64db662ba3cee83d2f55de0f9d71ad72c038
> 3rdparty/stout/include/stout/windows/os.hpp de9b04ad82443038a0f4408bc72cae1540a1beaf
>
> Diff: https://reviews.apache.org/r/54335/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 54335: Add os::var() to stout.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
> On Dec. 5, 2016, 7:30 p.m., Alex Clemmer wrote:
> > 3rdparty/stout/include/stout/posix/os.hpp, line 463
> > <https://reviews.apache.org/r/54335/diff/1/?file=1575118#file1575118line463>
> >
> > We probably want this function to return `Try<std::string>`, because we want to be able to use `os::var` in code that touches both the Windows and Unix codepaths.
Thanks, this was a bug I didn't catch since I hadn't built on Linux just yet.
> On Dec. 5, 2016, 7:30 p.m., Alex Clemmer wrote:
> > 3rdparty/stout/include/stout/windows/os.hpp, line 770
> > <https://reviews.apache.org/r/54335/diff/1/?file=1575119#file1575119line770>
> >
> > Technically the spec for this function does say that the second argument can be `0`, but I personally prefer the more self-documenting `KF_FLAG_DEFAULT`.
> >
> > Let me also give you something else to consider, that slightly contradicts what I'd thought before. Upon reading this code, I'd like to suggest that it is worth considering whether it is desirable to have `nullptr` passed in as the third argument, as Mesos is constantly launching processes with different users, and it might lead to unintended consequences to have `os::var` return different directories under different circumstances. The Windows implementation of the agent obviously has a lot of problems with `su`, but it I think the spirit of the POSIX spec of `/var` is a globally-accessible path that might be permissioned so that only `root` can access it, and I think it's worth thinking about duplicating those semantics here, too, perhaps by passing in the handle of the administrator, and (especially if getting that handle requires other permissions) perhaps finding a more appropriate path for this.
> >
> > If you'd like to punt on this point, that's fine, but I would encourage you then to document this with a comment to make the consequences of this design decision clear, as well as file a bug into our backlog to follow up on this later.
Agreed on `KF_FLAG_DEFAULT` over `0`.
It would be a major problem if this folder changed per user. Fortunately, the [`KNOWNFOLDERID`](https://msdn.microsoft.com/en-us/library/windows/desktop/dd378457(v=vs.85).aspx) `ProgramData` has folder type [`KF_CATEGORY enumeration`](https://msdn.microsoft.com/en-us/library/windows/desktop/bb762512(v=vs.85).aspx) `FIXED` which does not change:
> Fixed file system folders are not managed by the Shell and are usually given a permanent path when the system is installed. For example, the Windows and Program Files folders are fixed folders. A number of features such as redirection do not apply to this category.
So we are safe to use `nullptr` here. I've added a comment explaining such to the code.
> On Dec. 5, 2016, 7:30 p.m., Alex Clemmer wrote:
> > 3rdparty/stout/include/stout/windows/os.hpp, line 772
> > <https://reviews.apache.org/r/54335/diff/1/?file=1575119#file1575119line772>
> >
> > Typically we would want a relevant error message here. Although our error package does not report things like the line number, a good error message will at the very least give you something to `grep` for when a function crashes your process.
> >
> > (Also if you saw use returning just `WindowsError()` somewhere else, that's probably my fault, and you should learn from my pain :).)
Haha, I did indeed. Added a descriptive error message.
> On Dec. 5, 2016, 7:30 p.m., Alex Clemmer wrote:
> > 3rdparty/stout/include/stout/windows/os.hpp, line 782
> > <https://reviews.apache.org/r/54335/diff/1/?file=1575119#file1575119line782>
> >
> > I think it's worth considering whether this should be canonized and encoded as its own function in `stout/strings.hpp` or something. It seems like we don't want to be hand-rolling a unicode conversion in every place we need to convert from `wchar` -> `char`, as it's super error prone. Also, it will make it easier to manage locale changes later.
Absolutely it should be. The other problem with this is that it's assuming none of the characters in the original `wstring` were actually multi-byte. While this is *probably* true, it is **not** guaranteed per [MSDN](https://msdn.microsoft.com/en-us/library/windows/desktop/aa365247(v=vs.85).aspx), a path may:
> Use any character in the current code page for a name, including Unicode characters and characters in the extended character set (128\u2013255)
So I'll need to fix this.
> On Dec. 5, 2016, 7:30 p.m., Alex Clemmer wrote:
> > 3rdparty/stout/include/stout/windows/os.hpp, line 769
> > <https://reviews.apache.org/r/54335/diff/1/?file=1575119#file1575119line769>
> >
> > Although cumbersome, I would personally like to see this RAII'd somehow, perhaps with a `unique_ptr` or even something incidentally RAII'd, like a `vector`.
> >
> > My reason for suggesting this is not just safety, but also a matter of reasoning about the code: I tend to find it easier to reason about code when it's clear when something is done initializing, and when it's clear what the spec for destruction is, and the easiest way to accomplish this is to use RAII.
This is going to be interesting as the API handles the initialization... I'll see what I can do.
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54335/#review158032
-----------------------------------------------------------
On Dec. 5, 2016, 5:38 p.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54335/
> -----------------------------------------------------------
>
> (Updated Dec. 5, 2016, 5:38 p.m.)
>
>
> Review request for mesos and Alex Clemmer.
>
>
> Bugs: MESOS-6677
> https://issues.apache.org/jira/browse/MESOS-6677
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Returns `/var` on POSIX and (usually) `C:\ProgramData` on Windows.
> Uses Windows API to look up correct location for persistent,
> app-local variable data. Returns standard location on POSIX.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/posix/os.hpp c37e64db662ba3cee83d2f55de0f9d71ad72c038
> 3rdparty/stout/include/stout/windows/os.hpp de9b04ad82443038a0f4408bc72cae1540a1beaf
>
> Diff: https://reviews.apache.org/r/54335/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 54335: Add os::var() to stout.
Posted by Alex Clemmer <cl...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54335/#review158032
-----------------------------------------------------------
3rdparty/stout/include/stout/posix/os.hpp (line 463)
<https://reviews.apache.org/r/54335/#comment228696>
We probably want this function to return `Try<std::string>`, because we want to be able to use `os::var` in code that touches both the Windows and Unix codepaths.
3rdparty/stout/include/stout/windows/os.hpp (line 769)
<https://reviews.apache.org/r/54335/#comment228708>
Although cumbersome, I would personally like to see this RAII'd somehow, perhaps with a `unique_ptr` or even something incidentally RAII'd, like a `vector`.
My reason for suggesting this is not just safety, but also a matter of reasoning about the code: I tend to find it easier to reason about code when it's clear when something is done initializing, and when it's clear what the spec for destruction is, and the easiest way to accomplish this is to use RAII.
3rdparty/stout/include/stout/windows/os.hpp (line 770)
<https://reviews.apache.org/r/54335/#comment228706>
Technically the spec for this function does say that the second argument can be `0`, but I personally prefer the more self-documenting `KF_FLAG_DEFAULT`.
Let me also give you something else to consider, that slightly contradicts what I'd thought before. Upon reading this code, I'd like to suggest that it is worth considering whether it is desirable to have `nullptr` passed in as the third argument, as Mesos is constantly launching processes with different users, and it might lead to unintended consequences to have `os::var` return different directories under different circumstances. The Windows implementation of the agent obviously has a lot of problems with `su`, but it I think the spirit of the POSIX spec of `/var` is a globally-accessible path that might be permissioned so that only `root` can access it, and I think it's worth thinking about duplicating those semantics here, too, perhaps by passing in the handle of the administrator, and (especially if getting that handle requires other permissions) perhaps finding a more appropriate path for this.
If you'd like to punt on this point, that's fine, but I would encourage you then to document this with a comment to make the consequences of this design decision clear, as well as file a bug into our backlog to follow up on this later.
3rdparty/stout/include/stout/windows/os.hpp (line 772)
<https://reviews.apache.org/r/54335/#comment228699>
Typically we would want a relevant error message here. Although our error package does not report things like the line number, a good error message will at the very least give you something to `grep` for when a function crashes your process.
(Also if you saw use returning just `WindowsError()` somewhere else, that's probably my fault, and you should learn from my pain :).)
3rdparty/stout/include/stout/windows/os.hpp (line 782)
<https://reviews.apache.org/r/54335/#comment228719>
I think it's worth considering whether this should be canonized and encoded as its own function in `stout/strings.hpp` or something. It seems like we don't want to be hand-rolling a unicode conversion in every place we need to convert from `wchar` -> `char`, as it's super error prone. Also, it will make it easier to manage locale changes later.
- Alex Clemmer
On Dec. 5, 2016, 5:38 p.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54335/
> -----------------------------------------------------------
>
> (Updated Dec. 5, 2016, 5:38 p.m.)
>
>
> Review request for mesos and Alex Clemmer.
>
>
> Bugs: MESOS-6677
> https://issues.apache.org/jira/browse/MESOS-6677
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Returns `/var` on POSIX and (usually) `C:\ProgramData` on Windows.
> Uses Windows API to look up correct location for persistent,
> app-local variable data. Returns standard location on POSIX.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/posix/os.hpp c37e64db662ba3cee83d2f55de0f9d71ad72c038
> 3rdparty/stout/include/stout/windows/os.hpp de9b04ad82443038a0f4408bc72cae1540a1beaf
>
> Diff: https://reviews.apache.org/r/54335/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>