From 3d6c6f9b9e957735a801b8d1587508dea9599795 Mon Sep 17 00:00:00 2001 From: Daan De Meyer Date: Wed, 24 Jul 2024 13:27:46 +0200 Subject: [PATCH 01/13] mkosi: Update to latest Includes the required fix to make mkosi copy NOCOW disk images properly. --- .github/workflows/mkosi.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/mkosi.yml b/.github/workflows/mkosi.yml index 7070e9c636..28b2829bbd 100644 --- a/.github/workflows/mkosi.yml +++ b/.github/workflows/mkosi.yml @@ -105,7 +105,7 @@ jobs: steps: - uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 - - uses: systemd/mkosi@7e975957a6af65c2e70428b6cda0c163ca7e1adc + - uses: systemd/mkosi@4eba736412c702bbbe2c6d4a58a92fa977219249 # Freeing up disk space with rm -rf can take multiple minutes. Since we don't need the extra free space # immediately, we remove the files in the background. However, we first move them to a different location From 8eb06fd162c8828739f8619437351d98d7105ec9 Mon Sep 17 00:00:00 2001 From: Daan De Meyer Date: Wed, 24 Jul 2024 09:55:21 +0200 Subject: [PATCH 02/13] test: Add ASSERT_FAIL() --- src/shared/tests.h | 10 ++++++++++ src/test/test-macro.c | 5 +++++ 2 files changed, 15 insertions(+) diff --git a/src/shared/tests.h b/src/shared/tests.h index 820c3f70b7..ced403483c 100644 --- a/src/shared/tests.h +++ b/src/shared/tests.h @@ -227,6 +227,16 @@ static inline int run_test_table(void) { } \ }) +#define ASSERT_FAIL(expr) \ + ({ \ + typeof(expr) _result = (expr); \ + if (_result >= 0) { \ + log_error_errno(_result, "%s:%i: Assertion failed: expected \"%s\" to fail, but it succeeded", \ + PROJECT_FILE, __LINE__, #expr); \ + abort(); \ + } \ + }) + #define ASSERT_ERROR(expr1, expr2) \ ({ \ int _expr1 = (expr1); \ diff --git a/src/test/test-macro.c b/src/test/test-macro.c index 9e2875d8a7..1d2993a3ae 100644 --- a/src/test/test-macro.c +++ b/src/test/test-macro.c @@ -1122,6 +1122,11 @@ TEST(ASSERT) { ASSERT_SIGNAL(ASSERT_OK_ERRNO(-1), SIGABRT); ASSERT_SIGNAL(ASSERT_OK_ERRNO(-ENOANO), SIGABRT); + ASSERT_FAIL(-ENOENT); + ASSERT_FAIL(-EPERM); + ASSERT_SIGNAL(ASSERT_FAIL(0), SIGABRT); + ASSERT_SIGNAL(ASSERT_FAIL(255), SIGABRT); + ASSERT_ERROR(-ENOENT, ENOENT); ASSERT_ERROR(RET_NERRNO(mkdir("/i/will/fail/with/enoent", 666)), ENOENT); ASSERT_SIGNAL(ASSERT_ERROR(0, ENOENT), SIGABRT); From 60a4505da4cfb57653fac79a79745e32ab0bc311 Mon Sep 17 00:00:00 2001 From: Daan De Meyer Date: Wed, 24 Jul 2024 09:56:01 +0200 Subject: [PATCH 03/13] test: Add ASSERT_EQ_ID128() and ASSERT_NE_ID128() --- src/shared/tests.h | 27 +++++++++++++++++++++++++++ src/test/test-macro.c | 10 ++++++++++ 2 files changed, 37 insertions(+) diff --git a/src/shared/tests.h b/src/shared/tests.h index ced403483c..121d4f0edd 100644 --- a/src/shared/tests.h +++ b/src/shared/tests.h @@ -429,3 +429,30 @@ static inline int run_test_table(void) { abort(); \ } \ }) + + +#define ASSERT_EQ_ID128(expr1, expr2) \ + ({ \ + typeof(expr1) _expr1 = (expr1); \ + typeof(expr2) _expr2 = (expr2); \ + if (!sd_id128_equal(_expr1, _expr2)) { \ + log_error("%s:%i: Assertion failed: \"%s == %s\", but \"%s != %s\"", \ + PROJECT_FILE, __LINE__, \ + #expr1, #expr2, \ + SD_ID128_TO_STRING(_expr1), SD_ID128_TO_STRING(_expr2)); \ + abort(); \ + } \ + }) + +#define ASSERT_NE_ID128(expr1, expr2) \ + ({ \ + typeof(expr1) _expr1 = (expr1); \ + typeof(expr2) _expr2 = (expr2); \ + if (sd_id128_equal(_expr1, _expr2)) { \ + log_error("%s:%i: Assertion failed: \"%s != %s\", but \"%s == %s\"", \ + PROJECT_FILE, __LINE__, \ + #expr1, #expr2, \ + SD_ID128_TO_STRING(_expr1), SD_ID128_TO_STRING(_expr2)); \ + abort(); \ + } \ + }) diff --git a/src/test/test-macro.c b/src/test/test-macro.c index 1d2993a3ae..3d5b0cfeed 100644 --- a/src/test/test-macro.c +++ b/src/test/test-macro.c @@ -4,6 +4,7 @@ #include #include "errno-util.h" +#include "id128-util.h" #include "log.h" #include "macro.h" #include "tests.h" @@ -1186,6 +1187,15 @@ TEST(ASSERT) { ASSERT_LT(-1, 1); ASSERT_SIGNAL(ASSERT_LT(0, 0), SIGABRT); ASSERT_SIGNAL(ASSERT_LT(1, -1), SIGABRT); + + ASSERT_EQ_ID128(SD_ID128_NULL, SD_ID128_NULL); + ASSERT_NE_ID128(SD_ID128_MAKE(51,df,0b,4b,c3,b0,4c,97,80,e2,99,b9,8c,a3,73,b8), + SD_ID128_MAKE(f0,3d,aa,eb,1c,33,4b,43,a7,32,17,29,44,bf,77,2e)); + ASSERT_SIGNAL( + ASSERT_EQ_ID128(SD_ID128_MAKE(51,df,0b,4b,c3,b0,4c,97,80,e2,99,b9,8c,a3,73,b8), + SD_ID128_MAKE(f0,3d,aa,eb,1c,33,4b,43,a7,32,17,29,44,bf,77,2e)), + SIGABRT); + ASSERT_SIGNAL(ASSERT_NE_ID128(SD_ID128_NULL, SD_ID128_NULL), SIGABRT); } DEFINE_TEST_MAIN(LOG_INFO); From 372e96a5a10630eb2e02111672221428e0be3fb8 Mon Sep 17 00:00:00 2001 From: Daan De Meyer Date: Wed, 24 Jul 2024 09:56:25 +0200 Subject: [PATCH 04/13] test-id128: Use new assertion macros --- src/test/test-id128.c | 318 +++++++++++++++++++++--------------------- 1 file changed, 159 insertions(+), 159 deletions(-) diff --git a/src/test/test-id128.c b/src/test/test-id128.c index 48fdbba6c7..a6ed640bd6 100644 --- a/src/test/test-id128.c +++ b/src/test/test-id128.c @@ -28,31 +28,31 @@ TEST(id128) { _cleanup_free_ char *b = NULL; _cleanup_close_ int fd = -EBADF; - assert_se(sd_id128_randomize(&id) == 0); + ASSERT_OK(sd_id128_randomize(&id)); printf("random: %s\n", sd_id128_to_string(id, t)); - assert_se(sd_id128_from_string(t, &id2) == 0); - assert_se(sd_id128_equal(id, id2)); - assert_se(sd_id128_in_set(id, id)); - assert_se(sd_id128_in_set(id, id2)); - assert_se(sd_id128_in_set(id, id2, id)); - assert_se(sd_id128_in_set(id, ID128_WALDI, id)); - assert_se(!sd_id128_in_set(id)); - assert_se(!sd_id128_in_set(id, ID128_WALDI)); - assert_se(!sd_id128_in_set(id, ID128_WALDI, ID128_WALDI)); + ASSERT_OK(sd_id128_from_string(t, &id2) == 0); + ASSERT_EQ_ID128(id, id2); + ASSERT_TRUE(sd_id128_in_set(id, id)); + ASSERT_TRUE(sd_id128_in_set(id, id2)); + ASSERT_TRUE(sd_id128_in_set(id, id2, id)); + ASSERT_TRUE(sd_id128_in_set(id, ID128_WALDI, id)); + ASSERT_FALSE(sd_id128_in_set(id)); + ASSERT_FALSE(sd_id128_in_set(id, ID128_WALDI)); + ASSERT_FALSE(sd_id128_in_set(id, ID128_WALDI, ID128_WALDI)); if (sd_booted() > 0 && sd_id128_get_machine(NULL) >= 0) { - assert_se(sd_id128_get_machine(&id) == 0); + ASSERT_OK(sd_id128_get_machine(&id)); printf("machine: %s\n", sd_id128_to_string(id, t)); - assert_se(sd_id128_get_boot(&id) == 0); + ASSERT_OK(sd_id128_get_boot(&id)); printf("boot: %s\n", sd_id128_to_string(id, t)); } printf("waldi: %s\n", sd_id128_to_string(ID128_WALDI, t)); ASSERT_STREQ(t, STR_WALDI); - assert_se(asprintf(&b, SD_ID128_FORMAT_STR, SD_ID128_FORMAT_VAL(ID128_WALDI)) == 32); + ASSERT_EQ(asprintf(&b, SD_ID128_FORMAT_STR, SD_ID128_FORMAT_VAL(ID128_WALDI)), 32); printf("waldi2: %s\n", b); ASSERT_STREQ(t, b); @@ -60,137 +60,137 @@ TEST(id128) { ASSERT_STREQ(q, UUID_WALDI); b = mfree(b); - assert_se(asprintf(&b, SD_ID128_UUID_FORMAT_STR, SD_ID128_FORMAT_VAL(ID128_WALDI)) == 36); + ASSERT_EQ(asprintf(&b, SD_ID128_UUID_FORMAT_STR, SD_ID128_FORMAT_VAL(ID128_WALDI)), 36); printf("waldi4: %s\n", b); ASSERT_STREQ(q, b); - assert_se(sd_id128_from_string(STR_WALDI, &id) >= 0); - assert_se(sd_id128_equal(id, ID128_WALDI)); + ASSERT_OK(sd_id128_from_string(STR_WALDI, &id)); + ASSERT_EQ_ID128(id, ID128_WALDI); - assert_se(sd_id128_from_string(UUID_WALDI, &id) >= 0); - assert_se(sd_id128_equal(id, ID128_WALDI)); + ASSERT_OK(sd_id128_from_string(UUID_WALDI, &id)); + ASSERT_EQ_ID128(id, ID128_WALDI); - assert_se(sd_id128_from_string("", &id) < 0); - assert_se(sd_id128_from_string("01020304-0506-0708-090a-0b0c0d0e0f101", &id) < 0); - assert_se(sd_id128_from_string("01020304-0506-0708-090a-0b0c0d0e0f10-", &id) < 0); - assert_se(sd_id128_from_string("01020304-0506-0708-090a0b0c0d0e0f10", &id) < 0); - assert_se(sd_id128_from_string("010203040506-0708-090a-0b0c0d0e0f10", &id) < 0); + ASSERT_FAIL(sd_id128_from_string("", &id)); + ASSERT_FAIL(sd_id128_from_string("01020304-0506-0708-090a-0b0c0d0e0f101", &id)); + ASSERT_FAIL(sd_id128_from_string("01020304-0506-0708-090a-0b0c0d0e0f10-", &id)); + ASSERT_FAIL(sd_id128_from_string("01020304-0506-0708-090a0b0c0d0e0f10", &id)); + ASSERT_FAIL(sd_id128_from_string("010203040506-0708-090a-0b0c0d0e0f10", &id)); - assert_se(id128_from_string_nonzero(STR_WALDI, &id) == 0); - assert_se(id128_from_string_nonzero(STR_NULL, &id) == -ENXIO); - assert_se(id128_from_string_nonzero("01020304-0506-0708-090a-0b0c0d0e0f101", &id) < 0); - assert_se(id128_from_string_nonzero("01020304-0506-0708-090a-0b0c0d0e0f10-", &id) < 0); - assert_se(id128_from_string_nonzero("01020304-0506-0708-090a0b0c0d0e0f10", &id) < 0); - assert_se(id128_from_string_nonzero("010203040506-0708-090a-0b0c0d0e0f10", &id) < 0); + ASSERT_OK(id128_from_string_nonzero(STR_WALDI, &id)); + ASSERT_ERROR(id128_from_string_nonzero(STR_NULL, &id), ENXIO); + ASSERT_FAIL(id128_from_string_nonzero("01020304-0506-0708-090a-0b0c0d0e0f101", &id)); + ASSERT_FAIL(id128_from_string_nonzero("01020304-0506-0708-090a-0b0c0d0e0f10-", &id)); + ASSERT_FAIL(id128_from_string_nonzero("01020304-0506-0708-090a0b0c0d0e0f10", &id)); + ASSERT_FAIL(id128_from_string_nonzero("010203040506-0708-090a-0b0c0d0e0f10", &id)); - assert_se(id128_is_valid(STR_WALDI)); - assert_se(id128_is_valid(UUID_WALDI)); - assert_se(!id128_is_valid("")); - assert_se(!id128_is_valid("01020304-0506-0708-090a-0b0c0d0e0f101")); - assert_se(!id128_is_valid("01020304-0506-0708-090a-0b0c0d0e0f10-")); - assert_se(!id128_is_valid("01020304-0506-0708-090a0b0c0d0e0f10")); - assert_se(!id128_is_valid("010203040506-0708-090a-0b0c0d0e0f10")); + ASSERT_TRUE(id128_is_valid(STR_WALDI)); + ASSERT_TRUE(id128_is_valid(UUID_WALDI)); + ASSERT_FALSE(id128_is_valid("")); + ASSERT_FALSE(id128_is_valid("01020304-0506-0708-090a-0b0c0d0e0f101")); + ASSERT_FALSE(id128_is_valid("01020304-0506-0708-090a-0b0c0d0e0f10-")); + ASSERT_FALSE(id128_is_valid("01020304-0506-0708-090a0b0c0d0e0f10")); + ASSERT_FALSE(id128_is_valid("010203040506-0708-090a-0b0c0d0e0f10")); fd = open_tmpfile_unlinkable(NULL, O_RDWR|O_CLOEXEC); - assert_se(fd >= 0); + ASSERT_OK(fd); /* First, write as UUID */ - assert_se(sd_id128_randomize(&id) >= 0); - assert_se(id128_write_fd(fd, ID128_FORMAT_UUID, id) >= 0); + ASSERT_OK(sd_id128_randomize(&id)); + ASSERT_OK(id128_write_fd(fd, ID128_FORMAT_UUID, id)); - assert_se(lseek(fd, 0, SEEK_SET) == 0); - assert_se(id128_read_fd(fd, ID128_FORMAT_PLAIN, &id2) == -EUCLEAN); + ASSERT_OK_ERRNO(lseek(fd, 0, SEEK_SET)); + ASSERT_ERROR(id128_read_fd(fd, ID128_FORMAT_PLAIN, &id2), EUCLEAN); - assert_se(lseek(fd, 0, SEEK_SET) == 0); - assert_se(id128_read_fd(fd, ID128_FORMAT_UUID, &id2) >= 0); - assert_se(sd_id128_equal(id, id2)); + ASSERT_OK_ERRNO(lseek(fd, 0, SEEK_SET)); + ASSERT_OK(id128_read_fd(fd, ID128_FORMAT_UUID, &id2)); + ASSERT_EQ_ID128(id, id2); - assert_se(lseek(fd, 0, SEEK_SET) == 0); - assert_se(id128_read_fd(fd, ID128_FORMAT_ANY, &id2) >= 0); - assert_se(sd_id128_equal(id, id2)); + ASSERT_OK_ERRNO(lseek(fd, 0, SEEK_SET)); + ASSERT_OK(id128_read_fd(fd, ID128_FORMAT_ANY, &id2)); + ASSERT_EQ_ID128(id, id2); /* Second, write as plain */ - assert_se(lseek(fd, 0, SEEK_SET) == 0); - assert_se(ftruncate(fd, 0) >= 0); + ASSERT_OK_ERRNO(lseek(fd, 0, SEEK_SET)); + ASSERT_OK_ERRNO(ftruncate(fd, 0)); - assert_se(sd_id128_randomize(&id) >= 0); - assert_se(id128_write_fd(fd, ID128_FORMAT_PLAIN, id) >= 0); + ASSERT_OK(sd_id128_randomize(&id)); + ASSERT_OK(id128_write_fd(fd, ID128_FORMAT_PLAIN, id)); - assert_se(lseek(fd, 0, SEEK_SET) == 0); - assert_se(id128_read_fd(fd, ID128_FORMAT_UUID, &id2) == -EUCLEAN); + ASSERT_OK_ERRNO(lseek(fd, 0, SEEK_SET) == 0); + ASSERT_ERROR(id128_read_fd(fd, ID128_FORMAT_UUID, &id2), EUCLEAN); - assert_se(lseek(fd, 0, SEEK_SET) == 0); - assert_se(id128_read_fd(fd, ID128_FORMAT_PLAIN, &id2) >= 0); - assert_se(sd_id128_equal(id, id2)); + ASSERT_OK_ERRNO(lseek(fd, 0, SEEK_SET)); + ASSERT_OK(id128_read_fd(fd, ID128_FORMAT_PLAIN, &id2)); + ASSERT_EQ_ID128(id, id2); - assert_se(lseek(fd, 0, SEEK_SET) == 0); - assert_se(id128_read_fd(fd, ID128_FORMAT_ANY, &id2) >= 0); - assert_se(sd_id128_equal(id, id2)); + ASSERT_OK_ERRNO(lseek(fd, 0, SEEK_SET)); + ASSERT_OK(id128_read_fd(fd, ID128_FORMAT_ANY, &id2)); + ASSERT_EQ_ID128(id, id2); /* Third, write plain without trailing newline */ - assert_se(lseek(fd, 0, SEEK_SET) == 0); - assert_se(ftruncate(fd, 0) >= 0); + ASSERT_OK_ERRNO(lseek(fd, 0, SEEK_SET)); + ASSERT_OK_ERRNO(ftruncate(fd, 0)); - assert_se(sd_id128_randomize(&id) >= 0); - assert_se(write(fd, sd_id128_to_string(id, t), 32) == 32); + ASSERT_OK(sd_id128_randomize(&id)); + ASSERT_EQ(write(fd, sd_id128_to_string(id, t), 32), 32); - assert_se(lseek(fd, 0, SEEK_SET) == 0); - assert_se(id128_read_fd(fd, ID128_FORMAT_UUID, &id2) == -EUCLEAN); + ASSERT_OK_ERRNO(lseek(fd, 0, SEEK_SET)); + ASSERT_ERROR(id128_read_fd(fd, ID128_FORMAT_UUID, &id2), EUCLEAN); - assert_se(lseek(fd, 0, SEEK_SET) == 0); - assert_se(id128_read_fd(fd, ID128_FORMAT_PLAIN, &id2) >= 0); - assert_se(sd_id128_equal(id, id2)); + ASSERT_OK_ERRNO(lseek(fd, 0, SEEK_SET)); + ASSERT_OK(id128_read_fd(fd, ID128_FORMAT_PLAIN, &id2)); + ASSERT_EQ_ID128(id, id2); /* Fourth, write UUID without trailing newline */ - assert_se(lseek(fd, 0, SEEK_SET) == 0); - assert_se(ftruncate(fd, 0) >= 0); + ASSERT_OK_ERRNO(lseek(fd, 0, SEEK_SET)); + ASSERT_OK_ERRNO(ftruncate(fd, 0)); - assert_se(sd_id128_randomize(&id) >= 0); - assert_se(write(fd, sd_id128_to_uuid_string(id, q), 36) == 36); + ASSERT_OK(sd_id128_randomize(&id)); + ASSERT_EQ(write(fd, sd_id128_to_uuid_string(id, q), 36), 36); - assert_se(lseek(fd, 0, SEEK_SET) == 0); - assert_se(id128_read_fd(fd, ID128_FORMAT_PLAIN, &id2) == -EUCLEAN); + ASSERT_OK_ERRNO(lseek(fd, 0, SEEK_SET)); + ASSERT_ERROR(id128_read_fd(fd, ID128_FORMAT_PLAIN, &id2), EUCLEAN); - assert_se(lseek(fd, 0, SEEK_SET) == 0); - assert_se(id128_read_fd(fd, ID128_FORMAT_UUID, &id2) >= 0); - assert_se(sd_id128_equal(id, id2)); + ASSERT_OK_ERRNO(lseek(fd, 0, SEEK_SET)); + ASSERT_OK(id128_read_fd(fd, ID128_FORMAT_UUID, &id2)); + ASSERT_EQ_ID128(id, id2); /* Fifth, tests for "uninitialized" */ - assert_se(lseek(fd, 0, SEEK_SET) == 0); - assert_se(ftruncate(fd, 0) >= 0); - assert_se(write(fd, "uninitialized", STRLEN("uninitialized")) == STRLEN("uninitialized")); - assert_se(lseek(fd, 0, SEEK_SET) == 0); - assert_se(id128_read_fd(fd, ID128_FORMAT_ANY, NULL) == -ENOPKG); + ASSERT_OK_ERRNO(lseek(fd, 0, SEEK_SET)); + ASSERT_OK_ERRNO(ftruncate(fd, 0)); + ASSERT_EQ(write(fd, "uninitialized", STRLEN("uninitialized")), (ssize_t) STRLEN("uninitialized")); + ASSERT_OK_ERRNO(lseek(fd, 0, SEEK_SET)); + ASSERT_ERROR(id128_read_fd(fd, ID128_FORMAT_ANY, NULL), ENOPKG); - assert_se(lseek(fd, 0, SEEK_SET) == 0); - assert_se(ftruncate(fd, 0) >= 0); - assert_se(write(fd, "uninitialized\n", STRLEN("uninitialized\n")) == STRLEN("uninitialized\n")); - assert_se(lseek(fd, 0, SEEK_SET) == 0); - assert_se(id128_read_fd(fd, ID128_FORMAT_ANY, NULL) == -ENOPKG); + ASSERT_OK_ERRNO(lseek(fd, 0, SEEK_SET)); + ASSERT_OK_ERRNO(ftruncate(fd, 0)); + ASSERT_EQ(write(fd, "uninitialized\n", STRLEN("uninitialized\n")), (ssize_t) STRLEN("uninitialized\n")); + ASSERT_OK_ERRNO(lseek(fd, 0, SEEK_SET)); + ASSERT_ERROR(id128_read_fd(fd, ID128_FORMAT_ANY, NULL), ENOPKG); - assert_se(lseek(fd, 0, SEEK_SET) == 0); - assert_se(ftruncate(fd, 0) >= 0); - assert_se(write(fd, "uninitialized\nfoo", STRLEN("uninitialized\nfoo")) == STRLEN("uninitialized\nfoo")); - assert_se(lseek(fd, 0, SEEK_SET) == 0); - assert_se(id128_read_fd(fd, ID128_FORMAT_ANY, NULL) == -EUCLEAN); + ASSERT_OK_ERRNO(lseek(fd, 0, SEEK_SET)); + ASSERT_OK_ERRNO(ftruncate(fd, 0)); + ASSERT_EQ(write(fd, "uninitialized\nfoo", STRLEN("uninitialized\nfoo")), (ssize_t) STRLEN("uninitialized\nfoo")); + ASSERT_OK_ERRNO(lseek(fd, 0, SEEK_SET)); + ASSERT_ERROR(id128_read_fd(fd, ID128_FORMAT_ANY, NULL), EUCLEAN); - assert_se(lseek(fd, 0, SEEK_SET) == 0); - assert_se(ftruncate(fd, 0) >= 0); - assert_se(write(fd, "uninit", STRLEN("uninit")) == STRLEN("uninit")); - assert_se(lseek(fd, 0, SEEK_SET) == 0); - assert_se(id128_read_fd(fd, ID128_FORMAT_ANY, NULL) == -EUCLEAN); + ASSERT_OK_ERRNO(lseek(fd, 0, SEEK_SET)); + ASSERT_OK_ERRNO(ftruncate(fd, 0)); + ASSERT_EQ(write(fd, "uninit", STRLEN("uninit")), (ssize_t) STRLEN("uninit")); + ASSERT_OK_ERRNO(lseek(fd, 0, SEEK_SET)); + ASSERT_ERROR(id128_read_fd(fd, ID128_FORMAT_ANY, NULL), EUCLEAN); /* build/systemd-id128 -a f03daaeb1c334b43a732172944bf772e show 51df0b4bc3b04c9780e299b98ca373b8 */ - assert_se(sd_id128_get_app_specific(SD_ID128_MAKE(51,df,0b,4b,c3,b0,4c,97,80,e2,99,b9,8c,a3,73,b8), - SD_ID128_MAKE(f0,3d,aa,eb,1c,33,4b,43,a7,32,17,29,44,bf,77,2e), &id) >= 0); - assert_se(sd_id128_equal(id, SD_ID128_MAKE(1d,ee,59,54,e7,5c,4d,6f,b9,6c,c6,c0,4c,a1,8a,86))); + ASSERT_OK(sd_id128_get_app_specific(SD_ID128_MAKE(51,df,0b,4b,c3,b0,4c,97,80,e2,99,b9,8c,a3,73,b8), + SD_ID128_MAKE(f0,3d,aa,eb,1c,33,4b,43,a7,32,17,29,44,bf,77,2e), &id)); + ASSERT_EQ_ID128(id, SD_ID128_MAKE(1d,ee,59,54,e7,5c,4d,6f,b9,6c,c6,c0,4c,a1,8a,86)); if (sd_booted() > 0 && sd_id128_get_machine(NULL) >= 0) { - assert_se(sd_id128_get_machine_app_specific(SD_ID128_MAKE(f0,3d,aa,eb,1c,33,4b,43,a7,32,17,29,44,bf,77,2e), &id) >= 0); - assert_se(sd_id128_get_machine_app_specific(SD_ID128_MAKE(f0,3d,aa,eb,1c,33,4b,43,a7,32,17,29,44,bf,77,2e), &id2) >= 0); - assert_se(sd_id128_equal(id, id2)); - assert_se(sd_id128_get_machine_app_specific(SD_ID128_MAKE(51,df,0b,4b,c3,b0,4c,97,80,e2,99,b9,8c,a3,73,b8), &id2) >= 0); - assert_se(!sd_id128_equal(id, id2)); + ASSERT_OK(sd_id128_get_machine_app_specific(SD_ID128_MAKE(f0,3d,aa,eb,1c,33,4b,43,a7,32,17,29,44,bf,77,2e), &id)); + ASSERT_OK(sd_id128_get_machine_app_specific(SD_ID128_MAKE(f0,3d,aa,eb,1c,33,4b,43,a7,32,17,29,44,bf,77,2e), &id2)); + ASSERT_EQ_ID128(id, id2); + ASSERT_OK(sd_id128_get_machine_app_specific(SD_ID128_MAKE(51,df,0b,4b,c3,b0,4c,97,80,e2,99,b9,8c,a3,73,b8), &id2)); + ASSERT_NE_ID128(id, id2); } /* Check return values */ @@ -214,7 +214,7 @@ TEST(sd_id128_get_invocation) { if (r < 0) log_warning_errno(r, "Failed to get invocation ID, ignoring: %m"); else { - assert(!sd_id128_equal(id, appid)); + ASSERT_NE_ID128(id, appid); log_info("Per-App Invocation ID: " SD_ID128_FORMAT_STR, SD_ID128_FORMAT_VAL(appid)); } @@ -223,8 +223,8 @@ TEST(sd_id128_get_invocation) { if (r < 0) log_warning_errno(r, "Failed to get invocation ID, ignoring: %m"); else { - assert(!sd_id128_equal(id, appid2)); - assert(!sd_id128_equal(appid, appid2)); + ASSERT_NE_ID128(id, appid2); + ASSERT_NE_ID128(appid, appid2); log_info("Per-App Invocation ID 2: " SD_ID128_FORMAT_STR, SD_ID128_FORMAT_VAL(appid2)); } @@ -233,9 +233,9 @@ TEST(sd_id128_get_invocation) { if (r < 0) log_warning_errno(r, "Failed to get invocation ID, ignoring: %m"); else { - assert(!sd_id128_equal(id, appid3)); - assert(sd_id128_equal(appid, appid3)); - assert(!sd_id128_equal(appid2, appid3)); + ASSERT_NE_ID128(id, appid3); + ASSERT_EQ_ID128(appid, appid3); + ASSERT_NE_ID128(appid2, appid3); log_info("Per-App Invocation ID 3: " SD_ID128_FORMAT_STR, SD_ID128_FORMAT_VAL(appid3)); } } @@ -256,7 +256,7 @@ TEST(benchmark_sd_id128_get_machine_app_specific) { for (unsigned i = 0; i < iterations; i++) { id.qwords[1] = i; - assert_se(sd_id128_get_machine_app_specific(id, &id2) >= 0); + ASSERT_OK(sd_id128_get_machine_app_specific(id, &id2)); } q = now(CLOCK_MONOTONIC) - t; @@ -271,79 +271,79 @@ TEST(id128_at) { sd_id128_t id, i; tfd = mkdtemp_open(NULL, O_PATH, &t); - assert_se(tfd >= 0); - assert_se(mkdirat(tfd, "etc", 0755) >= 0); - assert_se(symlinkat("etc", tfd, "etc2") >= 0); - assert_se(symlinkat("machine-id", tfd, "etc/hoge-id") >= 0); + ASSERT_OK(tfd); + ASSERT_OK_ERRNO(mkdirat(tfd, "etc", 0755)); + ASSERT_OK_ERRNO(symlinkat("etc", tfd, "etc2")); + ASSERT_OK_ERRNO(symlinkat("machine-id", tfd, "etc/hoge-id")); - assert_se(sd_id128_randomize(&id) == 0); + ASSERT_OK(sd_id128_randomize(&id)); - assert_se(id128_write_at(tfd, "etc/machine-id", ID128_FORMAT_PLAIN, id) >= 0); + ASSERT_OK(id128_write_at(tfd, "etc/machine-id", ID128_FORMAT_PLAIN, id)); if (geteuid() == 0) - assert_se(id128_write_at(tfd, "etc/machine-id", ID128_FORMAT_PLAIN, id) >= 0); + ASSERT_OK(id128_write_at(tfd, "etc/machine-id", ID128_FORMAT_PLAIN, id)); else - assert_se(id128_write_at(tfd, "etc/machine-id", ID128_FORMAT_PLAIN, id) == -EACCES); - assert_se(unlinkat(tfd, "etc/machine-id", 0) >= 0); - assert_se(id128_write_at(tfd, "etc2/machine-id", ID128_FORMAT_PLAIN, id) >= 0); - assert_se(unlinkat(tfd, "etc/machine-id", 0) >= 0); - assert_se(id128_write_at(tfd, "etc/hoge-id", ID128_FORMAT_PLAIN, id) >= 0); - assert_se(unlinkat(tfd, "etc/machine-id", 0) >= 0); - assert_se(id128_write_at(tfd, "etc2/hoge-id", ID128_FORMAT_PLAIN, id) >= 0); + ASSERT_ERROR(id128_write_at(tfd, "etc/machine-id", ID128_FORMAT_PLAIN, id), EACCES); + ASSERT_OK_ERRNO(unlinkat(tfd, "etc/machine-id", 0)); + ASSERT_OK(id128_write_at(tfd, "etc2/machine-id", ID128_FORMAT_PLAIN, id)); + ASSERT_OK_ERRNO(unlinkat(tfd, "etc/machine-id", 0)); + ASSERT_OK(id128_write_at(tfd, "etc/hoge-id", ID128_FORMAT_PLAIN, id)); + ASSERT_OK_ERRNO(unlinkat(tfd, "etc/machine-id", 0)); + ASSERT_OK(id128_write_at(tfd, "etc2/hoge-id", ID128_FORMAT_PLAIN, id)); /* id128_read_at() */ i = SD_ID128_NULL; /* Not necessary in real code, but for testing that the id is really assigned. */ - assert_se(id128_read_at(tfd, "etc/machine-id", ID128_FORMAT_PLAIN, &i) >= 0); - assert_se(sd_id128_equal(id, i)); + ASSERT_OK(id128_read_at(tfd, "etc/machine-id", ID128_FORMAT_PLAIN, &i)); + ASSERT_EQ_ID128(id, i); i = SD_ID128_NULL; - assert_se(id128_read_at(tfd, "etc2/machine-id", ID128_FORMAT_PLAIN, &i) >= 0); - assert_se(sd_id128_equal(id, i)); + ASSERT_OK(id128_read_at(tfd, "etc2/machine-id", ID128_FORMAT_PLAIN, &i)); + ASSERT_EQ_ID128(id, i); i = SD_ID128_NULL; - assert_se(id128_read_at(tfd, "etc/hoge-id", ID128_FORMAT_PLAIN, &i) >= 0); - assert_se(sd_id128_equal(id, i)); + ASSERT_OK(id128_read_at(tfd, "etc/hoge-id", ID128_FORMAT_PLAIN, &i)); + ASSERT_EQ_ID128(id, i); i = SD_ID128_NULL; - assert_se(id128_read_at(tfd, "etc2/hoge-id", ID128_FORMAT_PLAIN, &i) >= 0); - assert_se(sd_id128_equal(id, i)); + ASSERT_OK(id128_read_at(tfd, "etc2/hoge-id", ID128_FORMAT_PLAIN, &i)); + ASSERT_EQ_ID128(id, i); /* id128_read() */ - assert_se(p = path_join(t, "/etc/machine-id")); + ASSERT_NOT_NULL(p = path_join(t, "/etc/machine-id")); i = SD_ID128_NULL; - assert_se(id128_read(p, ID128_FORMAT_PLAIN, &i) >= 0); - assert_se(sd_id128_equal(id, i)); + ASSERT_OK(id128_read(p, ID128_FORMAT_PLAIN, &i)); + ASSERT_EQ_ID128(id, i); free(p); - assert_se(p = path_join(t, "/etc2/machine-id")); + ASSERT_NOT_NULL(p = path_join(t, "/etc2/machine-id")); i = SD_ID128_NULL; - assert_se(id128_read(p, ID128_FORMAT_PLAIN, &i) >= 0); - assert_se(sd_id128_equal(id, i)); + ASSERT_OK(id128_read(p, ID128_FORMAT_PLAIN, &i)); + ASSERT_EQ_ID128(id, i); free(p); - assert_se(p = path_join(t, "/etc/hoge-id")); + ASSERT_NOT_NULL(p = path_join(t, "/etc/hoge-id")); i = SD_ID128_NULL; - assert_se(id128_read(p, ID128_FORMAT_PLAIN, &i) >= 0); - assert_se(sd_id128_equal(id, i)); + ASSERT_OK(id128_read(p, ID128_FORMAT_PLAIN, &i)); + ASSERT_EQ_ID128(id, i); free(p); - assert_se(p = path_join(t, "/etc2/hoge-id")); + ASSERT_NOT_NULL(p = path_join(t, "/etc2/hoge-id")); i = SD_ID128_NULL; - assert_se(id128_read(p, ID128_FORMAT_PLAIN, &i) >= 0); - assert_se(sd_id128_equal(id, i)); + ASSERT_OK(id128_read(p, ID128_FORMAT_PLAIN, &i)); + ASSERT_EQ_ID128(id, i); /* id128_get_machine_at() */ i = SD_ID128_NULL; - assert_se(id128_get_machine_at(tfd, &i) >= 0); - assert_se(sd_id128_equal(id, i)); + ASSERT_OK(id128_get_machine_at(tfd, &i)); + ASSERT_EQ_ID128(id, i); /* id128_get_machine() */ i = SD_ID128_NULL; - assert_se(id128_get_machine(t, &i) >= 0); - assert_se(sd_id128_equal(id, i)); + ASSERT_OK(id128_get_machine(t, &i)); + ASSERT_EQ_ID128(id, i); } TEST(ID128_REFUSE_NULL) { @@ -352,18 +352,18 @@ TEST(ID128_REFUSE_NULL) { sd_id128_t id; tfd = mkdtemp_open(NULL, O_PATH, &t); - assert_se(tfd >= 0); + ASSERT_OK(tfd); - assert_se(id128_write_at(tfd, "zero-id", ID128_FORMAT_PLAIN | ID128_REFUSE_NULL, (sd_id128_t) {}) == -ENOMEDIUM); - assert_se(unlinkat(tfd, "zero-id", 0) >= 0); - assert_se(id128_write_at(tfd, "zero-id", ID128_FORMAT_PLAIN, (sd_id128_t) {}) >= 0); + ASSERT_ERROR(id128_write_at(tfd, "zero-id", ID128_FORMAT_PLAIN | ID128_REFUSE_NULL, (sd_id128_t) {}), ENOMEDIUM); + ASSERT_OK_ERRNO(unlinkat(tfd, "zero-id", 0)); + ASSERT_OK(id128_write_at(tfd, "zero-id", ID128_FORMAT_PLAIN, (sd_id128_t) {})); - assert_se(sd_id128_randomize(&id) == 0); - assert_se(!sd_id128_equal(id, SD_ID128_NULL)); - assert_se(id128_read_at(tfd, "zero-id", ID128_FORMAT_PLAIN, &id) >= 0); - assert_se(sd_id128_equal(id, SD_ID128_NULL)); + ASSERT_OK(sd_id128_randomize(&id)); + ASSERT_NE_ID128(id, SD_ID128_NULL); + ASSERT_OK(id128_read_at(tfd, "zero-id", ID128_FORMAT_PLAIN, &id)); + ASSERT_EQ_ID128(id, SD_ID128_NULL); - assert_se(id128_read_at(tfd, "zero-id", ID128_FORMAT_PLAIN | ID128_REFUSE_NULL, &id) == -ENOMEDIUM); + ASSERT_ERROR(id128_read_at(tfd, "zero-id", ID128_FORMAT_PLAIN | ID128_REFUSE_NULL, &id), ENOMEDIUM); } DEFINE_TEST_MAIN(LOG_INFO); From b91ad562280ec4f37ba970f1f167d2d626da2692 Mon Sep 17 00:00:00 2001 From: Daan De Meyer Date: Wed, 24 Jul 2024 10:50:38 +0200 Subject: [PATCH 05/13] test-fs-util: Modernize openat_report_new() test --- src/test/test-fs-util.c | 76 +++++++++++++++++++---------------------- 1 file changed, 36 insertions(+), 40 deletions(-) diff --git a/src/test/test-fs-util.c b/src/test/test-fs-util.c index d44e0437f5..f2fa51f55f 100644 --- a/src/test/test-fs-util.c +++ b/src/test/test-fs-util.c @@ -606,71 +606,67 @@ TEST(open_mkdir_at) { } TEST(openat_report_new) { - _cleanup_free_ char *j = NULL; - _cleanup_(rm_rf_physical_and_freep) char *d = NULL; - _cleanup_close_ int fd = -EBADF; + _cleanup_(rm_rf_physical_and_freep) char *t = NULL; + _cleanup_close_ int tfd = -EBADF, fd = -EBADF; bool b; - assert_se(mkdtemp_malloc(NULL, &d) >= 0); + ASSERT_OK((tfd = mkdtemp_open(NULL, 0, &t))); - j = path_join(d, "test"); - assert_se(j); - - fd = openat_report_new(AT_FDCWD, j, O_RDWR|O_CREAT, 0666, &b); - assert_se(fd >= 0); + fd = openat_report_new(tfd, "test", O_RDWR|O_CREAT, 0666, &b); + ASSERT_OK(fd); fd = safe_close(fd); - assert_se(b); + ASSERT_TRUE(b); - fd = openat_report_new(AT_FDCWD, j, O_RDWR|O_CREAT, 0666, &b); - assert_se(fd >= 0); + fd = openat_report_new(tfd, "test", O_RDWR|O_CREAT, 0666, &b); + ASSERT_OK(fd); fd = safe_close(fd); - assert_se(!b); + ASSERT_FALSE(b); - fd = openat_report_new(AT_FDCWD, j, O_RDWR|O_CREAT, 0666, &b); - assert_se(fd >= 0); + fd = openat_report_new(tfd, "test", O_RDWR|O_CREAT, 0666, &b); + ASSERT_OK(fd); fd = safe_close(fd); - assert_se(!b); + ASSERT_FALSE(b); - assert_se(unlink(j) >= 0); + ASSERT_OK_ERRNO(unlinkat(tfd, "test", 0)); - fd = openat_report_new(AT_FDCWD, j, O_RDWR|O_CREAT, 0666, &b); - assert_se(fd >= 0); + fd = openat_report_new(tfd, "test", O_RDWR|O_CREAT, 0666, &b); + ASSERT_OK(fd); fd = safe_close(fd); - assert_se(b); + ASSERT_TRUE(b); - fd = openat_report_new(AT_FDCWD, j, O_RDWR|O_CREAT, 0666, &b); - assert_se(fd >= 0); + fd = openat_report_new(tfd, "test", O_RDWR|O_CREAT, 0666, &b); + ASSERT_OK(fd); fd = safe_close(fd); - assert_se(!b); + ASSERT_FALSE(b); - assert_se(unlink(j) >= 0); + ASSERT_OK_ERRNO(unlinkat(tfd, "test", 0)); - fd = openat_report_new(AT_FDCWD, j, O_RDWR|O_CREAT, 0666, NULL); - assert_se(fd >= 0); + fd = openat_report_new(tfd, "test", O_RDWR|O_CREAT, 0666, NULL); + ASSERT_OK(fd); fd = safe_close(fd); - fd = openat_report_new(AT_FDCWD, j, O_RDWR|O_CREAT, 0666, &b); - assert_se(fd >= 0); + fd = openat_report_new(tfd, "test", O_RDWR|O_CREAT, 0666, &b); + ASSERT_OK(fd); fd = safe_close(fd); - assert_se(!b); + ASSERT_FALSE(b); - fd = openat_report_new(AT_FDCWD, j, O_RDWR, 0666, &b); - assert_se(fd >= 0); + fd = openat_report_new(tfd, "test", O_RDWR, 0666, &b); + ASSERT_OK(fd); fd = safe_close(fd); - assert_se(!b); + ASSERT_FALSE(b); - fd = openat_report_new(AT_FDCWD, j, O_RDWR|O_CREAT|O_EXCL, 0666, &b); - assert_se(fd == -EEXIST); + fd = openat_report_new(tfd, "test", O_RDWR|O_CREAT|O_EXCL, 0666, &b); + ASSERT_ERROR(fd, EEXIST); - assert_se(unlink(j) >= 0); + ASSERT_OK_ERRNO(unlinkat(tfd, "test", 0)); - fd = openat_report_new(AT_FDCWD, j, O_RDWR, 0666, &b); - assert_se(fd == -ENOENT); + fd = openat_report_new(tfd, "test", O_RDWR, 0666, &b); + ASSERT_ERROR(fd, ENOENT); - fd = openat_report_new(AT_FDCWD, j, O_RDWR|O_CREAT|O_EXCL, 0666, &b); - assert_se(fd >= 0); + fd = openat_report_new(tfd, "test", O_RDWR|O_CREAT|O_EXCL, 0666, &b); + ASSERT_OK(fd); fd = safe_close(fd); - assert_se(b); + ASSERT_TRUE(b); } TEST(xopenat_full) { From 0dd82dab91eaac5e7b17bd5e9a1e07c6d2b78dca Mon Sep 17 00:00:00 2001 From: Daan De Meyer Date: Wed, 24 Jul 2024 10:41:24 +0200 Subject: [PATCH 06/13] fs-util: Handle dangling symlinks in openat_report_new() openat() will always resolve symlinks, except if O_NOFOLLOW is passed or O_CREAT|O_EXCL is passed. This means that if a dangling symlink is passed to openat_report_new(), the first call to openat() will always fail with ENOENT and the second call to openat() will always fail with EEXIST. Let's catch this case explicitly and fallback to creating the file with just O_CREAT and assume we're the ones that created the file. We can't resolve the symlink with chase() because this function is itself called by chase() so we could end up in weird recursive calls if we'd try to do so. --- src/basic/fs-util.c | 23 +++++++++++++++++++++-- src/test/test-fs-util.c | 11 +++++++++++ 2 files changed, 32 insertions(+), 2 deletions(-) diff --git a/src/basic/fs-util.c b/src/basic/fs-util.c index 92ceb3b352..c3d2b6e052 100644 --- a/src/basic/fs-util.c +++ b/src/basic/fs-util.c @@ -1114,8 +1114,27 @@ int openat_report_new(int dirfd, const char *pathname, int flags, mode_t mode, b if (errno != EEXIST) return -errno; - /* Hmm, so now we got EEXIST? So it apparently exists now? If so, let's try to open again - * without the two flags. But let's not spin forever, hence put a limit on things */ + /* Hmm, so now we got EEXIST? This can indicate two things. First, if the path points to a + * dangling symlink, the first openat() will fail with ENOENT because the symlink is resolved + * and the second openat() will fail with EEXIST because symlinks are not followed when + * O_CREAT|O_EXCL is specified. Let's check for this explicitly and fall back to opening with + * just O_CREAT and assume we're the ones that created the file. */ + + struct stat st; + if (fstatat(dirfd, pathname, &st, AT_SYMLINK_NOFOLLOW) < 0) + return -errno; + + if (S_ISLNK(st.st_mode)) { + fd = openat(dirfd, pathname, flags | O_CREAT, mode); + if (fd < 0) + return -errno; + + *ret_newly_created = true; + return fd; + } + + /* If we're not operating on a symlink, someone might have created the file between the first + * and second call to openat(). Let's try again but with a limit so we don't spin forever. */ if (--attempts == 0) /* Give up eventually, somebody is playing with us */ return -EEXIST; diff --git a/src/test/test-fs-util.c b/src/test/test-fs-util.c index f2fa51f55f..8139af83ce 100644 --- a/src/test/test-fs-util.c +++ b/src/test/test-fs-util.c @@ -667,6 +667,17 @@ TEST(openat_report_new) { ASSERT_OK(fd); fd = safe_close(fd); ASSERT_TRUE(b); + + ASSERT_OK_ERRNO(symlinkat("target", tfd, "link")); + fd = openat_report_new(tfd, "link", O_RDWR|O_CREAT, 0666, &b); + ASSERT_OK(fd); + fd = safe_close(fd); + ASSERT_TRUE(b); + + fd = openat_report_new(tfd, "link", O_RDWR|O_CREAT, 0666, &b); + ASSERT_OK(fd); + fd = safe_close(fd); + ASSERT_FALSE(b); } TEST(xopenat_full) { From 32dfe3b63e609bc584ce8fd9b0f8380def9da0c1 Mon Sep 17 00:00:00 2001 From: Daan De Meyer Date: Tue, 23 Jul 2024 21:35:08 +0200 Subject: [PATCH 07/13] fs-util: Clean up properly in xopenat_full() on labelling error If we fail to relabel the file, we should unlink the file or directory again, so let's make sure we do that. --- src/basic/fs-util.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/src/basic/fs-util.c b/src/basic/fs-util.c index c3d2b6e052..426cb5d6f6 100644 --- a/src/basic/fs-util.c +++ b/src/basic/fs-util.c @@ -1143,7 +1143,7 @@ int openat_report_new(int dirfd, const char *pathname, int flags, mode_t mode, b int xopenat_full(int dir_fd, const char *path, int open_flags, XOpenFlags xopen_flags, mode_t mode) { _cleanup_close_ int fd = -EBADF; - bool made = false; + bool made_dir = false, made_file = false; int r; assert(dir_fd >= 0 || dir_fd == AT_FDCWD); @@ -1177,12 +1177,10 @@ int xopenat_full(int dir_fd, const char *path, int open_flags, XOpenFlags xopen_ if (r == -EEXIST) { if (FLAGS_SET(open_flags, O_EXCL)) return -EEXIST; - - made = false; } else if (r < 0) return r; else - made = true; + made_dir = true; if (FLAGS_SET(xopen_flags, XO_LABEL)) { r = label_ops_post(dir_fd, path); @@ -1194,7 +1192,7 @@ int xopenat_full(int dir_fd, const char *path, int open_flags, XOpenFlags xopen_ xopen_flags &= ~XO_LABEL; } - fd = RET_NERRNO(openat(dir_fd, path, open_flags, mode)); + fd = RET_NERRNO(openat_report_new(dir_fd, path, open_flags, mode, &made_file)); if (fd < 0) { if (IN_SET(fd, /* We got ENOENT? then someone else immediately removed it after we @@ -1207,7 +1205,7 @@ int xopenat_full(int dir_fd, const char *path, int open_flags, XOpenFlags xopen_ -ENOTDIR)) return fd; - if (made) + if (made_dir) (void) unlinkat(dir_fd, path, AT_REMOVEDIR); return fd; @@ -1216,10 +1214,16 @@ int xopenat_full(int dir_fd, const char *path, int open_flags, XOpenFlags xopen_ if (FLAGS_SET(open_flags, O_CREAT) && FLAGS_SET(xopen_flags, XO_LABEL)) { r = label_ops_post(dir_fd, path); if (r < 0) - return r; + goto error; } return TAKE_FD(fd); + +error: + if (made_dir || made_file) + (void) unlinkat(dir_fd, path, made_dir ? AT_REMOVEDIR : 0); + + return r; } int xopenat_lock_full( From 1b05ac946a6c7922365de7e0033cc82a75b5c361 Mon Sep 17 00:00:00 2001 From: Daan De Meyer Date: Tue, 23 Jul 2024 21:37:11 +0200 Subject: [PATCH 08/13] fs-util: Add XO_NOCOW flag Let's add a flag for xopenat() that immediately makes a file NOCOW after opening it if it's supported. --- src/basic/fs-util.c | 7 +++++++ src/basic/fs-util.h | 1 + 2 files changed, 8 insertions(+) diff --git a/src/basic/fs-util.c b/src/basic/fs-util.c index 426cb5d6f6..a6b53e7067 100644 --- a/src/basic/fs-util.c +++ b/src/basic/fs-util.c @@ -10,6 +10,7 @@ #include "alloc-util.h" #include "btrfs.h" +#include "chattr-util.h" #include "dirent-util.h" #include "fd-util.h" #include "fileio.h" @@ -1217,6 +1218,12 @@ int xopenat_full(int dir_fd, const char *path, int open_flags, XOpenFlags xopen_ goto error; } + if (FLAGS_SET(xopen_flags, XO_NOCOW)) { + r = chattr_fd(fd, FS_NOCOW_FL, FS_NOCOW_FL, NULL); + if (r < 0 && !ERRNO_IS_NOT_SUPPORTED(r)) + goto error; + } + return TAKE_FD(fd); error: diff --git a/src/basic/fs-util.h b/src/basic/fs-util.h index b050438cd7..2ec4b196f8 100644 --- a/src/basic/fs-util.h +++ b/src/basic/fs-util.h @@ -131,6 +131,7 @@ int parse_cifs_service(const char *s, char **ret_host, char **ret_service, char typedef enum XOpenFlags { XO_LABEL = 1 << 0, XO_SUBVOLUME = 1 << 1, + XO_NOCOW = 1 << 2, } XOpenFlags; int open_mkdir_at_full(int dirfd, const char *path, int flags, XOpenFlags xopen_flags, mode_t mode); From b9c0b6c011fd0b30d3484d21d70cef6f5ae2fc0a Mon Sep 17 00:00:00 2001 From: Daan De Meyer Date: Tue, 23 Jul 2024 21:43:13 +0200 Subject: [PATCH 09/13] repart: Make partition files NOCOW if the disk image is NOCOW On btrfs, reflinks into a disk image that has copy-on-write disabled only work if the source has copy-on-write disabled as well so let's make sure that's the case if the disk image has copy-on-write disabled. --- src/partition/repart.c | 71 +++++++++++++++++++++++++++++++----------- 1 file changed, 53 insertions(+), 18 deletions(-) diff --git a/src/partition/repart.c b/src/partition/repart.c index 5ba099940a..118a369206 100644 --- a/src/partition/repart.c +++ b/src/partition/repart.c @@ -23,6 +23,7 @@ #include "btrfs-util.h" #include "build.h" #include "chase.h" +#include "chattr-util.h" #include "conf-files.h" #include "conf-parser.h" #include "constants.h" @@ -3792,12 +3793,14 @@ static PartitionTarget* partition_target_free(PartitionTarget *t) { DEFINE_TRIVIAL_CLEANUP_FUNC(PartitionTarget*, partition_target_free); -static int prepare_temporary_file(PartitionTarget *t, uint64_t size) { +static int prepare_temporary_file(Context *context, PartitionTarget *t, uint64_t size) { _cleanup_(unlink_and_freep) char *temp = NULL; _cleanup_close_ int fd = -EBADF; const char *vt; + unsigned attrs = 0; int r; + assert(context); assert(t); r = var_tmp_dir(&vt); @@ -3812,6 +3815,16 @@ static int prepare_temporary_file(PartitionTarget *t, uint64_t size) { if (fd < 0) return log_error_errno(fd, "Failed to create temporary file: %m"); + r = read_attr_fd(fdisk_get_devfd(context->fdisk_context), &attrs); + if (r < 0 && !ERRNO_IS_NEG_NOT_SUPPORTED(r)) + return log_error_errno(r, "Failed to read file attributes of %s: %m", arg_node); + + if (FLAGS_SET(attrs, FS_NOCOW_FL)) { + r = chattr_fd(fd, FS_NOCOW_FL, FS_NOCOW_FL, NULL); + if (r < 0 && !ERRNO_IS_NOT_SUPPORTED(r)) + return log_error_errno(r, "Failed to disable copy-on-write on %s: %m", temp); + } + if (ftruncate(fd, size) < 0) return log_error_errno(errno, "Failed to truncate temporary file to %s: %m", FORMAT_BYTES(size)); @@ -3882,7 +3895,7 @@ static int partition_target_prepare( * reflinking support, we can take advantage of this and just reflink the result into the image. */ - r = prepare_temporary_file(t, size); + r = prepare_temporary_file(context, t, size); if (r < 0) return r; @@ -5831,6 +5844,7 @@ static int split_name_resolve(Context *context) { } static int context_split(Context *context) { + unsigned attrs = 0; int fd = -EBADF, r; if (!arg_split) @@ -5857,13 +5871,23 @@ static int context_split(Context *context) { if (partition_type_defer(&p->type)) continue; - fdt = open(p->split_path, O_WRONLY|O_NOCTTY|O_CLOEXEC|O_NOFOLLOW|O_CREAT|O_EXCL, 0666); + if (fd < 0) { + assert_se((fd = fdisk_get_devfd(context->fdisk_context)) >= 0); + + r = read_attr_fd(fd, &attrs); + if (r < 0 && !ERRNO_IS_NEG_NOT_SUPPORTED(r)) + return log_error_errno(r, "Failed to read file attributes of %s: %m", arg_node); + } + + fdt = xopenat_full( + AT_FDCWD, + p->split_path, + O_WRONLY|O_NOCTTY|O_CLOEXEC|O_NOFOLLOW|O_CREAT|O_EXCL, + attrs & FS_NOCOW_FL ? XO_NOCOW : 0, + 0666); if (fdt < 0) return log_error_errno(fdt, "Failed to open split partition file %s: %m", p->split_path); - if (fd < 0) - assert_se((fd = fdisk_get_devfd(context->fdisk_context)) >= 0); - if (lseek(fd, p->offset, SEEK_SET) < 0) return log_error_errno(errno, "Failed to seek to partition offset: %m"); @@ -6661,10 +6685,15 @@ static int context_crypttab(Context *context) { static int context_minimize(Context *context) { const char *vt = NULL; + unsigned attrs = 0; int r; assert(context); + r = read_attr_fd(context->backing_fd, &attrs); + if (r < 0 && !ERRNO_IS_NEG_NOT_SUPPORTED(r)) + return log_error_errno(r, "Failed to read file attributes of %s: %m", arg_node); + LIST_FOREACH(partitions, p, context->partitions) { _cleanup_(rm_rf_physical_and_freep) char *root = NULL; _cleanup_(unlink_and_freep) char *temp = NULL; @@ -6711,13 +6740,18 @@ static int context_minimize(Context *context) { if (r < 0) return log_error_errno(r, "Failed to generate temporary file path: %m"); + fd = xopenat_full( + AT_FDCWD, + temp, + O_CREAT|O_EXCL|O_CLOEXEC|O_RDWR|O_NOCTTY, + attrs & FS_NOCOW_FL ? XO_NOCOW : 0, + 0600); + if (fd < 0) + return log_error_errno(errno, "Failed to open temporary file %s: %m", temp); + if (fstype_is_ro(p->format)) fs_uuid = p->fs_uuid; else { - fd = open(temp, O_CREAT|O_EXCL|O_CLOEXEC|O_RDWR|O_NOCTTY, 0600); - if (fd < 0) - return log_error_errno(errno, "Failed to open temporary file %s: %m", temp); - /* This may seem huge but it will be created sparse so it doesn't take up any space * on disk until written to. */ if (ftruncate(fd, 1024ULL * 1024ULL * 1024ULL * 1024ULL) < 0) @@ -6768,7 +6802,7 @@ static int context_minimize(Context *context) { /* Read-only filesystems are minimal from the first try because they create and size the * loopback file for us. */ if (fstype_is_ro(p->format)) { - assert(fd < 0); + fd = safe_close(fd); fd = open(temp, O_RDONLY|O_CLOEXEC|O_NONBLOCK); if (fd < 0) @@ -6904,18 +6938,19 @@ static int context_minimize(Context *context) { if (r < 0) return log_error_errno(r, "Failed to generate temporary file path: %m"); - r = touch(temp); - if (r < 0) - return log_error_errno(r, "Failed to create temporary file: %m"); + fd = xopenat_full( + AT_FDCWD, + temp, + O_RDONLY|O_CLOEXEC|O_CREAT|O_NONBLOCK, + attrs & FS_NOCOW_FL ? XO_NOCOW : 0, + 0600); + if (fd < 0) + return log_error_errno(errno, "Failed to open temporary file %s: %m", temp); r = partition_format_verity_hash(context, p, temp, dp->copy_blocks_path); if (r < 0) return r; - fd = open(temp, O_RDONLY|O_CLOEXEC|O_NONBLOCK); - if (fd < 0) - return log_error_errno(errno, "Failed to open temporary file %s: %m", temp); - if (fstat(fd, &st) < 0) return log_error_errno(errno, "Failed to stat temporary file: %m"); From 4dee4e00392e64f6c4f84c1e8fee335b7dbf590c Mon Sep 17 00:00:00 2001 From: Daan De Meyer Date: Tue, 23 Jul 2024 22:44:03 +0200 Subject: [PATCH 10/13] repart: Create disk image file with copy-on-write disabled on btrfs COW on btrfs generally does not play well lots of random writes so let's make the disk images generated by repart NOCOW by default on btrfs like we do elsewhere across the codebase. --- src/partition/repart.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/partition/repart.c b/src/partition/repart.c index 118a369206..7db493cafc 100644 --- a/src/partition/repart.c +++ b/src/partition/repart.c @@ -7909,7 +7909,7 @@ static int find_root(Context *context) { if (!s) return log_oom(); - fd = open(arg_node, O_RDONLY|O_CREAT|O_EXCL|O_CLOEXEC|O_NOFOLLOW, 0666); + fd = xopenat_full(AT_FDCWD, arg_node, O_RDONLY|O_CREAT|O_EXCL|O_CLOEXEC|O_NOFOLLOW, XO_NOCOW, 0666); if (fd < 0) return log_error_errno(errno, "Failed to create '%s': %m", arg_node); From 5e496845212d6f1f49e6f6a749b0819301323b27 Mon Sep 17 00:00:00 2001 From: Daan De Meyer Date: Wed, 24 Jul 2024 14:54:06 +0200 Subject: [PATCH 11/13] Make read_attr_path() more generic Let's make this an openat() like function so it can be used in more scenarios. --- src/basic/chattr-util.c | 8 ++++---- src/basic/chattr-util.h | 2 +- src/shared/discover-image.c | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/basic/chattr-util.c b/src/basic/chattr-util.c index d76be5c99b..49831a26ce 100644 --- a/src/basic/chattr-util.c +++ b/src/basic/chattr-util.c @@ -148,15 +148,15 @@ int read_attr_fd(int fd, unsigned *ret) { return RET_NERRNO(ioctl(fd, FS_IOC_GETFLAGS, ret)); } -int read_attr_path(const char *p, unsigned *ret) { +int read_attr_at(int dir_fd, const char *path, unsigned *ret) { _cleanup_close_ int fd = -EBADF; - assert(p); + assert(dir_fd >= 0 || dir_fd == AT_FDCWD); assert(ret); - fd = open(p, O_RDONLY|O_CLOEXEC|O_NOCTTY|O_NOFOLLOW); + fd = xopenat(dir_fd, path, O_RDONLY|O_CLOEXEC|O_NOCTTY|O_NOFOLLOW); if (fd < 0) - return -errno; + return fd; return read_attr_fd(fd, ret); } diff --git a/src/basic/chattr-util.h b/src/basic/chattr-util.h index c1ee63b4fa..1fe38e32b1 100644 --- a/src/basic/chattr-util.h +++ b/src/basic/chattr-util.h @@ -52,7 +52,7 @@ static inline int chattr_path(const char *path, unsigned value, unsigned mask, u } int read_attr_fd(int fd, unsigned *ret); -int read_attr_path(const char *p, unsigned *ret); +int read_attr_at(int dir_fd, const char *path, unsigned *ret); /* Combination of chattr flags, that should be appropriate for secrets stored on disk: Secure Remove + * Exclusion from Dumping + Synchronous Writing (i.e. not caching in memory) + In-Place Updating (i.e. not diff --git a/src/shared/discover-image.c b/src/shared/discover-image.c index d7a9df70b1..dd78a013aa 100644 --- a/src/shared/discover-image.c +++ b/src/shared/discover-image.c @@ -1070,7 +1070,7 @@ int image_rename(Image *i, const char *new_name) { case IMAGE_DIRECTORY: /* Turn of the immutable bit while we rename the image, so that we can rename it */ - (void) read_attr_path(i->path, &file_attr); + (void) read_attr_at(AT_FDCWD, i->path, &file_attr); if (file_attr & FS_IMMUTABLE_FL) (void) chattr_path(i->path, 0, FS_IMMUTABLE_FL, NULL); From 2356104efcc599439c887b087da83fbeca1fa9a6 Mon Sep 17 00:00:00 2001 From: Daan De Meyer Date: Wed, 24 Jul 2024 14:56:11 +0200 Subject: [PATCH 12/13] copy: Copy file attributes as well Let's make sure we copy all file attributes that can be copied as well. --- src/shared/copy.c | 41 +++++++++++++++++++++++++++++++++-------- 1 file changed, 33 insertions(+), 8 deletions(-) diff --git a/src/shared/copy.c b/src/shared/copy.c index ac1a9b5933..c5c5cf083d 100644 --- a/src/shared/copy.c +++ b/src/shared/copy.c @@ -798,6 +798,7 @@ static int fd_copy_regular( void *userdata) { _cleanup_close_ int fdf = -EBADF, fdt = -EBADF; + unsigned attrs = 0; int r, q; assert(st); @@ -813,6 +814,10 @@ static int fd_copy_regular( if (fdf < 0) return fdf; + r = read_attr_fd(fdf, &attrs); + if (r < 0 && !ERRNO_IS_NOT_SUPPORTED(r) && r != -ELOOP) + return r; + if (copy_flags & COPY_MAC_CREATE) { r = mac_selinux_create_file_prepare_at(dt, to, S_IFREG); if (r < 0) @@ -824,6 +829,9 @@ static int fd_copy_regular( if (fdt < 0) return -errno; + if (attrs != 0) + (void) chattr_full(fdt, NULL, attrs, CHATTR_EARLY_FL, NULL, NULL, CHATTR_FALLBACK_BITWISE); + r = copy_bytes_full(fdf, fdt, UINT64_MAX, copy_flags, NULL, NULL, progress, userdata); if (r < 0) goto fail; @@ -839,6 +847,9 @@ static int fd_copy_regular( (void) futimens(fdt, (struct timespec[]) { st->st_atim, st->st_mtim }); (void) copy_xattr(fdf, NULL, fdt, NULL, copy_flags); + if (attrs != 0) + (void) chattr_full(fdt, NULL, attrs, ~CHATTR_EARLY_FL, NULL, NULL, CHATTR_FALLBACK_BITWISE); + if (FLAGS_SET(copy_flags, COPY_VERIFY_LINKED)) { r = fd_verify_linked(fdf); if (r < 0) @@ -1413,6 +1424,7 @@ int copy_file_at_full( _cleanup_close_ int fdf = -EBADF, fdt = -EBADF; struct stat st; + unsigned attrs = 0; int r; assert(dir_fdf >= 0 || dir_fdf == AT_FDCWD); @@ -1430,6 +1442,10 @@ int copy_file_at_full( if (r < 0) return r; + r = read_attr_at(dir_fdf, from, &attrs); + if (r < 0 && !ERRNO_IS_NOT_SUPPORTED(r) && r != -ELOOP) + return r; + WITH_UMASK(0000) { fdt = xopenat_lock_full(dir_fdt, to, flags|O_WRONLY|O_CREAT|O_CLOEXEC|O_NOCTTY, @@ -1446,8 +1462,10 @@ int copy_file_at_full( goto fail; } - if (chattr_mask != 0) - (void) chattr_fd(fdt, chattr_flags, chattr_mask & CHATTR_EARLY_FL, NULL); + attrs = (attrs & ~chattr_mask) | (chattr_flags & chattr_mask); + + if (attrs != 0) + (void) chattr_full(fdt, NULL, attrs, CHATTR_EARLY_FL, NULL, NULL, CHATTR_FALLBACK_BITWISE); r = copy_bytes_full(fdf, fdt, UINT64_MAX, copy_flags & ~COPY_LOCK_BSD, NULL, NULL, progress_bytes, userdata); if (r < 0) @@ -1462,8 +1480,8 @@ int copy_file_at_full( goto fail; } - if (chattr_mask != 0) - (void) chattr_fd(fdt, chattr_flags, chattr_mask & ~CHATTR_EARLY_FL, NULL); + if (attrs != 0) + (void) chattr_full(fdt, NULL, attrs, ~CHATTR_EARLY_FL, NULL, NULL, CHATTR_FALLBACK_BITWISE); if (copy_flags & (COPY_FSYNC|COPY_FSYNC_FULL)) { if (fsync(fdt) < 0) { @@ -1508,6 +1526,7 @@ int copy_file_atomic_at_full( _cleanup_(unlink_and_freep) char *t = NULL; _cleanup_close_ int fdt = -EBADF; + unsigned attrs = 0; int r; assert(to); @@ -1524,8 +1543,14 @@ int copy_file_atomic_at_full( if (fdt < 0) return fdt; - if (chattr_mask != 0) - (void) chattr_fd(fdt, chattr_flags, chattr_mask & CHATTR_EARLY_FL, NULL); + r = read_attr_at(dir_fdf, from, &attrs); + if (r < 0 && !ERRNO_IS_NOT_SUPPORTED(r) && r != -ELOOP) + return r; + + attrs = (attrs & ~chattr_mask) | (chattr_flags & chattr_mask); + + if (attrs != 0) + (void) chattr_full(fdt, NULL, attrs, CHATTR_EARLY_FL, NULL, NULL, CHATTR_FALLBACK_BITWISE); r = copy_file_fd_at_full(dir_fdf, from, fdt, copy_flags, progress_bytes, userdata); if (r < 0) @@ -1546,8 +1571,8 @@ int copy_file_atomic_at_full( t = mfree(t); - if (chattr_mask != 0) - (void) chattr_fd(fdt, chattr_flags, chattr_mask & ~CHATTR_EARLY_FL, NULL); + if (attrs != 0) + (void) chattr_full(fdt, NULL, attrs, ~CHATTR_EARLY_FL, NULL, NULL, CHATTR_FALLBACK_BITWISE); r = close_nointr(TAKE_FD(fdt)); /* even if this fails, the fd is now invalidated */ if (r < 0) From 42e9288180ce37abd86c58774950636ebce35432 Mon Sep 17 00:00:00 2001 From: Daan De Meyer Date: Wed, 24 Jul 2024 15:04:43 +0200 Subject: [PATCH 13/13] tree-wide: Don't explicity disable copy-on-write when copying images Since the copy helpers now copy file attributes as well, let's not explicitly disable copy-on-write anymore when we copy an image. If the source already has copy-on-write disabled, the copy will have it disabled as well. Otherwise, the copy will also have copy-on-write enabled. This makes sure that reflinks always work as reflink is only supported if both source and target are copy-on-write or both source and target are not copy-on-write. --- src/core/exec-invoke.c | 10 ---------- src/nspawn/nspawn.c | 6 ++---- src/shared/discover-image.c | 3 +-- 3 files changed, 3 insertions(+), 16 deletions(-) diff --git a/src/core/exec-invoke.c b/src/core/exec-invoke.c index 8c7e29f17a..c7082f9e2e 100644 --- a/src/core/exec-invoke.c +++ b/src/core/exec-invoke.c @@ -2893,16 +2893,6 @@ static int setup_ephemeral( if (fd < 0) return log_debug_errno(fd, "Failed to copy image %s to %s: %m", *root_image, new_root); - - /* A root image might be subject to lots of random writes so let's try to disable COW on it - * which tends to not perform well in combination with lots of random writes. - * - * Note: btrfs actually isn't impressed by us setting the flag after making the reflink'ed - * copy, but we at least want to make the intention clear. - */ - r = chattr_fd(fd, FS_NOCOW_FL, FS_NOCOW_FL, NULL); - if (r < 0) - log_debug_errno(r, "Failed to disable copy-on-write for %s, ignoring: %m", new_root); } else { assert(*root_directory); diff --git a/src/nspawn/nspawn.c b/src/nspawn/nspawn.c index 01c3a432a5..5c7b156858 100644 --- a/src/nspawn/nspawn.c +++ b/src/nspawn/nspawn.c @@ -6072,10 +6072,8 @@ static int run(int argc, char *argv[]) { { BLOCK_SIGNALS(SIGINT); - r = copy_file_full(arg_image, np, O_EXCL, arg_read_only ? 0400 : 0600, - FS_NOCOW_FL, FS_NOCOW_FL, - COPY_REFLINK|COPY_CRTIME|COPY_SIGINT, - NULL, NULL); + r = copy_file(arg_image, np, O_EXCL, arg_read_only ? 0400 : 0600, + COPY_REFLINK|COPY_CRTIME|COPY_SIGINT); } if (r == -EINTR) { log_error_errno(r, "Interrupted while copying image file to %s, removed again.", np); diff --git a/src/shared/discover-image.c b/src/shared/discover-image.c index dd78a013aa..1386452203 100644 --- a/src/shared/discover-image.c +++ b/src/shared/discover-image.c @@ -1205,8 +1205,7 @@ int image_clone(Image *i, const char *new_name, bool read_only) { case IMAGE_RAW: new_path = strjoina("/var/lib/machines/", new_name, ".raw"); - r = copy_file_atomic_full(i->path, new_path, read_only ? 0444 : 0644, FS_NOCOW_FL, FS_NOCOW_FL, - COPY_REFLINK|COPY_CRTIME, NULL, NULL); + r = copy_file_atomic(i->path, new_path, read_only ? 0444 : 0644, COPY_REFLINK|COPY_CRTIME); break; case IMAGE_BLOCK: