mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <benjamin.mah...@gmail.com>
Subject Re: Review Request 47482: Added preliminary support for parsing ELF files in stout.
Date Thu, 19 May 2016 04:04:05 GMT

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


Fix it, then Ship it!





3rdparty/stout/configure.ac (line 154)
<https://reviews.apache.org/r/47482/#comment198484>

    s/posix//



3rdparty/stout/include/stout/elf.hpp (lines 20 - 33)
<https://reviews.apache.org/r/47482/#comment198523>

    Looks like we need to audit these?



3rdparty/stout/include/stout/elf.hpp (line 69)
<https://reviews.apache.org/r/47482/#comment198524>

    Hm.. could we move this down towards where it is first used?



3rdparty/stout/include/stout/elf.hpp (line 71)
<https://reviews.apache.org/r/47482/#comment198526>

    Can we document what this does?



3rdparty/stout/include/stout/elf.hpp (line 80)
<https://reviews.apache.org/r/47482/#comment198525>

    We should avoid logging the caller-available information here as it leads to redundant
logging.



3rdparty/stout/include/stout/elf.hpp (line 91)
<https://reviews.apache.org/r/47482/#comment198527>

    Ditto on excluding the path here.



3rdparty/stout/include/stout/elf.hpp (line 94)
<https://reviews.apache.org/r/47482/#comment198503>

    `s/ */* /` here and elsewhere



3rdparty/stout/include/stout/elf.hpp (lines 94 - 106)
<https://reviews.apache.org/r/47482/#comment198504>

    Hm.. could we use some unabbreviated variable names here and elsewhere? E.g. 'section',
'section_header', 'section_type', etc. Since the type names are opaque that would help the
reader.



3rdparty/stout/include/stout/elf.hpp (line 104)
<https://reviews.apache.org/r/47482/#comment198528>

    Hm.. emplace here seems a bit odd since there's no need to avoid copying for pointer types.
Let's avoid emplace for now overall since we're not doing any large copying here, we can optimize
it later.



3rdparty/stout/include/stout/elf.hpp (line 134)
<https://reviews.apache.org/r/47482/#comment198529>

    const for these member functions?



3rdparty/stout/include/stout/elf.hpp (line 137)
<https://reviews.apache.org/r/47482/#comment198530>

    Why cast instead of using the Class enum value?



3rdparty/stout/include/stout/elf.hpp (line 138)
<https://reviews.apache.org/r/47482/#comment198531>

    Let's do an audit to match the names in the log messages: 'gelf_getclass' here for example.



3rdparty/stout/include/stout/elf.hpp (line 156)
<https://reviews.apache.org/r/47482/#comment198532>

    We've started to use snake_case in stout and libprocess (we used to, then started using
camelCase, and now we've moved back), it's unfortunately inconsistent so I would be fine either
way here. If you don't mind we can switch all the variables here to use snake_case.



3rdparty/stout/include/stout/elf.hpp (line 169)
<https://reviews.apache.org/r/47482/#comment198533>

    Looks like we should make this an Option to catch the case where the loop completes without
finding a pointer.


- Benjamin Mahler


On May 18, 2016, 8:20 p.m., Kevin Klues wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47482/
> -----------------------------------------------------------
> 
> (Updated May 18, 2016, 8:20 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-5400
>     https://issues.apache.org/jira/browse/MESOS-5400
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Right now we are able to parse ELF formatted shared libraries and
> extract their canonical SONAME and external library dependencies. In
> the future, we should add support for fully parsing an ELf file for
> easy access to all of its contents.
> 
> The current implementation relies on libelf. We should probably remove
> this dependency in future versions (mostly since the headers for
> libelf are not installed on a standard Linux distribution by default).
> 
> 
> Diffs
> -----
> 
>   3rdparty/stout/configure.ac ada1e22c72875fe9d557f07e4846128be0bcef13 
>   3rdparty/stout/include/stout/elf.hpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/47482/diff/
> 
> 
> Testing
> -------
> 
> The test for this is actually in a follow-on patch for testing ld.so.cache parsing. The
test itself is run with:
> ```
> GTEST_FILTER="LdcacheTest.Parse" make check -j
> ```
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>


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