You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by James Peach <jp...@apache.org> on 2016/12/13 18:35:43 UTC
Review Request 54712: Use the stout ELF parser to implement ldd.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54712/
-----------------------------------------------------------
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
Re: Review Request 54712: Use the stout ELF parser to implement ldd.
Posted by Jiang Yan Xu <xu...@apple.com>.
> On Dec. 15, 2016, 6:47 a.m., Jiang Yan Xu wrote:
> > src/linux/ldd.cpp, line 50
> > <https://reviews.apache.org/r/54712/diff/1/?file=1582969#file1582969line50>
> >
> > `s/dependencies/_dependencies/`?
> >
> > I would say `needed` is good variable name here but `_dependencies` and `dependency` below is obviously a good pair.
>
> James Peach wrote:
> I pretty strongly disagree that `_dependencies` is a good name, but it is a pattern that is often used in Mesos. I'll just do that.
It's a trick to name a couple of variables that mean the same thing. It's sometimes too hard to come up with different names that all mean the same thing but also immediately distinguishable. Even if I just use a bunch of synonyms I don't know if it's easier to tell which is which? I don't think there's any hard and fast rule here so what would you have preferred?
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54712/#review159106
-----------------------------------------------------------
On Dec. 19, 2016, 3:23 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54712/
> -----------------------------------------------------------
>
> (Updated Dec. 19, 2016, 3:23 p.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 c8d4260c03d8cdee1951a50d293e9fdabcd2cf84
> src/Makefile.am 0f62ec70816e8b48e19d35036285656a6e7cd02b
> 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
>
>
Re: Review Request 54712: Use the stout ELF parser to implement ldd.
Posted by James Peach <jp...@apache.org>.
> On Dec. 15, 2016, 2:47 p.m., Jiang Yan Xu wrote:
> > src/linux/ldd.cpp, line 50
> > <https://reviews.apache.org/r/54712/diff/1/?file=1582969#file1582969line50>
> >
> > `s/dependencies/_dependencies/`?
> >
> > I would say `needed` is good variable name here but `_dependencies` and `dependency` below is obviously a good pair.
I pretty strongly disagree that `_dependencies` is a good name, but it is a pattern that is often used in Mesos. I'll just do that.
- James
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54712/#review159106
-----------------------------------------------------------
On Dec. 19, 2016, 11:23 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54712/
> -----------------------------------------------------------
>
> (Updated Dec. 19, 2016, 11:23 p.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 c8d4260c03d8cdee1951a50d293e9fdabcd2cf84
> src/Makefile.am 0f62ec70816e8b48e19d35036285656a6e7cd02b
> 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
>
>
Re: Review Request 54712: Use the stout ELF parser to implement ldd.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
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
>
>
Re: Review Request 54712: Use the stout ELF parser to implement ldd.
Posted by James Peach <jp...@apache.org>.
> On Feb. 2, 2017, 12:04 a.m., Jiang Yan Xu wrote:
> > src/linux/ldd.hpp, line 30
> > <https://reviews.apache.org/r/54712/diff/2/?file=1589105#file1589105line30>
> >
> > Sorry it only occured to me after the review but can we remove the given `path` from the result set to match `ldd` semantics?
> >
> > Yes this method was originally not `ldd` and it doesn't have to be. But for something that's "pretty much" ldd, if we implement it as `ldd` and call it `ldd`, it's much easier for people to understand, right?
> >
> > :)
Done.
- James
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54712/#review163904
-----------------------------------------------------------
On Feb. 2, 2017, 12:15 a.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54712/
> -----------------------------------------------------------
>
> (Updated Feb. 2, 2017, 12:15 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 09ef1aee680c6b91476092dbf61d4ca3a8d256bb
> src/Makefile.am 6c9be54fe0ce3faaa26e2f090773d1b77bb6e430
> 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
>
>
Re: Review Request 54712: Use the stout ELF parser to implement ldd.
Posted by Jiang Yan Xu <xu...@apple.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54712/#review163904
-----------------------------------------------------------
src/linux/ldd.hpp (line 30)
<https://reviews.apache.org/r/54712/#comment235418>
Sorry it only occured to me after the review but can we remove the given `path` from the result set to match `ldd` semantics?
Yes this method was originally not `ldd` and it doesn't have to be. But for something that's "pretty much" ldd, if we implement it as `ldd` and call it `ldd`, it's much easier for people to understand, right?
:)
- Jiang Yan Xu
On Dec. 19, 2016, 3:23 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54712/
> -----------------------------------------------------------
>
> (Updated Dec. 19, 2016, 3:23 p.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 c8d4260c03d8cdee1951a50d293e9fdabcd2cf84
> src/Makefile.am 0f62ec70816e8b48e19d35036285656a6e7cd02b
> 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
>
>
Re: Review Request 54712: Use the stout ELF parser to implement ldd.
Posted by Jiang Yan Xu <xu...@apple.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54712/#review163557
-----------------------------------------------------------
src/linux/ldd.cpp (lines 29 - 32)
<https://reviews.apache.org/r/54712/#comment235116>
With the iterative approach I don't see the need to pass the `dependencies` hashset in?
Maybe we don't even need `collectDependencies`, everything can be put inside `ldd`.
src/linux/ldd.cpp (line 34)
<https://reviews.apache.org/r/54712/#comment235118>
Can we define what a `candidate` means here, espeically when it's alongside the locally defined `dependencies`? Like:
```
hashset<string> dependencies;
// Dependencies that have been discovered but yet to be validated and processed.
vector<string> candidates;
```
src/linux/ldd.cpp (lines 40 - 42)
<https://reviews.apache.org/r/54712/#comment235119>
If we are traversing in a particular order, say BFS, maybe pick from `front()`?
Also a brief comment stating that it is BFS?
src/linux/ldd.cpp (line 74)
<https://reviews.apache.org/r/54712/#comment235121>
s/was/is/?
- Jiang Yan Xu
On Dec. 19, 2016, 3:23 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54712/
> -----------------------------------------------------------
>
> (Updated Dec. 19, 2016, 3:23 p.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 c8d4260c03d8cdee1951a50d293e9fdabcd2cf84
> src/Makefile.am 0f62ec70816e8b48e19d35036285656a6e7cd02b
> 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
>
>
Re: Review Request 54712: Use the stout ELF parser to implement ldd.
Posted by Jiang Yan Xu <xu...@apple.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54712/#review164169
-----------------------------------------------------------
Ship it!
Ship It!
- Jiang Yan Xu
On Feb. 1, 2017, 4:15 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54712/
> -----------------------------------------------------------
>
> (Updated Feb. 1, 2017, 4:15 p.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 09ef1aee680c6b91476092dbf61d4ca3a8d256bb
> src/Makefile.am 6c9be54fe0ce3faaa26e2f090773d1b77bb6e430
> 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
>
>
Re: Review Request 54712: Use the stout ELF parser to implement ldd.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54712/
-----------------------------------------------------------
(Updated Feb. 2, 2017, 12:15 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 (updated)
-----
src/CMakeLists.txt 09ef1aee680c6b91476092dbf61d4ca3a8d256bb
src/Makefile.am 6c9be54fe0ce3faaa26e2f090773d1b77bb6e430
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
Re: Review Request 54712: Use the stout ELF parser to implement ldd.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54712/
-----------------------------------------------------------
(Updated Dec. 19, 2016, 11:23 p.m.)
Review request for mesos, Jie Yu, Kevin Klues, and Jiang Yan Xu.
Changes
-------
Rebase and address review feedback.
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 (updated)
-----
src/CMakeLists.txt c8d4260c03d8cdee1951a50d293e9fdabcd2cf84
src/Makefile.am 0f62ec70816e8b48e19d35036285656a6e7cd02b
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