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 54712: Use the stout ELF parser to implement ldd.
Date Thu, 15 Dec 2016 14:47:11 GMT

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



We should have a unit test for this in ldd_tests.cpp. :)

Even though the point of this utility is so we don't have to have a fixed and brittle list,
at least we can verify `libc.so.6` is in the result set and if `path` doesn't exist the result
is an Error?


src/linux/ldd.hpp (lines 28 - 29)
<https://reviews.apache.org/r/54712/#comment230072>

    Since this is a utility, no need to put it in `mesos::internal`. I think it's fine to
be not in a namespace.
    
    Alternatively,
    
    ```
    namespace ldd {
      Try<hashset<std::string>> load(
          const std::string& path,
          const std::vector<ldcache::Entry>& cache);
    }
    ```
    
    looks fine too.



src/linux/ldd.cpp (lines 24 - 25)
<https://reviews.apache.org/r/54712/#comment230270>

    With these using declarations, let's use unqualified `string` and `vector` in this file.



src/linux/ldd.cpp (line 35)
<https://reviews.apache.org/r/54712/#comment230209>

    I understand that `needed` comes from `DynamicTag::NEEDED` but here as the name of the
"out" parameter for the method `collectDependencies` this is not obvious. How about s/needed/dependencies/?



src/linux/ldd.cpp (lines 37 - 41)
<https://reviews.apache.org/r/54712/#comment230268>

    As noted by @bbannier, we are searching through a dependency graph (not necessarily a
tree). The algorithm for DFS on a directed graph is pretty well-defined so I'd say we just
implement that: https://en.wikipedia.org/wiki/Depth-first_search
    
    To avoid stuck in a cycle infinitely we indeed need to mark the visited node first before
recursing further.
    
    Our "marking a node as visited" code is just `needed.insert(path);`.
    
    We don't need this terminate condition:
    
    ```
    if (needed.contains(path)) {
      return Nothing();
    }
    ```
    
    Instead, if we have traversed all nodes we'll find no new nodes to DFS into (i.e., `collectDependencies()`)
and the recursion will go back up.



src/linux/ldd.cpp (line 50)
<https://reviews.apache.org/r/54712/#comment230210>

    `s/dependencies/_dependencies/`?
    
    I would say `needed` is good variable name here but `_dependencies` and `dependency` below
is obviously a good pair.



src/linux/ldd.cpp (line 51)
<https://reviews.apache.org/r/54712/#comment230083>

    2 spaces.



src/linux/ldd.cpp (lines 86 - 87)
<https://reviews.apache.org/r/54712/#comment230073>

    Your declaration is using the following style (which is more normal)
    
    ```
    Try<hashset<string>> ldd(
        const string& path,
        const vector<ldcache::Entry>& cache)
    ```
    
    so let's use the same style here.
    
    Your style here would have been fine if the return type and the method name together was
already too long.


- Jiang Yan Xu


On Dec. 13, 2016, 10:35 a.m., James Peach wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54712/
> -----------------------------------------------------------
> 
> (Updated Dec. 13, 2016, 10:35 a.m.)
> 
> 
> Review request for mesos, Jie Yu, Kevin Klues, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6588
>     https://issues.apache.org/jira/browse/MESOS-6588
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Use the stout ELF parser to implement an ldd() API that recursively
> gathers the link dependencies of a given program.
> 
> 
> Diffs
> -----
> 
>   src/CMakeLists.txt 0cc0451ae3ca0183da3d575cc4bd3c5b3ea30ecc 
>   src/Makefile.am a4c03c2b918816e6dd8872d37e5208f055619c47 
>   src/linux/ldd.hpp PRE-CREATION 
>   src/linux/ldd.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/54712/diff/
> 
> 
> Testing
> -------
> 
> sudo make check (Fedora 25)
> 
> 
> Thanks,
> 
> James Peach
> 
>


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