mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alex Clemmer <clemmer.alexan...@gmail.com>
Subject Re: Review Request 54335: Add os::var() to stout.
Date Mon, 05 Dec 2016 19:30:24 GMT

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


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