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.
If that's ok for you, I can bring all the suggestions discussed here in a new revision and keep your review tag.
Thanks,
Alexis
Alan