You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Jiang Yan Xu <xu...@apple.com> on 2017/01/31 23:30:19 UTC
Re: Review Request 53791: Use the stout ELF parser to collect Linux
rootfs files.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53791/#review163720
-----------------------------------------------------------
src/tests/containerizer/rootfs.hpp (line 21)
<https://reviews.apache.org/r/53791/#comment235203>
We have mixed instances of sentences with and without periods following `#error` but in general we omit the final period "if it's code" so remove it to follow the general rule? We can clean up the other inconsistent instances.
src/tests/containerizer/rootfs.hpp (line 39)
<https://reviews.apache.org/r/53791/#comment235235>
The word `directory` is still in the comment but you removed the logic that adds directories from the implementation?
src/tests/containerizer/rootfs.hpp (line 42)
<https://reviews.apache.org/r/53791/#comment235208>
Why remove `const &`?
src/tests/containerizer/rootfs.hpp (line 50)
<https://reviews.apache.org/r/53791/#comment235206>
s/copyPath/copy/? `path` already appears in the argument so no need to duplicate?
src/tests/containerizer/rootfs.cpp (lines 21 - 25)
<https://reviews.apache.org/r/53791/#comment235205>
We generally order things this way:
```
#include <stout/error.hpp>
#include <stout/foreach.hpp>
#include <stout/os.hpp>
#include <stout/strings.hpp>
#include <stout/os/stat.hpp>
```
The stuff with three-level directory structure are put below the ones with two.
src/tests/containerizer/rootfs.cpp (line 46)
<https://reviews.apache.org/r/53791/#comment235207>
Why use a different argument name `file` as opposed to `path` in the declaration?
src/tests/containerizer/rootfs.cpp (line 52)
<https://reviews.apache.org/r/53791/#comment235209>
Can we maintain a variable different from the input arugment for this?
src/tests/containerizer/rootfs.cpp (lines 55 - 57)
<https://reviews.apache.org/r/53791/#comment235213>
I can see the awkward situtation we are in: we call a helper method and the errno is hidden by it. Now we have to break the encapsulation and reconstruct it...
However it's probably best to avoid it if possible. In this case, I feel
```
Error("Failed to resolve '" + file + "');
```
already provides enough information so stringifying `ENOENT` doesn't buy us much?
src/tests/containerizer/rootfs.cpp (lines 61 - 63)
<https://reviews.apache.org/r/53791/#comment235216>
This fits in one line?
```
return Error("Failed to resolve '" + file + "': " + realpath.error());
```
If it didn't:
```
return Error(
"Failed to resolve '" + file + "': " + realpath.error());
```
In general, we break down further when it's mutliple arguments one per each line, like
```
Error(
arg1,
arg2,
arg3);
```
This is string continuation.
src/tests/containerizer/rootfs.cpp (line 62)
<https://reviews.apache.org/r/53791/#comment235214>
s/' :/':/
src/tests/containerizer/rootfs.cpp (line 66)
<https://reviews.apache.org/r/53791/#comment235218>
s/result/Try<Nothing> copy/?
We don't really need a method-level `result`. We already short-circuit whenever any error occurs. We don't need to save it.
src/tests/containerizer/rootfs.cpp (line 73)
<https://reviews.apache.org/r/53791/#comment235220>
Seems that `realpath()` leads to the eventual "real" path even with multiple symlinks like "link2 -> link1 -> real"? We don't need an iterative algorithm?
A related question is, if so, with "link2 -> link1 -> real", how do we copy `link1` over?
src/tests/containerizer/rootfs.cpp (line 87)
<https://reviews.apache.org/r/53791/#comment235238>
s/std::string/string/ here and elsewhere in this file?
src/tests/containerizer/rootfs.cpp (line 90)
<https://reviews.apache.org/r/53791/#comment235221>
I would still prefer a simple sentense "Path doesn't exist" so we don't break the encapsulation of `os::exists()`. I know it's a simple helper but it's a good principle IMO to just interpret what the API gives you. If the API is inadequate, we can change it.
src/tests/containerizer/rootfs.cpp
<https://reviews.apache.org/r/53791/#comment235224>
Why are we getting rid of the ability to add directories?
src/tests/containerizer/rootfs.cpp (line 114)
<https://reviews.apache.org/r/53791/#comment235237>
Indentation: the original looks better to me. This feels as if we were continuing the `if` statement and not the method call inside.
src/tests/containerizer/rootfs.cpp (line 147)
<https://reviews.apache.org/r/53791/#comment235226>
s/needed/files/ and use `|=` below?
src/tests/containerizer/rootfs.cpp (line 149)
<https://reviews.apache.org/r/53791/#comment235225>
s/foreach/foreach /
src/tests/containerizer/rootfs.cpp (line 157)
<https://reviews.apache.org/r/53791/#comment235233>
Use `files` here?
```
files |= dependencies.get();
```
src/tests/containerizer/rootfs.cpp (line 167)
<https://reviews.apache.org/r/53791/#comment235231>
I can see that you want to distinguish programs from non-programs but programs are also files? How about s/files/configs/?
src/tests/containerizer/rootfs.cpp (lines 171 - 176)
<https://reviews.apache.org/r/53791/#comment235232>
Can we avoid having a duplicate code block by just process the union together? i.e.,
```
files |= configs;
foreach (const string& file, files) {} // Move down the identical block from above.
```
- Jiang Yan Xu
On Dec. 19, 2016, 3:24 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53791/
> -----------------------------------------------------------
>
> (Updated Dec. 19, 2016, 3:24 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/Makefile.am 0f62ec70816e8b48e19d35036285656a6e7cd02b
> 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
>
>