----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46370/#review133860 -----------------------------------------------------------
src/linux/capabilities.hpp (line 37) <https://reviews.apache.org/r/46370/#comment198509> As I mentioned earlier, can you specify the enum base as well? http://en.cppreference.com/w/cpp/language/enum src/linux/capabilities.hpp (line 84) <https://reviews.apache.org/r/46370/#comment199328> Let's move this too the end of this file. src/linux/capabilities.hpp (line 107) <https://reviews.apache.org/r/46370/#comment199339> s/index/type/ src/linux/capabilities.cpp (line 48) <https://reviews.apache.org/r/46370/#comment199336> constexpr char I would kill empty lines between those constants. src/linux/capabilities.cpp (lines 58 - 60) <https://reviews.apache.org/r/46370/#comment199340> Not needed. src/linux/capabilities.cpp (line 69) <https://reviews.apache.org/r/46370/#comment199432> I would suggest that we do not use a union here. Instead, use `struct __user_cap_data_struct data` directly. src/linux/capabilities.cpp (line 70) <https://reviews.apache.org/r/46370/#comment199338> `s/__u32/uint32_t/ src/linux/capabilities.cpp (lines 75 - 90) <https://reviews.apache.org/r/46370/#comment199431> Let's inline those helpers and make SyscallPayload a pure struct. src/linux/capabilities.cpp (lines 93 - 111) <https://reviews.apache.org/r/46370/#comment199337> Is it being used? Why do we need that? src/linux/capabilities.cpp (line 116) <https://reviews.apache.org/r/46370/#comment199329> I would follow the following style: ``` switch (cap) { case CHOWN: return stream << "CHOWN"; case DAC_OVERRIDE: return stream << "DAC_OVERRIDE"; ... default: UNREACHABLE(); } ``` src/linux/capabilities.cpp (lines 354 - 370) <https://reviews.apache.org/r/46370/#comment199341> Same here. src/linux/capabilities.cpp (lines 545 - 547) <https://reviews.apache.org/r/46370/#comment199435> Making 'sets[x]' here Option is confusing. Can you make it non optional? src/linux/capabilities.cpp (lines 555 - 570) <https://reviews.apache.org/r/46370/#comment199437> The check here will be performed by the kernel. I would say let's just rely on kernel to do the check, and get rid of the redundent check here. src/linux/capabilities.cpp (lines 606 - 639) <https://reviews.apache.org/r/46370/#comment199438> Similarily, let's make this method very simple and it's callers responsibility to make sure it has proper capability to do prctl here. - Jie Yu On May 17, 2016, 2:59 p.m., Jojy Varghese wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/46370/ > ----------------------------------------------------------- > > (Updated May 17, 2016, 2:59 p.m.) > > > Review request for mesos, Jie Yu and Kevin Klues. > > > Bugs: MESOS-5051 > https://issues.apache.org/jira/browse/MESOS-5051 > > > Repository: mesos > > > Description > ------- > > This change introduces basic API for linux capabilities. This is not a > comprehensive API but is strictly limited to the need for securing Mesos > containers using linux capabilities. > > > Diffs > ----- > > src/CMakeLists.txt e0c538d9e6542fbe279bfbf6f20172e4c611c859 > src/Makefile.am ce5245883f3d2661812272702c0d2060513b6d88 > src/linux/capabilities.hpp PRE-CREATION > src/linux/capabilities.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/46370/diff/ > > > Testing > ------- > > make check. > > > Thanks, > > Jojy Varghese > >
