-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55242/#review161413
-----------------------------------------------------------
3rdparty/stout/include/stout/os/posix/chown.hpp (lines 36 - 42)
<https://reviews.apache.org/r/55242/#comment232712>
We are handling the situation where we can't `stat` the root. Is it a file/dir permission
concern?
What if we can't `stat` the subdirs and files?
I seems to me that for the two things we are doing:
1) Only descend if the root is a directory and `recursive` is true:
If we just put a
```
if (!recursive && node->fts_level != FTS_ROOTLEVEL) {
break;
}
```
inside the while loop so we only need to call `lchown` from one place.
2) Dealing with issues with `stat` (and other errros)
We need to handle then inside the loop too. Even though `chown` requires a "privledged"
user (unless if you chown to yourself I think). I guess we shouldn't assume this while traversing
the tree and should let the system call fail themselves at appropriate places.
3rdparty/stout/include/stout/os/posix/chown.hpp (line 43)
<https://reviews.apache.org/r/55242/#comment232693>
s/char */char*/
s/paths_/paths/ since there's no `paths`?
3rdparty/stout/include/stout/os/posix/chown.hpp (lines 52 - 53)
<https://reviews.apache.org/r/55242/#comment232694>
`node` should align with `FTSENT` and we should put each expression on a new line.
However the following calls `node = fts_read(tree)` from just one place which I think
is nicer?
```
FTSENT* node;
while ((node = ::fts_read(tree)) != nullptr) {
}
```
3rdparty/stout/include/stout/os/posix/chown.hpp (lines 54 - 56)
<https://reviews.apache.org/r/55242/#comment232695>
Will the inability to `stat` the file result in `FTS_NS`? Do we not need to handle other
fts errors?
If we don't, our `if` condition is going to silently ignore these files and report success.
3rdparty/stout/tests/os_tests.cpp (line 728)
<https://reviews.apache.org/r/55242/#comment232789>
Unfortunately it's a bit tricky to test the permission stuff I guess.
3rdparty/stout/tests/os_tests.cpp (line 746)
<https://reviews.apache.org/r/55242/#comment232783>
s/link/symlink/
3rdparty/stout/tests/os_tests.cpp (line 751)
<https://reviews.apache.org/r/55242/#comment232786>
s/tree/subtree/?
3rdparty/stout/tests/os_tests.cpp (line 752)
<https://reviews.apache.org/r/55242/#comment232787>
Comment that `9` is just an arbitrary uid that we picked?
- Jiang Yan Xu
On Jan. 11, 2017, 4:29 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55242/
> -----------------------------------------------------------
>
> (Updated Jan. 11, 2017, 4:29 p.m.)
>
>
> Review request for mesos, Benjamin Mahler and Jiang Yan Xu.
>
>
> Bugs: MESOS-6862
> https://issues.apache.org/jira/browse/MESOS-6862
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Reimplement os::chown() to use fts(3) rather than sometimes spawning
> chown(1). This removes the use of the shell and the corresponding
> need to sanitize path arguments. It also enables us to provide
> consistent handling of symbolic links for the recursive and
> non-recursive cases. We ensure that symlinks are never followed
> and that we always change the ownership of the link itself, not
> its referent.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/os/posix/chown.hpp c82e2e574019c5ee5f17ea105a6d225006388a45
> 3rdparty/stout/include/stout/os/posix/stat.hpp 1ab20e75fc18b336162b62e2f4f23b68f6685183
> 3rdparty/stout/tests/os_tests.cpp 30735e28a26ff713469711d63538676ed4e327d9
>
> Diff: https://reviews.apache.org/r/55242/diff/
>
>
> Testing
> -------
>
> `sudo make check` (Fedora 25)
>
>
> Thanks,
>
> James Peach
>
>
|