mesos-reviews mailing list archives

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53791/#review156259
-----------------------------------------------------------




src/tests/containerizer/rootfs.cpp (line 58)
<https://reviews.apache.org/r/53791/#comment226461>

    This feels like an implementation of `ldd()` that coule be generally useful.
    
    Would it make sense to rename ldcache.hpp|cpp as ld.hpp|cpp and add `ldd()` as a method
in ld.hpp|cpp make sense?



src/tests/containerizer/rootfs.cpp (lines 58 - 61)
<https://reviews.apache.org/r/53791/#comment226462>

    This feels like an intermediate method for recursion. Would be nice to put a front-end
method above it:
    
    ```
    Try<hashset<string>> ldd(const string& path);
    ```



src/tests/containerizer/rootfs.cpp (line 61)
<https://reviews.apache.org/r/53791/#comment226463>

    Using `hashset` would make it more explicit that no order is implied (otherwise this has
to be a vector).
    
    And we can use `contains()`!



src/tests/containerizer/rootfs.cpp (lines 69 - 71)
<https://reviews.apache.org/r/53791/#comment226459>

    It feels like it's not worth extracting out the logic
    
    (similar code from volume.cpp)
    ```
            Try<elf::File*> load = elf::File::load(realpath.get());
            if (load.isError()) {
              return Error("Failed to elf::File::load '" + realpath.get() + "':"
                           " " + load.error());
            }
    
            Owned<elf::File> file(load.get());
    ```
    
    into a 10-line helper method and spend 4 lines here?



src/tests/containerizer/rootfs.cpp (line 73)
<https://reviews.apache.org/r/53791/#comment226460>

    Turn it into a hashset for easier checking?



src/tests/containerizer/rootfs.cpp (lines 81 - 84)
<https://reviews.apache.org/r/53791/#comment226458>

    If we have to do this, format it this way
    
    ```
    auto entry = std::find_if(
        cache.begin(),
        cache.end(),
        [dependency](const ldcache::Entry& e) {
          return strings::startsWith(e.name, dependency);
        });
    ```
    
    But is prefix matching necessary? Can't we do extract matching (i.e.,`hashset.contains()`)?
    
    Note that our seed list are just programs without versions.



src/tests/containerizer/rootfs.cpp (line 82)
<https://reviews.apache.org/r/53791/#comment226457>

    No space.



src/tests/containerizer/rootfs.cpp (line 107)
<https://reviews.apache.org/r/53791/#comment226466>

    Not convinced that we need to extract it out since it's used only in a single place?



src/tests/containerizer/rootfs.cpp (line 196)
<https://reviews.apache.org/r/53791/#comment226438>

    The following is more widely used.
    
    ```
    #ifdef __linux__
    ```



src/tests/containerizer/rootfs.cpp (lines 196 - 201)
<https://reviews.apache.org/r/53791/#comment226464>

    The whole file should just be linux only so we don't have to do it here.



src/tests/containerizer/rootfs.cpp (line 223)
<https://reviews.apache.org/r/53791/#comment226465>

    Would the following look better?
    
    ```
    hashset<string> needed(programs.begin(), programs.end());
    
    foreach (const string& program, programs) {
      Try<hashset<string>> dependencies = ldd(file);
    
      // error handling.
    
      needed = needed | dependencies;
    }
    ```


- Jiang Yan Xu


On Nov. 15, 2016, 3:34 p.m., James Peach wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53791/
> -----------------------------------------------------------
> 
> (Updated Nov. 15, 2016, 3:34 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, Kevin Klues, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6588
>     https://issues.apache.org/jira/browse/MESOS-6588
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> The dependencies for the programs we need in the Linux root
> filesystem vary over time and across distributions. Since Stout
> has support for parsing the library dependencies of ELF binaries,
> use this to collect the necessary dependencies when constructing
> a root filesystem for the tests.
> 
> 
> Diffs
> -----
> 
>   src/tests/containerizer/rootfs.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/53791/diff/
> 
> 
> Testing
> -------
> 
> sudo make check (Fedora 24)
> 
> 
> Thanks,
> 
> James Peach
> 
>