> On Dec. 8, 2016, 6:45 a.m., Alex Clemmer wrote: > > 3rdparty/stout/include/stout/posix/os.hpp, line 470 > > <https://reviews.apache.org/r/54514/diff/1/?file=1579536#file1579536line470> > > > > It looks like you have decided not to take Jie's suggestion in [1], to > > implement this as a family of functions, `os::runstatedir`, `os:: > > localstatedir`, _etc_.? > > > > That's fine if so, I bring it up just to make sure that we're > > consciously making this decision to not do this, rather than accidentally > > letting it slip through the cracks. It looks like the vast majority of > > places we're putting something in `/var/run`, we're actually putting it in > > `/var/run/mesos`, so the utility of that abstraction is probably not > > particularly high. > > > > [1] https://reviews.apache.org/r/54335/
I like Jie's suggestion, but it would only apply to `os::var` -> `os::localstatedir`, since `os::runtime_dir` != `os::runstatedir`, as it includes the `mesos` (or `mesos/runtime` on Windows) suffix. It is this way for the exact reason you point out; we never use `/var/run` directly, only `/var/run/mesos`, and I primarily refactored this to ensure the fallback `/tmp/mesos/runtime` would happen consistently (and not live in the CLI code). We ought to implement GNU Coding Standards with respect to directory naming, but we should go all the way with it instead of just partially implement it here, as they extend to other directories too. So that set of changes should be made separately. > On Dec. 8, 2016, 6:45 a.m., Alex Clemmer wrote: > > 3rdparty/stout/include/stout/posix/os.hpp, line 475 > > <https://reviews.apache.org/r/54514/diff/1/?file=1579536#file1579536line475> > > > > Why call `os::var` a second time here? Did you mean for this to be > > simply `var.get()`? Here, and in the Windows version. Oops. Good catch. > On Dec. 8, 2016, 6:45 a.m., Alex Clemmer wrote: > > 3rdparty/stout/include/stout/posix/os.hpp, line 476 > > <https://reviews.apache.org/r/54514/diff/1/?file=1579536#file1579536line476> > > > > Do you think we should be adding `#include <stout/os/access.hpp>` to > > this and the Windows version make this header standalone? Now that you mention it, yes. > On Dec. 8, 2016, 6:45 a.m., Alex Clemmer wrote: > > 3rdparty/stout/include/stout/posix/os.hpp, line 477 > > <https://reviews.apache.org/r/54514/diff/1/?file=1579536#file1579536line477> > > > > I'm wondering if someone can speak to how we expect `runtime_dir` to be > > used here. If we expect that we're going to `su` or something after we get > > `runtime_dir` back, then it might be the case that either the user we're > > switching to, or from, doesn't have access. If that is the case, should we > > be checking this here, or after we get the path back? I, too, would like this information. But I'll note that logically this is consistent with the previous code, which ran the `user == "root"` check at the same time. - Andrew ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54514/#review158504 ----------------------------------------------------------- On Dec. 8, 2016, 1:59 a.m., Andrew Schwartzmeyer wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54514/ > ----------------------------------------------------------- > > (Updated Dec. 8, 2016, 1:59 a.m.) > > > Review request for mesos and Alex Clemmer. > > > Bugs: MESOS-6722 > https://issues.apache.org/jira/browse/MESOS-6722 > > > Repository: mesos > > > Description > ------- > > Encapsulates the platform-specific runtime directories > `/var/run/mesos` and `C:\ProgramData\mesos\runtime`. > Checks for read/write access to `/var/run` and `C:\ProgramData`, > and falls back to `os::temp()/mesos/runtime` on error. > > The `os::runtime_dir()` function is introduced to handle both the > platform-specificity of the the runtime data folders and the > error handling in the case of the default folder not being writable. > Note that previously the CLI code checked the usability of > `/var/run/mesos` on POSIX by checking `if (user == "root")` > instead of testing the read and write access of `/var/run`. > This made it so that Mesos would use `/tmp/mesos/runtime` > if it was run as any user other than `root`, > even if that user had write access to `/var/run`. > Checking for permission instead of checking a username > is the preferred way to handle permission checks. > > > Diffs > ----- > > 3rdparty/stout/include/stout/posix/os.hpp > 8443aa0cf0a8d8d52e36282611c2ab15ca4dd354 > 3rdparty/stout/include/stout/windows/os.hpp > 2f20ccc64e255a60a1b7f33d684969942f12e45f > > Diff: https://reviews.apache.org/r/54514/diff/ > > > Testing > ------- > > make && make check on Linux: no failures. > msbuild and attach to a master on Windows: no failures. > > > Thanks, > > Andrew Schwartzmeyer > >
