-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/75096/#review226703
-----------------------------------------------------------
Ship it!
local edits:
```
diff --git a/src/linux/cgroups.cpp b/src/linux/cgroups.cpp
index e5605a092..3dae320bd 100644
--- a/src/linux/cgroups.cpp
+++ b/src/linux/cgroups.cpp
@@ -2920,30 +2920,22 @@ bool Entry::encompasses(const Entry& other) const
return true;
}
- // Check if major numbers are specified, if so, check if they are unequal
- // Also check if this entry has wildcard major.
if (selector.major.isSome() && selector.major != other.selector.major) {
return false;
}
- // Do the same for minor now that we know major matches.
if (selector.minor.isSome() && selector.minor != other.selector.minor) {
return false;
}
- // Check if the two entries have different device types,
- // if they do, check if the this entry has a wildcard type "ALL".
- if (
- selector.type != Entry::Selector::Type::ALL &&
- selector.type != other.selector.type) {
+ if (selector.type != Entry::Selector::Type::ALL
+ && selector.type != other.selector.type) {
return false;
}
- // Check if all accesses requested by the other entry are specified.
- if (
- (!access.mknod && other.access.mknod) ||
- (!access.read && other.access.read) ||
- (!access.write && other.access.write)) {
+ if ((!access.mknod && other.access.mknod)
+ || (!access.read && other.access.read)
+ || (!access.write && other.access.write)) {
return false;
}
@@ -2956,8 +2948,7 @@ bool Entry::Access::none() const { return !mknod && !read
&& !write; }
bool Entry::Selector::has_wildcard() const
{
- return !major.isSome() || !minor.isSome() ||
- type == Entry::Selector::Type::ALL;
+ return major.isNone() || minor.isNone() || type ==
Entry::Selector::Type::ALL;
}
diff --git a/src/linux/cgroups.hpp b/src/linux/cgroups.hpp
index 3c6ed7a2f..4fefb5c3b 100644
--- a/src/linux/cgroups.hpp
+++ b/src/linux/cgroups.hpp
@@ -933,9 +933,11 @@ namespace devices {
struct Entry
{
static Try<Entry> parse(const std::string& s);
- // Checks if entry accepts all devices with any major/minor numbers and
access
+
+ // Returns true iff entry matches all devices accesses.
bool is_catch_all() const;
- // Checks if this entry fully contains the other's device accesses
+
+ // Returns true iff this entry fully contains the other's device accesses.
bool encompasses(const Entry& other) const;
struct Selector
@@ -950,7 +952,8 @@ struct Entry
Type type;
Option<unsigned int> major; // Matches all `major` numbers if None.
Option<unsigned int> minor; // Matches all `minor` numbers if None.
- // Checks if entry has wildcards in major/minor number and device type
+
+ // Returns iff major or minor are wildcards or if type == ALL.
bool has_wildcard() const;
};
@@ -959,7 +962,6 @@ struct Entry
bool read;
bool write;
bool mknod;
- // Checks if this entry does not specify any access type
bool none() const;
};
diff --git a/src/tests/containerizer/cgroups_tests.cpp
b/src/tests/containerizer/cgroups_tests.cpp
index 9dd1f9180..e4ca7933b 100644
--- a/src/tests/containerizer/cgroups_tests.cpp
+++ b/src/tests/containerizer/cgroups_tests.cpp
@@ -1416,59 +1416,76 @@ TEST(DevicesTest, SubsetHelperTest)
CHECK_NOTERROR(cgroups::devices::Entry::parse("c *:1 rwm"));
EXPECT_TRUE(subset.encompasses(subset));
-
+ EXPECT_TRUE(superset.encompasses(superset));
EXPECT_FALSE(subset.encompasses(superset));
EXPECT_TRUE(superset.encompasses(subset));
superset = CHECK_NOTERROR(cgroups::devices::Entry::parse("c 3:* rwm"));
+ EXPECT_TRUE(subset.encompasses(subset));
+ EXPECT_TRUE(superset.encompasses(superset));
EXPECT_FALSE(subset.encompasses(superset));
EXPECT_TRUE(superset.encompasses(subset));
superset = CHECK_NOTERROR(cgroups::devices::Entry::parse("a"));
+ EXPECT_TRUE(subset.encompasses(subset));
+ EXPECT_TRUE(superset.encompasses(superset));
EXPECT_FALSE(subset.encompasses(superset));
EXPECT_TRUE(superset.encompasses(subset));
subset = CHECK_NOTERROR(cgroups::devices::Entry::parse("c 3:1 rm"));
superset = CHECK_NOTERROR(cgroups::devices::Entry::parse("c 3:1 rwm"));
+ EXPECT_TRUE(subset.encompasses(subset));
+ EXPECT_TRUE(superset.encompasses(superset));
EXPECT_FALSE(subset.encompasses(superset));
EXPECT_TRUE(superset.encompasses(subset));
subset = CHECK_NOTERROR(cgroups::devices::Entry::parse("b 3:1 rm"));
+ EXPECT_TRUE(subset.encompasses(subset));
+ EXPECT_TRUE(superset.encompasses(superset));
EXPECT_FALSE(subset.encompasses(superset));
EXPECT_FALSE(superset.encompasses(subset));
}
-TEST(DevicesTest, AccessNoneTest) {
+TEST(DevicesTest, AccessNoneTest)
+{
cgroups::devices::Entry::Access access;
+
access.read = false;
access.write = false;
access.mknod = false;
EXPECT_TRUE(access.none());
+
access.read = true;
access.write = false;
access.mknod = false;
EXPECT_FALSE(access.none());
+
access.read = false;
access.write = true;
access.mknod = false;
EXPECT_FALSE(access.none());
+
access.read = false;
access.write = false;
access.mknod = true;
EXPECT_FALSE(access.none());
+
access.read = true;
access.write = true;
access.mknod = false;
EXPECT_FALSE(access.none());
+
access.read = true;
access.write = false;
access.mknod = true;
EXPECT_FALSE(access.none());
+
access.read = false;
access.write = true;
access.mknod = true;
EXPECT_FALSE(access.none());
+
access.read = true;
access.write = true;
access.mknod = true;
@@ -1476,20 +1493,24 @@ TEST(DevicesTest, AccessNoneTest) {
}
-TEST(DeviceTest, SelectorWildcardTest) {
+TEST(DeviceTest, SelectorWildcardTest)
+{
cgroups::devices::Entry::Selector selector;
selector.type = cgroups::devices::Entry::Selector::Type::ALL;
selector.major = 1;
selector.minor = 1;
EXPECT_TRUE(selector.has_wildcard());
+
selector.type = cgroups::devices::Entry::Selector::Type::BLOCK;
selector.major = Option<unsigned int>::none();
selector.minor = 1;
EXPECT_TRUE(selector.has_wildcard());
+
selector.type = cgroups::devices::Entry::Selector::Type::BLOCK;
selector.major = 1;
selector.minor = Option<unsigned int>::none();
EXPECT_TRUE(selector.has_wildcard());
+
selector.type = cgroups::devices::Entry::Selector::Type::BLOCK;
selector.major = 1;
selector.minor = 1;
@@ -1498,4 +1519,4 @@ TEST(DeviceTest, SelectorWildcardTest) {
} // namespace tests {
} // namespace internal {
-} // namespace mesos {
\ No newline at end of file
+} // namespace mesos {
```
- Benjamin Mahler
On July 18, 2024, 8:19 p.m., Jason Zhou wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/75096/
> -----------------------------------------------------------
>
> (Updated July 18, 2024, 8:19 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Currently, the Entry class does not have readable helper functions for
> determining whether the device accesses represented by one Entry would
> be a subset of that of another. In addition, we want more readable ways
> to determine if a device has wildcards present and if it has any
> accesses specified.
>
> These additions will streamline the logic in the DeviceManager
> DeviceManager, which will heavily utilize the Entry class, improving
> code readability.
>
>
> Diffs
> -----
>
> src/linux/cgroups.hpp eaa4cdf9fd0fe6dd848a6626c4c42ccab070e232
> src/linux/cgroups.cpp c4289493315eb4765192f90099c5b632c6d7f9d1
> src/tests/containerizer/cgroups_tests.cpp
> 0f4967ff1754fcf54e9363e2bc62c7bc44ac4a56
>
>
> Diff: https://reviews.apache.org/r/75096/diff/2/
>
>
> Testing
> -------
>
> Tests added for `is_improper_subset_of`, `Selector::has_wildcard`,
> `Access::none`. Tests passed.
>
>
> Thanks,
>
> Jason Zhou
>
>
