mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrew Schwartzmeyer <and...@schwartzmeyer.com>
Subject Re: Review Request 54877: Windows: Stout: Removed dependency on Shell API.
Date Tue, 20 Dec 2016 18:50:41 GMT


> On Dec. 20, 2016, 12:29 a.m., Daniel Pravat wrote:
> > 3rdparty/stout/include/stout/windows/os.hpp, line 748
> > <https://reviews.apache.org/r/54877/diff/1/?file=1589098#file1589098line748>
> >
> >     I don't think the conversion to UTF-8 is appropiate here.
> 
> Andrew Schwartzmeyer wrote:
>     What would you convert it to? It's currently in UTF-16, and Windows paths are allowed
to have (almost) any Unicode character.
> 
> Alex Clemmer wrote:
>     I think he's saying that bad things will happen if you use strings that have Unicode
in them, so it's probably better to just error out.

I'm not following... That is a case our codebase must deal with, NTFS paths are stored in
Unicode:

https://msdn.microsoft.com/en-us/library/windows/desktop/dd317748(v=vs.85).aspx

and on top of that, Windows file paths can:

> Use any character in the current code page for a name, including Unicode characters and
characters in the extended character set (128–255)

https://msdn.microsoft.com/en-us/library/windows/desktop/aa365247(v=vs.85).aspx

So what I'm saying is that bad things _must not happen_ if we have paths with non-ASCII characters
in them; we need to handle that at any point we deal with file paths on Windows.

I'm not saying this function is likely to return a path like `C:\ÂÑÐ¥`, but that would
be a valid file path, and so we must be treating all our paths on Windows as Unicode.

If this is not the case, are purposefully constraining ourselves to ASCII, and if so, why?


- Andrew


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


On Dec. 19, 2016, 11:20 p.m., Andrew Schwartzmeyer wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54877/
> -----------------------------------------------------------
> 
> (Updated Dec. 19, 2016, 11:20 p.m.)
> 
> 
> Review request for mesos, Daniel Pravat, Alex Clemmer, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> The API `SHGetKnownFolderPath` requires `Shell32.dll`,
> which is not available on Nano server.
> The equivalent API `GetAllUsersProfileDirectory`
> only requires `Userenv.dll`, which is available on Nano.
> 
> This API is also friendlier, as we own the allocation.
> 
> The Unicode version `GetAllUsersProfileDirectoryW` is
> explicitly used so that we are guaranteed a Unicode path,
> which we then convert from UTF-16 to UTF-8,
> instead of using the ASCII version which depends on a
> varying Windows code-page, and is not recommended.
> 
> A `vector<wchar_t>` is used over a `wstring` to avoid dealing
> with the placement of the null-terminating character.
> 
> 
> Diffs
> -----
> 
>   3rdparty/stout/include/stout/windows.hpp e641c46d033372e1b6c9f9c066b1ad4957d55088 
>   3rdparty/stout/include/stout/windows/os.hpp 5cd92545a49648e39e8eb7cf131895e9cfc97902

> 
> Diff: https://reviews.apache.org/r/54877/diff/
> 
> 
> Testing
> -------
> 
> cmake && msbuild, attach agent to master and check default `runtime_dir` value.
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>


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