> On Sept. 7, 2018, 1:37 a.m., James Peach wrote: > > We should add a basic test for this, even if it just ensures that the > > result contains the elements 0, 1 and 2.
Added a test here: https://reviews.apache.org/r/68991/ > On Sept. 7, 2018, 1:37 a.m., James Peach wrote: > > 3rdparty/stout/include/stout/os/lsof.hpp > > Lines 16 (patched) > > <https://reviews.apache.org/r/68642/diff/1/?file=2082824#file2082824line16> > > > > Is this really a double newline? I can't keep track, but this seems > > inconsistent with the other files in this patch. > > > > I looked in the current headers and found 3 variants of how many blank > > lines files like this should have :) It seems we do not have a well-known rule for this, and I checked a few header files under `3rdparty/stout/include/stout/os`, it looks like most of them have double newline, so let's follow it :) > On Sept. 7, 2018, 1:37 a.m., James Peach wrote: > > 3rdparty/stout/include/stout/os/posix/lsof.hpp > > Lines 29 (patched) > > <https://reviews.apache.org/r/68642/diff/1/?file=2082825#file2082825line29> > > > > Why hashset rather than vector? By definition there aren't any > > duplicates, so we don't need the overhead of hashset here? > > > > /cc @bmahler If we make this method return a vector, then in the subsequent patches when we remove whitelist fds from the vector, we need to use `std::find` to get the position of the to-be-deleted fd and then call `std::vector::erase` to remove it which is inefficient, especially the implementation of `std::vector::erase` may need to relocate all the elements (http://www.cplusplus.com/reference/vector/vector/erase/) . So I think we can use `std::list` like what we did for `os::ls`. > On Sept. 7, 2018, 1:37 a.m., James Peach wrote: > > 3rdparty/stout/include/stout/os/windows/lsof.hpp > > Lines 23 (patched) > > <https://reviews.apache.org/r/68642/diff/1/?file=2082826#file2082826line23> > > > > In other case, we simply omitted the Windows version altogether, e.g. > > https://reviews.apache.org/r/68398/ > > > > /cc @andschwa But I also see a couple of cases that delete the Windows version, like `chroot`, `inode`, `getuid`, `getgid`, etc. - Qian ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68642/#review208411 ----------------------------------------------------------- On Sept. 6, 2018, 9:25 a.m., Qian Zhang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68642/ > ----------------------------------------------------------- > > (Updated Sept. 6, 2018, 9:25 a.m.) > > > Review request for mesos, Gilbert Song and James Peach. > > > Bugs: MESOS-9152 > https://issues.apache.org/jira/browse/MESOS-9152 > > > Repository: mesos > > > Description > ------- > > Added `lsof()` into stout. > > > Diffs > ----- > > 3rdparty/stout/include/Makefile.am a9c61fcba253a811494cdcdb0afb3d3a018f4585 > 3rdparty/stout/include/stout/os.hpp > aee041891b7e7ff93a0b1ac31019a7a3d4eae962 > 3rdparty/stout/include/stout/os/lsof.hpp PRE-CREATION > 3rdparty/stout/include/stout/os/posix/lsof.hpp PRE-CREATION > 3rdparty/stout/include/stout/os/windows/lsof.hpp PRE-CREATION > > > Diff: https://reviews.apache.org/r/68642/diff/1/ > > > Testing > ------- > > > Thanks, > > Qian Zhang > >