mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Kevin Klues <klue...@gmail.com>
Subject Re: Review Request 46370: Introduced linux capabilities API.
Date Wed, 20 Apr 2016 03:15:29 GMT

-----------------------------------------------------------
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
> 
>


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