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