----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50266/#review143038 -----------------------------------------------------------
Let me know if you want to chat about any of the feedback! Thanks! src/linux/capabilities.hpp (line 37) <https://reviews.apache.org/r/50266/#comment208726> I'd prefer using `int` here since protobuf enum converts to int. src/linux/capabilities.hpp (line 81) <https://reviews.apache.org/r/50266/#comment208729> Let's move all global functions down (close to each other) below `getAllSupportedCapabilities`. src/linux/capabilities.hpp (line 84) <https://reviews.apache.org/r/50266/#comment208727> Ditto on using int here. src/linux/capabilities.hpp (lines 108 - 110) <https://reviews.apache.org/r/50266/#comment208802> No need to for this friend unqulified function. This can just be a member function. src/linux/capabilities.hpp (line 113) <https://reviews.apache.org/r/50266/#comment208879> I'd prefer we don't store a bit flag here. That makes the code uncessarily complex. Let's hide all the bit tricky in `Capabilities` class below (as it interacts with syscall interface directly). Let's keep other part of the code simple. Also, i'd prefer explicitly use 4 variables here intead of using an array. ``` Set<Capability> effective; Set<Capability> permitted; Set<Capability> inheritable; Set<Capability> bounded; ``` That means the `get` and `set` probably need to use a `switch` statement. But we can kill `MAX_CAPABILITY_TYPE` which is confusing. src/linux/capabilities.hpp (lines 117 - 119) <https://reviews.apache.org/r/50266/#comment208882> Let's use `/***/` style comments so that doxygen can pick them up. Ditto on all interfaces that you want to expose in this header. src/linux/capabilities.hpp (lines 124 - 134) <https://reviews.apache.org/r/50266/#comment208894> I'd suggest that we don't create a class `Capabilities`. Instead, let's put all methods under namesapce `capabilities`. Looks like we just need to cache `lastCapability` (`/proc/sys/kernel/cap_last_cap`), which should not change. There's no reason to keep a class for that. ``` namespace capabilities { Try<Nothing> initialize(); Try<ProcessCapabilities> get(); Try<Nothing> set(const ProcessCapabilities& set); ... } ``` src/linux/capabilities.hpp (lines 136 - 137) <https://reviews.apache.org/r/50266/#comment208888> I don't think this is needed. `getAllSupportedCapabilities` is sufficient. src/linux/capabilities.hpp (lines 147 - 148) <https://reviews.apache.org/r/50266/#comment208883> Why the extra space before `@param` and `@return`? src/linux/capabilities.hpp (line 171) <https://reviews.apache.org/r/50266/#comment208900> This function is pretty high level. I don't think it belongs here. Looking at other capability libraries like https://github.com/syndtr/gocapability/tree/master/capability, it's not there. Let's move this out of this patch first. src/linux/capabilities.hpp (lines 179 - 180) <https://reviews.apache.org/r/50266/#comment208891> Do we still need this given that we only accept `_LINUX_CAPABILITY_VERSION_3`? src/linux/capabilities.hpp (lines 187 - 190) <https://reviews.apache.org/r/50266/#comment208901> I would reuse `convert` for all of them: ``` Capability convert(const CapabilityInfo::Cpability& capability); CpabilityInfo convert(const Set<Capability>& capabilities); ``` src/linux/capabilities.hpp (line 196) <https://reviews.apache.org/r/50266/#comment208885> I'd prefer this method returns `Set<Capability>`. We should have a general way to convert `CapabilityInfo` to `Set<Capability>`, and vise versa, instead of having different return type (or parameter type) mixed in this file. src/linux/capabilities.hpp (lines 203 - 212) <https://reviews.apache.org/r/50266/#comment208902> This does not belong here. We should put it in include/mesos/type_utils.hpp and src/common/type_utils.cpp. Please add those in a separate patch. src/linux/capabilities.hpp (lines 215 - 228) <https://reviews.apache.org/r/50266/#comment208903> Let's introduce this in a separate patch. src/linux/capabilities.cpp (lines 34 - 39) <https://reviews.apache.org/r/50266/#comment208904> Any reason why we need to declare them here? Shouldn't `<linux/capability>` already declared them? src/linux/capabilities.cpp (lines 46 - 48) <https://reviews.apache.org/r/50266/#comment208805> No need for 'static' here if constexpr is used. Also, I would prefer calling it: ``` PROC_CAP_LAST_CAP ``` src/linux/capabilities.cpp (line 47) <https://reviews.apache.org/r/50266/#comment208803> any reason this is not a constexpr? src/linux/capabilities.cpp (line 48) <https://reviews.apache.org/r/50266/#comment208804> Ditto here? src/linux/capabilities.cpp (line 59) <https://reviews.apache.org/r/50266/#comment208806> Let's move streaming functions to the end of this file to improve readability. Readers more care about the impl. of other functions. src/linux/capabilities.cpp (lines 99 - 100) <https://reviews.apache.org/r/50266/#comment208807> put them in one line: ``` default: UNREACHABLE(); ``` src/linux/capabilities.cpp (lines 112 - 113) <https://reviews.apache.org/r/50266/#comment208808> Ditto here. src/linux/capabilities.cpp (line 191) <https://reviews.apache.org/r/50266/#comment208917> As I mentioned above, this should be the `initialize` function. src/linux/capabilities.cpp (line 257) <https://reviews.apache.org/r/50266/#comment208925> s/processCaps/capabilities/ src/linux/capabilities.cpp (line 274) <https://reviews.apache.org/r/50266/#comment208935> Let's not introduce this helper yet (same for setBound). Just merge them into `set` function below. - Jie Yu On July 20, 2016, 10:18 p.m., Benjamin Bannier wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/50266/ > ----------------------------------------------------------- > > (Updated July 20, 2016, 10:18 p.m.) > > > Review request for mesos and Jie Yu. > > > 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. > > This patch is based on the work in https://reviews.apache.org/r/46370/. > > > Diffs > ----- > > src/CMakeLists.txt bde76f7840afe55f20d7551b3f7e5fe522f7f326 > src/Makefile.am cc83525a4455bbb0e654d346921d66ed2436411d > src/linux/capabilities.hpp PRE-CREATION > src/linux/capabilities.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/50266/diff/ > > > Testing > ------- > > `make check` and `sudo make check` (Debian jessie, gcc-4.9.2, w/o > optimizations) > > > Thanks, > > Benjamin Bannier > >
