On 29/07/2024 18:47, Alexis Lothoré wrote:
Hello Alan, thanks for the review
On 7/29/24 19:29, Alan Maguire wrote:
On 29/07/2024 09:20, Alexis Lothoré (eBPF Foundation) wrote:
test_dev_cgroup is defined as a standalone test program, and so is not executed in CI.
Convert it to test_progs framework so it is tested automatically in CI, and remove the old test. In order to be able to run it in test_progs, /dev/null must remain usable, so change the new test to test operations on devices 1:3 as valid, and operations on devices 1:5 (/dev/zero) as invalid.
Signed-off-by: Alexis Lothoré (eBPF Foundation) alexis.lothore@bootlin.com
A few small suggestions but looks great!
Reviewed-by: Alan Maguire alan.maguire@oracle.com
[...]
- unlink(path);
- ret = mknod(path, mode, makedev(dev_major, dev_minor));
- ASSERT_EQ(ret, expected_ret, "mknod");
no need to unlink unless "if (!ret)"
Indeed, you are right.
[...]
- skel = dev_cgroup__open_and_load();
- if (!ASSERT_OK_PTR(skel, "load program"))
goto cleanup_cgroup;- if (!ASSERT_OK(bpf_prog_attach(bpf_program__fd(skel->progs.bpf_prog1),
cgroup_fd, BPF_CGROUP_DEVICE, 0),"attach_program"))I'd suggest using bpf_program__attach_cgroup() here as you can assign the link in the skeleton; see prog_tests/cgroup_v1v2.c.
Ah yes, thanks for the hint !
goto cleanup_progs;- if (test__start_subtest("deny-mknod"))
test_mknod("/dev/test_dev_cgroup_zero", S_IFCHR, 1, 5, -EPERM);nit: group with other deny subtests.
ACK
- if (test__start_subtest("allow-mknod"))
test_mknod("/dev/test_dev_cgroup_null", S_IFCHR, 1, 3, 0);- if (test__start_subtest("allow-read"))
test_read("/dev/urandom", buf, TEST_BUFFER_SIZE, TEST_BUFFER_SIZE);Nit: should we have a separate garbage buffer for the successful /dev/urandom read? We're not validating buffer contents anywhere but we will overwrite our test string I think and it'll end up non-null terminated.
True, but since the tests aren't performing any string operation on it, is it really a big deal ? I can even switch the string to a byte array, if it can prevent any mistake.
There's no need, don't worry. As long as the size limits ensure we don't overrun the buffer, we're good.
If that's ok for you, I can bring all the suggestions discussed here in a new revision and keep your review tag.
Sounds great, thanks!
Alan