> On July 22, 2016, 2:32 a.m., Jie Yu wrote: > > src/linux/capabilities.cpp, lines 46-48 > > <https://reviews.apache.org/r/50266/diff/1/?file=1448444#file1448444line46> > > > > No need for 'static' here if constexpr is used. > > > > Also, I would prefer calling it: > > ``` > > PROC_CAP_LAST_CAP > > ```
I made all variables in this block `constexpr`. I am not sure how storage duration would be affected by using `constexpr` since this does not make these variable compile-time constants (i.e., the compiler might still allocate storage, but it could just as well optimize the variable away), but I also dropped the `static` specifiers. Also, I use the name you suggested now. > On July 22, 2016, 2:32 a.m., Jie Yu wrote: > > src/linux/capabilities.cpp, line 47 > > <https://reviews.apache.org/r/50266/diff/1/?file=1448444#file1448444line47> > > > > any reason this is not a constexpr? `constexpr` now. > On July 22, 2016, 2:32 a.m., Jie Yu wrote: > > src/linux/capabilities.cpp, line 48 > > <https://reviews.apache.org/r/50266/diff/1/?file=1448444#file1448444line48> > > > > Ditto here? Done. > On July 22, 2016, 2:32 a.m., Jie Yu wrote: > > src/linux/capabilities.cpp, line 257 > > <https://reviews.apache.org/r/50266/diff/1/?file=1448444#file1448444line257> > > > > s/processCaps/capabilities/ Renamed here and also for the case of the parameters of `doCapSet` and `set` below. > On July 22, 2016, 2:32 a.m., Jie Yu wrote: > > src/linux/capabilities.cpp, lines 34-39 > > <https://reviews.apache.org/r/50266/diff/1/?file=1448444#file1448444line34> > > > > Any reason why we need to declare them here? Shouldn't > > `<linux/capability>` already declared them? These are libc function mapping to sys calls which are only exposed via libcap development headers. Declaring them here ourself allows us to not depend on the presence of the development header files. I agree this is not something we do often. Should we just introduce the dependency on the dev files and do away with our own decls of these functions here? > On July 22, 2016, 2:32 a.m., Jie Yu wrote: > > src/linux/capabilities.hpp, lines 203-212 > > <https://reviews.apache.org/r/50266/diff/1/?file=1448443#file1448443line203> > > > > 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. I moved the code and added it in https://reviews.apache.org/r/50889/. > On July 22, 2016, 2:32 a.m., Jie Yu wrote: > > src/linux/capabilities.hpp, lines 215-228 > > <https://reviews.apache.org/r/50266/diff/1/?file=1448443#file1448443line215> > > > > Let's introduce this in a separate patch. Moved to https://reviews.apache.org/r/50889/. - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50266/#review143038 ----------------------------------------------------------- On Aug. 8, 2016, 1:03 p.m., Benjamin Bannier wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/50266/ > ----------------------------------------------------------- > > (Updated Aug. 8, 2016, 1:03 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 1286ee08fe2d60867326a1f2585f054c20b52208 > src/Makefile.am 1a9b083493612cf610b80ac5a1c11c29d6302933 > 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 > >
