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

This commit and https://reviews.apache.org/r/44439/ should probably be combined 
and then broken up into the follow two commits (one for the implementation and 
one for the test):

Added test for cgroups device entry parsing.
Added helper classes and functions for managing cgroups devices.

The motivation being that these helpers should probably not live inside stout.  
Stout is designed as a **platform independent** API, whereas the stuff you 
added here is very specific to cgroups on Linux.  I would suggest putting it 
into `src/linux/cgroups.{hpp,cpp}` with the rest of the cgroups stuff (as you 
alrdady have some stuff in 44439).  As for the test, I would put it at the 
bottom of `src/tests/containerizer/cgroups_tests.cp` and call it 
`TEST(CgroupsDeviceTest, ParseTest)`.

The comments on the files below are still valid, they just be moved to these 
new locations.

3rdparty/libprocess/3rdparty/stout/Makefile.am (line 29)

    Reviewboard is reporting a whitespace error here. However, I downloaded 
things locally and everything is fine.
    That said, this test should probably not exist in stout (nor should the 
device helpers).  See the other comments for a suggestion on where to put it 

3rdparty/libprocess/3rdparty/stout/include/Makefile.am (line 24)

    Unnecessary, with move to `src/linux/cgroups.hpp`

3rdparty/libprocess/3rdparty/stout/include/stout/device.hpp (line 25)

    Once moved to `src/linux/cgroups.hpp`, this should all exist in the 
`cgroups::devices` namespace.
    Also, all of the classes in this file should be declared in the `.hpp` file 
and defined in the `.cpp`.
    This is a similar pattern followed by `cgroups::memory::pressure::Counter`

3rdparty/libprocess/3rdparty/stout/include/stout/device.hpp (lines 26 - 31)

    I would probably embed this enum inside the `Device` class (as mentioned in 
a comment below).

3rdparty/libprocess/3rdparty/stout/include/stout/device.hpp (line 33)

    I would suggest adding a class called `DeviceEntry`, which containes 
subfields for `Selector` and `Access`.
    With this, a parse function in the `cgroups::devices::parse()` namespace 
can be written to parse full whitelist entries, i.e.:
    static Try<DeviceEntry> parse(const std::string& entry);
    I would also then make the `Selector` and `Access` classes structs instead 
of classes.

3rdparty/libprocess/3rdparty/stout/include/stout/device.hpp (line 35)

    I would probably not embed parse functions in the `Selector` and `Access` 
classes as you have done here.  I sugest having a global `parse()` function 
that operates on `DeviceEntry` objects as described in a previous comment.

3rdparty/libprocess/3rdparty/stout/include/stout/device.hpp (lines 37 - 40)

    We typically don't "pre-declare" variables like this in Mesos.  Instead, we 
declare them just before their first use.
    Also, using a size_t for index isn't really the correct type here. It 
should probably be `std::string::size_type`.

3rdparty/libprocess/3rdparty/stout/include/stout/device.hpp (lines 42 - 100)

    Assuming we move this parse function out to a global `Try<DeviceEntry> 
parse()` function, I feel we can get away sith something much simpler here. 
Something like:
    Try<DeviceEntry> parse(const std::string& entry)
      std::vector<std::string> tokens = strings::tokenize(entry, " ");
      if (tokens.size() == 0) {
        return Error("Unable to parse cgroups device entry: no arguments 
      if (token[0] == "a") {
        Selector selector(Selector::Type::ALL, "*", "*");
        Access access(true, true, true);
        return DeviceEntry(selector, access);
      Selector selector;
      if (token[0] == "b") {
        selector.deviceType = Selector::Type::BLOCK;
      else if (token[0] == "c") {
        selector.deviceType = Selector::Type::CHARACTER;
      } else {
        return Error("Unable to parse cgroups device entry: unknown device 
type: ", + token[0]);
      if (tokens.size() != 3) {
        return Error("Unable to parse cgroups device entry: not enough 
arguments for device type: ", + token[0]);
      Selector selector;
      selector.deviceType = token[0];
      std::vector<std::string> deviceNumbers = strings::tokenize(token[1], ":");
      if (deviceNumbers.size() != 2) {
        return Error("Unable to parse cgroups device entry: unknown device 
number: ", + token[1]);
      Try<unsigned int> major = numify<unsigned int>(deviceNumbers[0]);
      if (major.isError()) {
        return Error("Unable to parse cgroups device entry: unknown major 
device: ", + deviceNumbers[0]);
      selector.major = major.get();
      Try<unsigned int> minor = numify<unsigned int>(deviceNumbers[1]);
      if (minor.isError()) {
        return Error("Unable to parse cgroups device entry: unknown minor 
device: ", + deviceNumbers[1]);
      selector.minor = minor.get();
      std::vector<std::string> access = strings::tokenize(token[2], "");
      if (deviceNumbers.size() > 3) {
        return Error("Unable to parse cgroups device entry: too many access 
arguments: ", + token[2]);
      Access access;
      foreach (const string& permission, access) {
        if (permission == "r") {
          access.read = true;
        } else if (permission == "w") {
          access.write = true;
        } else if (permission == "m") {
          access.mkdnode = true;    
        } else {
          return Error("Unable to parse cgroups device entry: invalid 
permission: ", + permission);
      return DeviceEntry(selector, access);

3rdparty/libprocess/3rdparty/stout/include/stout/device.hpp (line 50)

    I know why this works, but it is probably not clear to someone just reading 
the code as to why we just bail out here when we encounter an 'a' (even the 
cgroups docs aren't very clear on this, and you have to play around with things 
to actually discover this).
    I would add a comment here mentioning what's going on here.
    Also, I wouldn't bother setting deviceType here -- jut pass ALL directly to 
the `return Selector()` call.

3rdparty/libprocess/3rdparty/stout/include/stout/device.hpp (line 105)

    We should probably have a variabnt of this that takes all three parameters 
as inputs.

3rdparty/libprocess/3rdparty/stout/include/stout/device.hpp (lines 112 - 116)

    No need for these accessors if we make this a struct.

3rdparty/libprocess/3rdparty/stout/include/stout/device.hpp (lines 124 - 136)

    We will still need this.
    We will also need one for the DeviceEntry that just strems the `Selector` 
and `Access` structs out properly.

3rdparty/libprocess/3rdparty/stout/include/stout/device.hpp (lines 141 - 172)

    See above comment for DeviceEntry parser.

3rdparty/libprocess/3rdparty/stout/include/stout/device.hpp (lines 183 - 187)

    Again, no need for accessors here if this becomes a struct.

3rdparty/libprocess/3rdparty/stout/tests/CMakeLists.txt (line 25)

    Unnecessary, with a move to `src/tests/containerizer/cgroups_tests.cpp`

3rdparty/libprocess/3rdparty/stout/tests/device_tests.cpp (lines 22 - 47)

    I would probably remove the extra spaces here, like so:
    TEST(DeviceTest, parseTest)
      Try<Selector> selector1 = Selector::parse("c *:3");
      EXPECT_EQ("c *:3", stringify(selector1.get()));
      Try<Selector> selector2 = Selector::parse("a");
      EXPECT_EQ("a *:*", stringify(selector2.get()));
      Try<Selector> selector3 = Selector::parse("za23");
      Try<Selector> selector4 = Selector::parse("c 1:");
      Try<Access> access1 = Access::parse("m r w");
      EXPECT_EQ("rwm", stringify(access1.get()));
      Try<Access> access2 = Access::parse("t r w");
    Normally we put extra spaces to separate `EXPECT` calls from a sequence of 
operations that precede it.  However, in this case, each `EXPECT` call is 
directly tied to the one operation right above it, so it is cleaner to group 
them together.
    This test will obvioulsy need to be updated given the feedback above.

- Kevin Klues

On March 14, 2016, 6:22 p.m., Abhishek Dasgupta wrote:
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44796/
> -----------------------------------------------------------
> (Updated March 14, 2016, 6:22 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
> -------
> Helper classes are added for device entry which is passed on to device 
> cgroups to provide devices as resources.
> Diffs
> -----
>   3rdparty/libprocess/3rdparty/stout/Makefile.am 
> 400c6dc451602926f93b22713af8c66d7ca59ca6 
>   3rdparty/libprocess/3rdparty/stout/include/Makefile.am 
> c10c6d9d7c68a2d5b27d68736a49d212e70dcd05 
>   3rdparty/libprocess/3rdparty/stout/include/stout/device.hpp PRE-CREATION 
>   3rdparty/libprocess/3rdparty/stout/tests/CMakeLists.txt 
> 03e09fb33bf038930a2be0370883511e93f94753 
>   3rdparty/libprocess/3rdparty/stout/tests/device_tests.cpp PRE-CREATION 
> Diff: https://reviews.apache.org/r/44796/diff/
> Testing
> -------
> sudo make -j2 check GTEST_FILTER="DeviceTest.parseTest"
> Thanks,
> Abhishek Dasgupta

Reply via email to