On 2/17/20 8:22 AM, Michal Koutný wrote:
Hello On Thu, Feb 13, 2020 at 09:33:45AM -0700, Tom Hromatka <tom.hroma...@oracle.com> wrote:[...] +static const char * const NAMES[][MAX_NAMES] = { + {"tasks", "cpu.shares", "cpu.weight", "cpu.foo"}, + {"tasks"}, + {"tasks", "memory.limit_in_bytes", "memory.memsw.limit_in_bytes"}, + {"tasks", "cpuset.exclusive", "cpuset.foo", "cpuset.bar", "cpuset.x"}, + {"tasks", "namespaces.blah"} +}; [...] +class CgroupGetCgroupTest : public ::testing::Test { + protected: + + void CreateNames(const char * const names[], + const char * const values[], + const char * const ctrl_name) { + char tmp_path[FILENAME_MAX]; + FILE *f; + int i; + + for (i = 0; i < MAX_NAMES; i++) { + if (names[i] == NULL) + break;Where is the NULL value supposed to come from? (Maybe I'm missing something but you rely on implicit zeroing by the array initializers and I can't see how that works for arrays with exactly MAX_NAMES entries (case for cpuset controller).
Good question. You are correct - I am relying on implicit zeroing for the shorter arrays. (And the more I think about it, the more I don't like that. I'll explicitly populate the shorter arrays in a new rev.) For the arrays with exactly MAX_NAMES, I'm relying on the maximum limit in the for loop itself to exit the loop.
+ void SetUp() override { [...] + /* + * arbitrarily don't make the cgroup directory in + * the freezer controller + */ + if (i == 1) + continue;Here the exception is called after "freezer" controller. (Also, maybe controller enum may be more readable.)
Good idea on an enum. I wanted to mimic the real world where some cgroups aren't present in all controllers and didn't really settle on a graceful way to do it. I think the enum would help a lot.
+static void validate_value(const struct control_value * const cv, + int names_idx) +{ + bool found_match = false; + int i; + + for (i = 0; i < MAX_NAMES; i++) { + if (NAMES[names_idx][i] == NULL) + break;Similarly, here it's not clear when is the NULL value set.
Same (slightly) convoluted logic as above. I'll try to refactor it into something easier to digest. Thanks.
+static void validate_cgc(const struct cgroup_controller * const cgc, + int names_idx) +{ + int i, names_cnt = 0; + + for (i = 0; i < MAX_NAMES; i++) { + if (NAMES[names_idx][i] != NULL) + names_cnt++; + } + + /* The tasks file isn't counted */ + ASSERT_EQ(names_cnt - 1, cgc->index); + + for (i = 0; i < cgc->index; i++) { + validate_value(cgc->values[i], names_idx); + }I noticed that if a same key-value pair was present multiple times (and some keys missing), this check would still pass.
Good find. I had the exact same thought, but I was torn on whether I should fix it or not. Adding the logic to ensure all the settings completely matched got kind of ugly - and required a lot more lines of code. I'll revisit it and see if I can come up with something more graceful, but I'm struggling to find the right balance between verifying correctness and the unwieldiness of the tests. Thoughts?
+static void validate_cg(const struct cgroup * const cg) [...] + for (i = 0, names_idx = 0; i < cg->index; i++, names_idx++) { + /* + * We did not create our cgroup under the freezer controller. + * Advance the index to the next controller. memory comes + * immediately after it. + */ + if (strcmp(cg->controller[i]->name, "memory") == 0) + names_idx++;Here the exception case is called by "memory" controller (cf "freezer" before). That may be impractical for maintenance.
Yes, rather hideous. I think your suggestion of the enum should help a lot.
HTH,
Your comments over the last few months have been very helpful. My sincere thanks! Tom
Michal
_______________________________________________ Libcg-devel mailing list Libcg-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/libcg-devel
