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).
> + 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.)
> +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.
> +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.
> +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.
HTH,
Michal
_______________________________________________
Libcg-devel mailing list
Libcg-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/libcg-devel
