Code refactoring #34
Comments
|
Note that #1 is related to refactoring the internal structures of Landlock, but probably the implementation of more efficient data structure for domains should be a separate patch. |
|
Some of the common helpers should be inlined if it's not possible to generalize them [1]. |
|
Hello @l0kod, @gnoack! ObjectiveRefactor MotivationAs @gnoack noted putting conditionals in the test is often considered bad practice (e.g.). Tests are becoming more complicated, harder to read and extend. For example, this part of static void test_bind_and_connect(struct __test_metadata *const _metadata,
const struct service_fixture *const srv,
const bool deny_bind, const bool deny_connect)
[...]
ret = connect_variant_addrlen(inval_fd, srv, get_addrlen(srv, true));
if (srv->protocol.domain == AF_UNIX) {
EXPECT_EQ(-EINVAL, ret);
} else if (deny_connect) {
EXPECT_EQ(-EACCES, ret);
} else if (srv->protocol.type == SOCK_STREAM) {
/* No listening server, whatever the value of deny_bind. */
EXPECT_EQ(-ECONNREFUSED, ret);
} else {
EXPECT_EQ(0, ret)
{
TH_LOG("Failed to connect to socket: %s",
strerror(errno));
}
}
[...]This problem has already appeared in two patches that I'm working on:
Both cases lead to the addition of new conditionals to existing tests, which negatively affects complexity and readability. Proposed solution
Outline of changesFor example, TEST_F(protocol, bind)
{
[...]
test_bind_and_connect(_metadata, &self->srv0, false, false);
test_bind_and_connect(_metadata, &self->srv1,
is_restricted(&variant->prot, variant->sandbox), false);
test_bind_and_connect(_metadata, &self->srv2,
is_restricted(&variant->prot, variant->sandbox),
is_restricted(&variant->prot, variant->sandbox));
}can be refactored like that: static void test_tcp_stream_allowed_bind_allowed_connect(
struct __test_metadata *const _metadata,
const struct service_fixture *const srv)
{
EXPECT_EQ(0, test_bind_variant(_metadata, srv));
EXPECT_EQ(-ECONNREFUSED, test_connect_variant(_metadata, srv));
/* Simple server-client connection trial. */
test_allowed_connection(_metadata, srv);
}
static void test_tcp_stream_denied_bind_allowed_connect(
struct __test_metadata *const _metadata,
const struct service_fixture *const srv)
{
EXPECT_EQ(-EACCES, test_bind_variant(_metadata, srv));
EXPECT_EQ(-ECONNREFUSED, test_connect_variant(_metadata, srv));
}
static void test_tcp_stream_allowed_bind_denied_connect(
struct __test_metadata *const _metadata,
const struct service_fixture *const srv)
{
EXPECT_EQ(0, test_bind_variant(_metadata, srv));
EXPECT_EQ(-EACCES, test_connect_variant(_metadata, srv));
test_denied_connection(_metadata, srv);
}
static void test_tcp_stream_denied_bind_denied_connect(
struct __test_metadata *const _metadata,
const struct service_fixture *const srv)
{
EXPECT_EQ(-EACCES, test_bind_variant(_metadata, srv));
EXPECT_EQ(-EACCES, test_connect_variant(_metadata, srv));
}
static void
test_not_restricted_stream_bind_connect(struct __test_metadata *const _metadata,
const struct service_fixture *const srv)
{
EXPECT_EQ(0, test_bind_variant(_metadata, srv));
EXPECT_EQ(-ECONNREFUSED, test_connect_variant(_metadata, srv));
test_allowed_connection(_metadata, srv);
}
static void
test_non_stream_bind_connect(struct __test_metadata *const _metadata,
const struct service_fixture *const srv)
{
EXPECT_EQ(0, test_bind_variant(_metadata, srv));
EXPECT_EQ(0, test_connect_variant(_metadata, srv));
}
TEST_F(protocol, bind)
{
[...]
if (is_tcp(variant) && is_sandboxed(variant)) {
test_tcp_stream_allowed_bind_allowed_connect(_metadata,
&self->srv0);
test_tcp_stream_denied_bind_allowed_connect(_metadata,
&self->srv1);
test_tcp_stream_denied_bind_denied_connect(_metadata,
&self->srv2);
} else if (is_stream(variant)) {
test_not_restricted_stream_bind_connect(_metadata, &self->srv0);
test_not_restricted_stream_bind_connect(_metadata, &self->srv1);
test_not_restricted_stream_bind_connect(_metadata, &self->srv2);
} else {
test_non_stream_bind_connect(_metadata, &self->srv0);
test_non_stream_bind_connect(_metadata, &self->srv1);
test_non_stream_bind_connect(_metadata, &self->srv2);
}
} |
|
Sorry for the delay, you message wasn't lost but we were at the OSS/LSS/LPC conferences last week and I guess we had some work to catch up. I'm thinking about that and I'll answer tomorrow. |
|
I get it, thank you! |
|
Hi! Apologies from my side as well, the last two weeks went by in a blur, and I also had some surprises waiting for me at work after the conference ;-) If the different This is admittedly a shallow feedback without looking too deep in the code, I might be missing something. I'll have another deeper look at the code review on Friday. |
No problem! Please, take your time. Presentation was really good btw 👍
Yeah, I've considered such approach. It has following pros:
And cons:
To be sure that we're on the same, consider following example: TEST_F(protocol, connect_unspec)
{
[...]
EXPECT_EQ(0, bind_variant(bind_fd, &self->srv0));
EXPECT_EQ(srv0->err_listen, listen(bind_fd, backlog));
child = fork();
ASSERT_LE(0, child);
if (child == 0) {
int connect_fd, ret;
[...]
EXPECT_EQ(0, connect_variant(connect_fd, &self->srv0));
EXPECT_EQ(srv0->err_reconnect, connect_variant(connect_fd, &self->srv0));
[...]
EXPECT_EQ(srv0->err_disconnect, connect_variant(connect_fd, &self->unspec_any0));
EXPECT_EQ(srv0->err_reconnect, connect_variant(connect_fd, &self->srv0));
[...]
EXPECT_EQ(srv0->err_disconnect, connect_variant(connect_fd, &self->unspec_any0));
[...]
}
client_fd = bind_fd;
ASSERT_LE(srv1->err_accept, accept(bind_fd, NULL, 0));
[...]
}
|
About the fix, it should change as less as possible the kernel and test code to ease backporting down to Linux 6.10 (which is the oldest supported branch just after Linux 6.7). So this should not be an issue but we need to check against the latest changes merged for Linux 6.12: e1b061b So, the following changes should come after the #40 fix.
We should indeed try to write simpler tests and put the expected results directly in the tests. [...]
[...] What about something like this instead: struct expect_srv {
const struct service_fixture *const srv;
int err_bind;
int err_connect;
};
TEST_F(protocol, bind)
{
struct expect_srv expect0 = {
.srv = self->srv0,
};
struct expect_srv expect1 = {
.srv = self->srv1,
};
struct expect_srv expect2 = {
.srv = self->srv2,
};
if (is_tcp(variant) && is_sandboxed(variant)) {
expect1->err_bind = -ECONNREFUSED;
expect2->err_bind = -ECONNREFUSED;
expect2->err_connect = -EACCES;
}
// [...]
test_bind_connect_listen(_metadata, expect0));
test_bind_connect_listen(_metadata, expect1));
test_bind_connect_listen(_metadata, expect2));And then add the logic of what can be tested (e.g. listen depends on err_bind being 0) in the test_bind_connect_listen() helper. |
Ok, I'll send the fix first.
This is close to the approach proposed by @gnoack above. Placing error code in the fixture may be better in terms of the number of conditions. I'm not sure about adding variables for error codes (as I commented above), but if everyone agrees with this approach, let's use it. |
Fixtures are used to initialize common things, but these error codes may be different according to the sandbox, which is per-test (and then also per-variant).
This makes the code less verbose because we don't need to specify when an access request should be allowed. This will also be useful if we want to add more tests to some helpers (e.g. |
I sent a patch series to improve the current status: https://lore.kernel.org/all/[email protected]/ |
We can try to improve landlock code structure and extensibility, remove repetitive patterns.
As @gnoack suggested it'll be effective to gather refactoring ideas before implementing the patch itself. It'll probably be convenient to suggest any ideas in the comments on this issue so that they can be discussed and approved or rejected here. All approved ideas can be marked in the description of this issue.
After collecting ideas, someone can be assigned to implement the appropriate patch.
The text was updated successfully, but these errors were encountered: