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



I added a few comments below, but in general, I feel like there are places this 
code could be greatly simplified.  Specifically, it's not obvious to me why we 
need all of the different classes you define (or maybe more about why they need 
to be part of the public interface).  Could you clarify the rational here a bit?


src/linux/capabilities.hpp (line 30)
<https://reviews.apache.org/r/46370/#comment193174>

    Is this file #including itself? Does this compile?



src/linux/capabilities.hpp (lines 32 - 33)
<https://reviews.apache.org/r/46370/#comment193176>

    This should all probably live in the mesos::internal::capabilities 
namespace.



src/linux/capabilities.hpp (lines 39 - 45)
<https://reviews.apache.org/r/46370/#comment193178>

    These are both probably unnecessary (see comments below).



src/linux/capabilities.hpp (lines 50 - 90)
<https://reviews.apache.org/r/46370/#comment193180>

    Since we should probably be embedding this in a `capabilities` namespace, 
it is redundant to call this enum `Capability`. I'd sugggest `Privilege`.  That 
way one of these can be accessed as e.g. `capabiliites::Privilege::SETGID`.
    
    Also, as mentioned in a comment below, this should probably be declared as 
an `enum class` for better type checking.
    
    The `COUNT` trick mentioned below should probably be applied here as well.



src/linux/capabilities.hpp (lines 94 - 99)
<https://reviews.apache.org/r/46370/#comment193179>

    From my reading of: http://man7.org/linux/man-pages/man7/capabilities.7.html
    
    this enum should probably be called `Set`.
    
    Note, the name `Capability` at the front is unnecessary if we embed this in 
the `capabilities` namespace.
    
    Also, it's pretty standard practice in C++ to define an `enum` as a `enum 
class` for better type checking.  As such, you can define the final element 
with a common name of `COUNT` to get at the size of the enum.
    
    For example, you can get at the size of the enum as: 
`capabilities::Set::COUNT` instead of relying on the `const` for 
`NUMBER_OF_CAP_SETS` defined above.



src/linux/capabilities.hpp (line 178)
<https://reviews.apache.org/r/46370/#comment193181>

    Didn't we discuss not making this a class, and only having get()/set() 
calls as part of the namespace?



src/linux/capabilities.hpp (line 209)
<https://reviews.apache.org/r/46370/#comment193182>

    What did we decide about the `add()` pairing to this `drop()` call?



src/linux/capabilities.cpp (lines 36 - 38)
<https://reviews.apache.org/r/46370/#comment193171>

    Is there not a header file you can just include here?



src/linux/capabilities.cpp (lines 124 - 125)
<https://reviews.apache.org/r/46370/#comment193183>

    This should be unnecessary. See: 
https://github.com/klueska-mesosphere/mesos/blob/master/src/linux/cgroups.cpp#L2435


- Kevin Klues


On April 19, 2016, 5:02 p.m., Jojy Varghese wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46370/
> -----------------------------------------------------------
> 
> (Updated April 19, 2016, 5:02 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 ca59a1824352423f4db6ef8bb41acc6fe602c041 
>   src/Makefile.am a8f68316c5d4d4c82d99363535a97fa5b1caafc5 
>   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
> 
>

Reply via email to