Re: Review Request 53706: Implemented `os::user' on Windows.

[Joseph Wu <jo...@mesosphere.io>]

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




3rdparty/stout/include/stout/os/windows/su.hpp (line 30)
<https://reviews.apache.org/r/53706/#comment230370>

    It would be nice to have a comment here explaining:
    
    * We include `Security.h` for the `GetUserNameEx` method included by `SecExt.h`.  Comments in `SecExt.h` note that `SecExt.h` should only be included via `Security.h`.
    * In order to include `SecExt.h`, we must define either `SECURITY_WIN32` or `SECURITY_KERNEL`.  These determine whether the methods are usermode or kernel operations.  We don't need the kernel level permissions.



3rdparty/stout/include/stout/os/windows/su.hpp (line 33)
<https://reviews.apache.org/r/53706/#comment230365>

    Instead of adding a library here, can't we add this to the `STOUT_LIBS` variable?



3rdparty/stout/include/stout/os/windows/su.hpp (line 63)
<https://reviews.apache.org/r/53706/#comment230362>

    Let's use a `while (true)` instead.  Also see the next issue.



3rdparty/stout/include/stout/os/windows/su.hpp (lines 66 - 70)
<https://reviews.apache.org/r/53706/#comment230371>

    According to https://msdn.microsoft.com/en-us/library/windows/desktop/ms724435(v=vs.85).aspx
    
    When this error value is set:
    > The lpNameBuffer buffer is too small. The lpnSize parameter contains the number of bytes required to receive the name.
    
    That means you only need to resize the buffer once.  This also means you can remove the loop.



3rdparty/stout/include/stout/os/windows/su.hpp (line 71)
<https://reviews.apache.org/r/53706/#comment230372>

    Instead of effectively calling `::GetLastError` twice (once directly, once via `WindowsError`), just save the value of `WindowsError` and access the code via `.code`.
    
    Also, it would be nice to include some more text in the error body.



3rdparty/stout/include/stout/os/windows/su.hpp (lines 76 - 77)
<https://reviews.apache.org/r/53706/#comment230373>

    Can the `auto ret` be inlined into the second line?
    
    Also, what's the importance behind replacing backslashes with dots?


- Joseph Wu


On Nov. 29, 2016, 11:54 p.m., Daniel Pravat wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53706/
> -----------------------------------------------------------
> 
> (Updated Nov. 29, 2016, 11:54 p.m.)
> 
> 
> Review request for mesos, Alex Naparu, Artem Harutyunyan, Alex Clemmer, Joseph Wu, and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Implemented `os::user' on Windows.
> 
> 
> Diffs
> -----
> 
>   3rdparty/stout/include/stout/os/windows/su.hpp 1bb70964adbb80aa6502fbfe69de2c34dc74e655 
> 
> Diff: https://reviews.apache.org/r/53706/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Daniel Pravat
> 
>