----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54515/#review159393 -----------------------------------------------------------
Ship it! src/slave/flags.cpp (line 226) <https://reviews.apache.org/r/54515/#comment230389> Nit: We close `#ifdef`s like `#endif // __WINDOWS__`. src/slave/flags.cpp (lines 227 - 230) <https://reviews.apache.org/r/54515/#comment230388> You might have overlooked the ending `()` in `[]() -> string {}()` when writing this comment :) The default value for `--runtime_dir` is only calculated once per instantiation of the flag object. Not once per access of the `runtime_dir` field. I'll reword this comment to make it more accurate. - Joseph Wu On Dec. 15, 2016, 4:08 p.m., Andrew Schwartzmeyer wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54515/ > ----------------------------------------------------------- > > (Updated Dec. 15, 2016, 4:08 p.m.) > > > Review request for mesos, Alex Clemmer and Joseph Wu. > > > Bugs: MESOS-6722 > https://issues.apache.org/jira/browse/MESOS-6722 > > > Repository: mesos > > > Description > ------- > > This commit improves the logic of `Flags::runtime_dir` to use > `os::var()` for encapsulation of `/var` vs `C:\ProgramData`. > > However, because the suffixes are different (`/run/mesos` versus > `/mesos/runtime`), the CLI still has some platform-specific code. We > chose to keep this logic in the CLI so that it is set once, and users > can continue to consume `Flags::runtime_dir` instead of something like > `os::runtime_dir()`. This is primarily due to the fact that we have two > default paths: a system path (in `/var/run` and `ProgramData`) and a > fallback path in `/tmp` if the `os::access()` check fails. We > specifically want to avoid the situation where `os::access()` is called > multiple times due the use of `su` throughout Mesos. The `runtime_dir` > must not change after it has been set. > > The permission check for the system default path is fixed to check > actual access permissions to the directory in which Mesos creates its > runtime directory, instead of assuming permissions based on a comparison > of `os::user() == "root"`. > > Stylistically we chose a bit of duplication as a trade-off for enhanced > readability and auto-formatting to work properly. > > > Diffs > ----- > > src/slave/flags.cpp a1dfbf934ef7d44cdb019aae826c90caaf477775 > > Diff: https://reviews.apache.org/r/54515/diff/ > > > Testing > ------- > > make && make check on Linux: no failures. > msbuild and attach to a master on Windows: no failures. > > Checked that running the agent as non-root on Linux *without* read/write > permissions to `/var/run` > correctly fell back to `/tmp/mesos/runtime`. > > Checked that running the agent as non-administrator on Windows correctly fell > back to `os::temp()/mesos/runtime`. > > Checked that running as `root` on Linux and `Administrator` on Windows > chose the correct default `runtime_dir` paths. > > > Thanks, > > Andrew Schwartzmeyer > >
