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 54514: Add `os::runtime_dir()` to Stout.
Date Thu, 08 Dec 2016 19:32:13 GMT


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


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