This is an automatically generated e-mail. To reply, visit:

As mentioned in https://reviews.apache.org/r/44476/ this patch should probably 
be partially combined with it and then split out into tow pieces:

1) The new API code
2) The tests.

Other comments below.

src/linux/cgroups.hpp (line 34)

    This should go away, given the feedback in the previous patch.

src/linux/cgroups.hpp (lines 629 - 635)

    This function probably shouldn't do the work of checking the list for an 
entry.  Instead, it should return a vector of whitelist entries (i.e. 
`vector<DeviceEntry>`) which the user can parse as he sees fit. The comment 
should cahnge to match.

src/linux/cgroups.cpp (lines 2426 - 2427)

    Given the feedback on the previous patch, this should probably take a 
`DeviceEntry`, not a `Selector` and `Access` parameter separately.
    I realize this is contary to what we suggested previously, but we've had a 
bit more time to think about it now, and we think this makes the msot sense.

src/linux/cgroups.cpp (lines 2429 - 2437)

    As per the comment above, we should probably jsut return a vector here.  
Let the code outside of this decide what to do with the list.

src/linux/cgroups.cpp (lines 2443 - 2444)

    Same comment as above.

src/linux/cgroups.cpp (lines 2450 - 2451)

    We should probably put a blank line between thses two statements for 
consistency with other code.

src/linux/cgroups.cpp (lines 2461 - 2462)

    Same comment as above.

src/linux/cgroups.cpp (lines 2468 - 2469)

    Same comment as above.

src/tests/containerizer/cgroups_tests.cpp (line 1284)

    Can you put a comment here about why we need to deny all by default for 
this to work (I know why this is necessary, but it may not be clear for others 
trying to walk through all the cases)?
    For example, without first denying all, the following fails:
    * write allow all
    * check /dev/console
    This will fail because of the way the devices.list file works. It only 
actually prints out entries **explicitly** written to it previously.
    This means that writing "allow all" followed by "check /dev/null" will 
always fail, even though we obviously have access.
    Unfortunately, this is just the nature of the interface and due 
"whitelisting" instead of "blacklisting".

src/tests/containerizer/cgroups_tests.cpp (lines 1286 - 1287)

    Update throughout with `DeviceEntry`

src/tests/containerizer/cgroups_tests.cpp (line 1287)

    I would probably not use any sort of default constructor for either 
`Selector` or `Access`.  It is much more readable to see the actual parameters 

src/tests/containerizer/cgroups_tests.cpp (lines 1289 - 1293)

    This test will probably have to change given the new semantics of 

- Kevin Klues

On March 14, 2016, 6:25 p.m., Abhishek Dasgupta wrote:
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44439/
> -----------------------------------------------------------
> (Updated March 14, 2016, 6:25 p.m.)
> Review request for mesos, Ben Mahler, Felix Abecassis, Kevin Klues, and 
> Niklas Nielsen.
> Bugs: MESOS-3368
>     https://issues.apache.org/jira/browse/MESOS-3368
> Repository: mesos
> Description
> -------
> There are some helper methods added to support device cgroups in cgroups 
> abstraction to aid isolators controlling access to devices.
> Diffs
> -----
>   src/linux/cgroups.hpp 51ccefd1fd2d0989b9bd31342d3d1e3701f88ed2 
>   src/linux/cgroups.cpp df18ed46a2a96871f67c7eb4233c3b4c27b7aa1c 
>   src/tests/containerizer/cgroups_tests.cpp 
> acaed9b3f8a04964092cef413133834d0cf5a145 
> Diff: https://reviews.apache.org/r/44439/diff/
> Testing
> -------
> make check
> Thanks,
> Abhishek Dasgupta

Reply via email to