> On May 17, 2016, 10:23 p.m., Jojy Varghese wrote: > > 3rdparty/stout/include/stout/elf.hpp, line 77 > > <https://reviews.apache.org/r/47482/diff/1/?file=1385836#file1385836line77> > > > > 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<File>` rather than a `Try<File*>`, 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<unique_ptr<File>>` 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 > > <https://reviews.apache.org/r/47482/diff/1/?file=1385836#file1385836line94> > > > > 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 > > <https://reviews.apache.org/r/47482/diff/1/?file=1385836#file1385836line184> > > > > 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 > > <https://reviews.apache.org/r/47482/diff/1/?file=1385836#file1385836line196> > > > > 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 > >
