mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jiang Yan Xu <...@jxu.me>
Subject Re: Review Request 55242: Stop using os::system to chown a directory hierarchy.
Date Fri, 13 Jan 2017 08:27:25 GMT

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


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