Re: Review Request 68642: Added `lsof()` into stout.

[Qian Zhang <zh...@gmail.com>]

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