----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44796/#review123505 -----------------------------------------------------------
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) <https://reviews.apache.org/r/44796/#comment185734> 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 instead. 3rdparty/libprocess/3rdparty/stout/include/Makefile.am (line 24) <https://reviews.apache.org/r/44796/#comment185739> Unnecessary, with move to `src/linux/cgroups.hpp` 3rdparty/libprocess/3rdparty/stout/include/stout/device.hpp (line 25) <https://reviews.apache.org/r/44796/#comment185740> 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) <https://reviews.apache.org/r/44796/#comment185742> 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) <https://reviews.apache.org/r/44796/#comment185749> 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) <https://reviews.apache.org/r/44796/#comment185751> 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) <https://reviews.apache.org/r/44796/#comment185743> 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) <https://reviews.apache.org/r/44796/#comment185745> 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 provided"); } 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) <https://reviews.apache.org/r/44796/#comment185744> 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) <https://reviews.apache.org/r/44796/#comment185760> 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) <https://reviews.apache.org/r/44796/#comment185761> No need for these accessors if we make this a struct. 3rdparty/libprocess/3rdparty/stout/include/stout/device.hpp (lines 124 - 136) <https://reviews.apache.org/r/44796/#comment185762> 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) <https://reviews.apache.org/r/44796/#comment185764> See above comment for DeviceEntry parser. 3rdparty/libprocess/3rdparty/stout/include/stout/device.hpp (lines 183 - 187) <https://reviews.apache.org/r/44796/#comment185763> Again, no need for accessors here if this becomes a struct. 3rdparty/libprocess/3rdparty/stout/tests/CMakeLists.txt (line 25) <https://reviews.apache.org/r/44796/#comment185736> Unnecessary, with a move to `src/tests/containerizer/cgroups_tests.cpp` 3rdparty/libprocess/3rdparty/stout/tests/device_tests.cpp (lines 22 - 47) <https://reviews.apache.org/r/44796/#comment185738> 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"); EXPECT_ERROR(selector3); Try<Selector> selector4 = Selector::parse("c 1:"); EXPECT_ERROR(selector4); Try<Access> access1 = Access::parse("m r w"); EXPECT_EQ("rwm", stringify(access1.get())); Try<Access> access2 = Access::parse("t r w"); EXPECT_ERROR(access2); } ``` 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 > >
