mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jie Yu <yujie....@gmail.com>
Subject Re: Review Request 50266: Introduced linux capabilities API.
Date Fri, 22 Jul 2016 00:32:34 GMT

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


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