mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Bannier <benjamin.bann...@mesosphere.io>
Subject Re: Review Request 53791: Use the stout ELF parser to collect Linux rootfs files.
Date Tue, 13 Dec 2016 10:13:36 GMT

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



Thanks for looking into this James.

Looks mostly good for me; I was also able to execute tests using the created rootfs under
e.g., fedora-25 which wasn't possible before.

I left some mostly minor comments on style and edge cases.


src/linux/ldd.hpp (lines 31 - 32)
<https://reviews.apache.org/r/53791/#comment229861>

    Please add includes for `string` and `vector`.



src/linux/ldd.cpp (line 32)
<https://reviews.apache.org/r/53791/#comment229864>

    Mesos does not like anon namespaces. Since this namespace e.g., declares no types, you
could just replace its usage with making `collectDependencies` `static` without loss.



src/linux/ldd.cpp (line 37)
<https://reviews.apache.org/r/53791/#comment229874>

    Let's not use an out parameter, but instead return a `Try<hashset<T>`.
    
    Note that this requires making a copy of an in parameter `needed` and to explicitly calculate
the union in below `foreach` loop. In the end the code  might require more copies, but have
clearer side effects.



src/linux/ldd.cpp (line 46)
<https://reviews.apache.org/r/53791/#comment229863>

    `load.isError()`



src/linux/ldd.cpp (line 53)
<https://reviews.apache.org/r/53791/#comment229862>

    nit: 4 spaces.



src/linux/ldd.cpp (line 54)
<https://reviews.apache.org/r/53791/#comment229871>

    `dependencies.isError()`



src/linux/ldd.cpp (line 58)
<https://reviews.apache.org/r/53791/#comment229875>

    nit: space after `foreach`.



src/linux/ldd.cpp (lines 82 - 83)
<https://reviews.apache.org/r/53791/#comment229869>

    Let's move this up right after the check `needed.contains(path)`.
    
    Right now in pathological cases like e.g., a`libA` depending on `libB`, and `libB` depending
on `libA`, we would recurse indefinitely.



src/linux/ldd.cpp (line 90)
<https://reviews.apache.org/r/53791/#comment229877>

    We often don't try hard enough, but since this is new code, could we use `Path` for paths
everywhere here to make semantics clearer?



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

    We also need to check for `realpath.isNone()`.



src/tests/containerizer/rootfs.cpp (lines 64 - 70)
<https://reviews.apache.org/r/53791/#comment229881>

    This will fail to create a working link if `file` is not a direct link to `realpath`,
e.g., if we have multiple levels of links.
    
    This error already existed in the original implementation, could you add a `TODO`, or
even better create a ticket?



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

    +1!



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

    nit: Let's break this line after `Error('.



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

    Not originally yours, but we could just as well use a set (e.g., `set` or `hashset`) here
to express that duplicates are not meaningful.
    
    Here and below.



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

    nit: Not yours, but should be 4 spaces.



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

    Could we move this right above the loop using it?



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

    nit: Not yours, but should be 4 spaces.



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

    Let's break this line after `Error(`.



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

    nit: Not yours, but should be 4 spaces.


- Benjamin Bannier


On Nov. 29, 2016, 2:21 a.m., James Peach wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53791/
> -----------------------------------------------------------
> 
> (Updated Nov. 29, 2016, 2:21 a.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/CMakeLists.txt 0884c6bb6f2ebddadc5cfc6b72d704086709748d 
>   src/Makefile.am 85eda538caf39f81f052896e744b7b0c724f81bb 
>   src/linux/ldd.hpp PRE-CREATION 
>   src/linux/ldd.cpp PRE-CREATION 
>   src/tests/containerizer/rootfs.hpp 6bc3835cbb62536ec933ef38c9e15138b8611e5f 
>   src/tests/containerizer/rootfs.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/53791/diff/
> 
> 
> Testing
> -------
> 
> sudo make check (Fedora 24)
> 
> 
> Thanks,
> 
> James Peach
> 
>


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