mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Benjamin Hindman" <b...@berkeley.edu>
Subject Re: Review Request: Added generic process utilities to libstout.
Date Sat, 15 Jun 2013 22:11:08 GMT

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



3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp
<https://reviews.apache.org/r/11092/#comment45257>

    I would have actually expected os::pids to be an OS specific primitive for this stuff.
See my comment below.



3rdparty/libprocess/3rdparty/stout/include/stout/os/ls.hpp
<https://reviews.apache.org/r/11092/#comment45249>

    s/OS_HPP/OS_LS_HPP/



3rdparty/libprocess/3rdparty/stout/include/stout/os/osx.hpp
<https://reviews.apache.org/r/11092/#comment45255>

    I'd really like to see this stuff simplified with os::sysctl. To help, I threw the review
up at https://reviews.apache.org/r/11895.



3rdparty/libprocess/3rdparty/stout/include/stout/os/osx.hpp
<https://reviews.apache.org/r/11092/#comment45256>

    I feel like the OS X version of os::process(pid) should be implemented via 'proc_pidinfo'
(basically the code above in os::processes()). In fact, just like the Linux implementation
of os::processes(), it seems like the OS X implementation of os::processes() should invoke
os::process(pid) for each pid it gets via sysctl. Better yet, it seems like os::processes()
could be implemented OS agnostically via os::process(pid) and an OS specific os::pids():
    
    ----------------------------
    inline Try<std::list<Process> > processes()
    {
      const Try<std::set<pid_t> >& pids = os::pids();
    
      if (pids.isError()) {
        return Error(pids.error());
      }
    
      std::list<Process> result;
      foreach (pid_t pid, pids.get()) {
        const Try<os::Process>& process = os::process(pid);
    
        // Ignore any processes that disappear.
        if (process.isError()) {
          continue;
        }
    
        result.push_back(process.get());
      }
      return result;
    }
    --------------
    
    The Linux implementation of os::pids() would just invoke proc::pids() and the OS X implementation
would look like this (using the new os::sysctl):
    
    --------------
    Try<std::set<pid_t> > pids()
    {
      Try<int> maxproc = os::sysctl(CTL_KERN, KERN_MAXPROC).integer();
    
      if (maxproc.isError()) {
        return Error(maxproc.error());
      }
    
      Try<vector<kinfo_proc> > processes =
        os::sysctl(CTL_KERN, KERN_PROC, KERN_PROC_ALL).table(maxproc.get());
    
      if (processes.isError()) {
        return Error(processes.error());
      }
    
      std::set<pid_t> result;
    
      foreach (const kinfo_proc& process, processes.get()) {
        result.push_back(process.kp_proc.p_pid);
      }
    
      return result;
    }
    --------------



3rdparty/libprocess/3rdparty/stout/include/stout/os/process.hpp
<https://reviews.apache.org/r/11092/#comment45254>

    Can we add a newline please?



3rdparty/libprocess/3rdparty/stout/include/stout/proc.hpp
<https://reviews.apache.org/r/11092/#comment45248>

    s/tokens/format/ ?



3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp
<https://reviews.apache.org/r/11092/#comment45252>

    /s/linux/Linux/ and s/cpu/CPU/ ;)



3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp
<https://reviews.apache.org/r/11092/#comment45250>

    s/foreach(/foreach (/



3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp
<https://reviews.apache.org/r/11092/#comment45253>

    Any reason not to 'break'?


- Benjamin Hindman


On June 11, 2013, 9:20 p.m., Ben Mahler wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11092/
> -----------------------------------------------------------
> 
> (Updated June 11, 2013, 9:20 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Vinod Kone.
> 
> 
> Description
> -------
> 
> These were reviewed by benh, and I'm sending these out in a new chain as the old chain
no longer made sense.
> 
> Old reviews:
> https://reviews.apache.org/r/10895/
> https://reviews.apache.org/r/10896/
> https://reviews.apache.org/r/10897/
> https://reviews.apache.org/r/10898/
> https://reviews.apache.org/r/10899/
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/3rdparty/stout/Makefile.am 84062a0e1dfe4ec04bac7cac5ebaac4b945eb66e

>   3rdparty/libprocess/3rdparty/stout/include/stout/fs.hpp c1a05b58cbe1d0a9d006dec2db47d147149d421b

>   3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 1b3fb47d7567b5467fef2a2bb15d5c4a2ea42aa5

>   3rdparty/libprocess/3rdparty/stout/include/stout/os/linux.hpp PRE-CREATION 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/ls.hpp PRE-CREATION 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/osx.hpp PRE-CREATION 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/process.hpp PRE-CREATION 
>   3rdparty/libprocess/3rdparty/stout/include/stout/proc.hpp b59735fbe993839aae3cb9ca7871517798c4cd46

>   3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 047778d05ebbbefd85e4a163dbb6ab8445edfb7f

>   3rdparty/libprocess/3rdparty/stout/tests/proc_tests.cpp 2305ef53b700d5092802eab6f54a2893f5622e02

> 
> Diff: https://reviews.apache.org/r/11092/diff/
> 
> 
> Testing
> -------
> 
> Added tests, make check on OSX and Linux.
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>


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