Re: Review Request 47482: Added preliminary support for parsing ELF files in stout.

2016-05-18 Thread Benjamin Mahler 

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


s/posix//



3rdparty/stout/include/stout/elf.hpp (lines 20 - 33)


Looks like we need to audit these?



3rdparty/stout/include/stout/elf.hpp (line 69)


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



3rdparty/stout/include/stout/elf.hpp (line 71)


Can we document what this does?



3rdparty/stout/include/stout/elf.hpp (line 80)


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



3rdparty/stout/include/stout/elf.hpp (line 91)


Ditto on excluding the path here.



3rdparty/stout/include/stout/elf.hpp (line 94)


`s/ */* /` here and elsewhere



3rdparty/stout/include/stout/elf.hpp (lines 94 - 106)


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)


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)


const for these member functions?



3rdparty/stout/include/stout/elf.hpp (line 137)


Why cast instead of using the Class enum value?



3rdparty/stout/include/stout/elf.hpp (line 138)


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)


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)


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

Re: Review Request 47482: Added preliminary support for parsing ELF files in stout.

2016-05-18 Thread Kevin Klues 


> On May 18, 2016, 5:35 p.m., Jojy Varghese wrote:
> > 3rdparty/stout/include/stout/elf.hpp, line 56
> > 
> >
> > Wondering why the implementation is in the header file.

For better or worse, stout is a header-only library.  The implementation of all 
classes / functions are in header files.


> On May 18, 2016, 5:35 p.m., Jojy Varghese wrote:
> > 3rdparty/stout/include/stout/elf.hpp, line 63
> > 
> >
> > Its recommended to use a smart pointer here. They provide the RAII 
> > property that you need here. Also, the pattern in Mesos is to use factory 
> > methods that return a `Owned`.

`Owner` doesn't exist in stout. As I mentioned in response to your comment 
above about "Dont we have to close the open file in error cases?", I tried 
using a `std::unique_ptr` here, but it doesn't play nicely with `Try<>`. If you 
have any other suggestions, I'd love to hear them.  Maybe it's worth adding a 
comment in the code somehwere about the limitations I encountered.


> On May 18, 2016, 5:35 p.m., Jojy Varghese wrote:
> > 3rdparty/stout/include/stout/elf.hpp, line 188
> > 
> >
> > The advantage of emplace_back here would be that the `string` object 
> > will be created only once as opposed to being first created and then copied.

OK, good to know.


- Kevin


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


On May 18, 2016, 3:16 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47482/
> ---
> 
> (Updated May 18, 2016, 3:16 a.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
> ---
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 47482: Added preliminary support for parsing ELF files in stout.

2016-05-18 Thread Jojy Varghese 

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




3rdparty/stout/include/stout/elf.hpp (line 56)


Wondering why the implementation is in the header file.



3rdparty/stout/include/stout/elf.hpp (line 63)


Its recommended to use a smart pointer here. They provide the RAII property 
that you need here. Also, the pattern in Mesos is to use factory methods that 
return a `Owned`.



3rdparty/stout/include/stout/elf.hpp (line 188)


The advantage of emplace_back here would be that the `string` object will 
be created only once as opposed to being first created and then copied.


- Jojy Varghese


On May 18, 2016, 3:16 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47482/
> ---
> 
> (Updated May 18, 2016, 3:16 a.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
> ---
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 47482: Added preliminary support for parsing ELF files in stout.

2016-05-17 Thread Kevin Klues 


> On May 17, 2016, 10:23 p.m., Jojy Varghese wrote:
> > 3rdparty/stout/include/stout/elf.hpp, line 77
> > 
> >
> > Dont we have to close the open file in error cases?

What I actually needed to be doing was deleting the File object on every error 
path.  This implicitly calls `close()` and `elf_end()` when needed as well as 
deletes the memory for the object. I was originally returning a `Try` 
rather than a `Try`, so this destruction  was happening automatically as 
the object went out of scope.  This has some obviousl problems of it's own, 
which is why I moved to the pointer model.

I really wanted to return a `Try` here, but `Try` has 
problems with the missing copy consructor on `unique_ptr`, so I had to settle 
for a normal pointer.  Unfortunately, this means that I have to manually delete 
this object at the call site, but I don't think that's much of a problem in 
this particular case (we will very rarely be working with these objects).


> On May 17, 2016, 10:23 p.m., Jojy Varghese wrote:
> > 3rdparty/stout/include/stout/elf.hpp, line 94
> > 
> >
> > Maybe break out after finding DYNAMIC section?

Since there is only one valid `case` statement, a break is unnecessary. 
Moreover, as new `case` statements become valid, I don't actually envision them 
having a break at all, more like:
```
switch ((SectionType)shdr.sh_type) {
  case SectionType::TYPE1:
  case SectionType::TYPE2:
  case SectionType::TYPE3:
  case SectionType::DYNAMIC:
file->sections[(SectionType)shdr.sh_type].push_back(scn);
}
```


> On May 17, 2016, 10:23 p.m., Jojy Varghese wrote:
> > 3rdparty/stout/include/stout/elf.hpp, line 184
> > 
> >
> > emplace_back?

What's the advantage or disadvantage of this?  My impression is that 
`emplace_back()` is only really useful if you want to pass a bunch or arguments 
to it in the `...` style rather than passing it a single object or a whole 
nother vector.


> On May 17, 2016, 10:23 p.m., Jojy Varghese wrote:
> > 3rdparty/stout/include/stout/elf.hpp, line 196
> > 
> >
> > const?

Looking at this again, I don't even need to save the `path` as part of the 
`File` object.  I've removed it.


- Kevin


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


On May 17, 2016, 7:17 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47482/
> ---
> 
> (Updated May 17, 2016, 7:17 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
> ---
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 47482: Added preliminary support for parsing ELF files in stout.

2016-05-17 Thread Kevin Klues 

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

(Updated May 18, 2016, 3:16 a.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Updated based on comments.  Thanks!


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 (updated)
-

  3rdparty/stout/configure.ac ada1e22c72875fe9d557f07e4846128be0bcef13 
  3rdparty/stout/include/stout/elf.hpp PRE-CREATION 

Diff: https://reviews.apache.org/r/47482/diff/


Testing
---


Thanks,

Kevin Klues

Re: Review Request 47482: Added preliminary support for parsing ELF files in stout.

2016-05-17 Thread Jojy Varghese 

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




3rdparty/stout/include/stout/elf.hpp (line 77)


Dont we have to close the open file in error cases?



3rdparty/stout/include/stout/elf.hpp (line 94)


Maybe break out after finding DYNAMIC section?



3rdparty/stout/include/stout/elf.hpp (line 184)


emplace_back?



3rdparty/stout/include/stout/elf.hpp (line 196)


const?


- Jojy Varghese


On May 17, 2016, 7:17 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47482/
> ---
> 
> (Updated May 17, 2016, 7:17 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
> ---
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>