On 11/07/2022 18:27, Günther Noack wrote:
On Fri, Jul 08, 2022 at 01:17:46PM +0200, Mickaël Salaün wrote:
[...]
+ {
+ .path = file1_s1d1,
+ .access = LANDLOCK_ACCESS_FS_READ_FILE |
+ LANDLOCK_ACCESS_FS_WRITE_FILE |
+ LANDLOCK_ACCESS_FS_TRUNCATE,
+ },
+ {
+ .path = file2_s1d2,
+ .access = LANDLOCK_ACCESS_FS_READ_FILE |
+ LANDLOCK_ACCESS_FS_WRITE_FILE,
+ },
+ {
+ .path = file1_s1d2,
+ .access = LANDLOCK_ACCESS_FS_TRUNCATE,
+ },
Move this entry before file2_s1d2 to keep the paths sorted and make this
easier to read. You can change the access rights per path to also keep their
ordering though.
I've admittedly found it difficult to remember which of these files
and subdirectories exist and how they are named and mixed up the names
at least twice when developing these tests. To make it easier, I've now
renamed these by including this at the top of the test:
char *file_rwt = file1_s1d1;
char *file_rw = file2_s1s1;
// etc
With the test using names like file_rwt, I find that easier to work
with and found myself jumping less between the "rules" on top and the
place where the assertions are written out.
This is admittedly a bit out of line with the other tests, but maybe
it's worth doing? Let me know what you think.
It indeed makes things clearer.
+ {
+ .path = dir_s2d3,
+ .access = LANDLOCK_ACCESS_FS_TRUNCATE,
+ },
+ // Implicitly: No access rights for file2_s1d1.
Comment to move after the use of file1_s1d1.
I'm understanding this as "keep the files in order according to the
layout". I've reshuffled things a bit by renaming them, but this is
also in the right order now.
Right.
[...]
+ reg_fd = open(file1_s1d1, O_RDWR | O_CLOEXEC);
+ ASSERT_LE(0, reg_fd);
+ EXPECT_EQ(0, ftruncate(reg_fd, 10));
You should not use EXPECT but ASSERT here. I use EXPECT when an error could
block a test or when it could stop a cleanup (i.e. teardown).
ASSERT is the variant that stops the test immediately, whereas EXPECT
notes down the test failure and continues execution.
So in that spirit, I tried to use:
* ASSERT for successful open() calls where the FD is still needed later
* ASSERT for close() (for symmetry with open())
* EXPECT for expected-failing open() calls where the FD is not used later
* EXPECT for everything else
I understand your logic, but this gymnastic adds complexity to writing
tests (which might be difficult to explain) for not much gain. Indeed,
all these tests should pass, except if we add a SKIP (cf.
https://lore.kernel.org/all/20220628222941.2642917-1-jeffxu@xxxxxxxxxx/).
In the case of an open FD, it will not be an issue to not close it if a
test failed, which is not the same with FIXTURE_TEARDOWN where we want
the workspace to be clean after tests, whether they succeeded or failed.
I had another pass over the tests and have started to use EXPECT for a
few expected-failing open() calls.
The selftest framework seems inspired by the Googletest framework
(https://google.github.io/googletest/primer.html#assertions) where
this is described as: "Usually EXPECT_* are preferred, as they allow
more than one failure to be reported in a test. However, you should
use ASSERT_* if it doesn’t make sense to continue when the assertion
in question fails."
I think this is good in theory, but in practice, at least for the
Landlock selftests, everything should pass. Any test failure is a
blocker because it breaks the contract with users.
I find it very difficult to write tests that would check as much as
possible, even if some of these tests failed, without unexpected
behaviors (e.g. blocking the whole tests, writing to unexpected
locations…) because it changes the previous state from a known state to
a set of potential states (e.g. when creating or removing files). Doing
it generically increases complexity for tests which may already be
difficult to understand. When investigating a test failure, we can still
replace some ASSERT with EXPECT though.
I imagined that the same advice would apply to the kernel selftests?
Please let me know if I'm overlooking subtle differences here.
I made kselftest_harness.h generally available (outside of seccomp) but
I guess each subsystem maintainer might handle that differently.
See
https://lore.kernel.org/all/1b043379-b6eb-d272-c9b9-25c6960e1ef1@xxxxxxxxxxx/
for similar concerns.
