From: Vito Caputo Date: Tue, 7 Dec 2021 22:18:14 +0000 (-0800) Subject: journal-file: require MMapCache* for journal_file_open() X-Git-Tag: v250-rc1~27^2 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=refs%2Fpull%2F21527%2Fhead;p=thirdparty%2Fsystemd.git journal-file: require MMapCache* for journal_file_open() Previously the MMapCache* was optionally NULL, which open would handle by creating a new MMapCache* for the occasion. This produced some slightly circuitous refcount-handling code in the function, as well as arguably creating opportunities for weirdness where an MMapCache* was intended to be supplied but happened to be NULL, which this magic would then paper over. In any case, this was basically only being utilized by tests, apparently just to avoid having to create an MMapCache. So update the relevant tests to supply an MMapCache and make journal_file_open() treat a NULL MMapCache* as fatal w/assert. --- diff --git a/src/journal/test-journal-flush.c b/src/journal/test-journal-flush.c index f0a7024002a..dc05126b572 100644 --- a/src/journal/test-journal-flush.c +++ b/src/journal/test-journal-flush.c @@ -14,6 +14,7 @@ #include "string-util.h" int main(int argc, char *argv[]) { + _cleanup_(mmap_cache_unrefp) MMapCache *m = NULL; _cleanup_free_ char *fn = NULL; char dn[] = "/var/tmp/test-journal-flush.XXXXXX"; JournaldFile *new_journal = NULL; @@ -21,12 +22,14 @@ int main(int argc, char *argv[]) { unsigned n = 0; int r; + m = mmap_cache_new(); + assert_se(m != NULL); assert_se(mkdtemp(dn)); (void) chattr_path(dn, FS_NOCOW_FL, FS_NOCOW_FL, NULL); fn = path_join(dn, "test.journal"); - r = journald_file_open(-1, fn, O_CREAT|O_RDWR, 0644, false, 0, false, NULL, NULL, NULL, NULL, &new_journal); + r = journald_file_open(-1, fn, O_CREAT|O_RDWR, 0644, false, 0, false, NULL, m, NULL, NULL, &new_journal); assert_se(r >= 0); if (argc > 1) diff --git a/src/journal/test-journal-interleaving.c b/src/journal/test-journal-interleaving.c index 48be6a14bce..c543b87b695 100644 --- a/src/journal/test-journal-interleaving.c +++ b/src/journal/test-journal-interleaving.c @@ -34,8 +34,13 @@ _noreturn_ static void log_assert_errno(const char *text, int error, const char } while (false) static JournaldFile *test_open(const char *name) { + _cleanup_(mmap_cache_unrefp) MMapCache *m = NULL; JournaldFile *f; - assert_ret(journald_file_open(-1, name, O_RDWR|O_CREAT, 0644, true, UINT64_MAX, false, NULL, NULL, NULL, NULL, &f)); + + m = mmap_cache_new(); + assert_se(m != NULL); + + assert_ret(journald_file_open(-1, name, O_RDWR|O_CREAT, 0644, true, UINT64_MAX, false, NULL, m, NULL, NULL, &f)); return f; } @@ -198,15 +203,19 @@ static void test_skip(void (*setup)(void)) { static void test_sequence_numbers(void) { + _cleanup_(mmap_cache_unrefp) MMapCache *m = NULL; char t[] = "/var/tmp/journal-seq-XXXXXX"; JournaldFile *one, *two; uint64_t seqnum = 0; sd_id128_t seqnum_id; + m = mmap_cache_new(); + assert_se(m != NULL); + mkdtemp_chdir_chattr(t); assert_se(journald_file_open(-1, "one.journal", O_RDWR|O_CREAT, 0644, - true, UINT64_MAX, false, NULL, NULL, NULL, NULL, &one) == 0); + true, UINT64_MAX, false, NULL, m, NULL, NULL, &one) == 0); append_number(one, 1, &seqnum); printf("seqnum=%"PRIu64"\n", seqnum); @@ -223,7 +232,7 @@ static void test_sequence_numbers(void) { memcpy(&seqnum_id, &one->file->header->seqnum_id, sizeof(sd_id128_t)); assert_se(journald_file_open(-1, "two.journal", O_RDWR|O_CREAT, 0644, - true, UINT64_MAX, false, NULL, NULL, NULL, one, &two) == 0); + true, UINT64_MAX, false, NULL, m, NULL, one, &two) == 0); assert_se(two->file->header->state == STATE_ONLINE); assert_se(!sd_id128_equal(two->file->header->file_id, one->file->header->file_id)); @@ -254,7 +263,7 @@ static void test_sequence_numbers(void) { seqnum = 0; assert_se(journald_file_open(-1, "two.journal", O_RDWR, 0, - true, UINT64_MAX, false, NULL, NULL, NULL, NULL, &two) == 0); + true, UINT64_MAX, false, NULL, m, NULL, NULL, &two) == 0); assert_se(sd_id128_equal(two->file->header->seqnum_id, seqnum_id)); diff --git a/src/journal/test-journal-stream.c b/src/journal/test-journal-stream.c index f7fc4332a9e..115ce807c98 100644 --- a/src/journal/test-journal-stream.c +++ b/src/journal/test-journal-stream.c @@ -60,6 +60,7 @@ static void verify_contents(sd_journal *j, unsigned skip) { } static void run_test(void) { + _cleanup_(mmap_cache_unrefp) MMapCache *m = NULL; JournaldFile *one, *two, *three; char t[] = "/var/tmp/journal-stream-XXXXXX"; unsigned i; @@ -69,13 +70,16 @@ static void run_test(void) { size_t l; dual_timestamp previous_ts = DUAL_TIMESTAMP_NULL; + m = mmap_cache_new(); + assert_se(m != NULL); + assert_se(mkdtemp(t)); assert_se(chdir(t) >= 0); (void) chattr_path(t, FS_NOCOW_FL, FS_NOCOW_FL, NULL); - assert_se(journald_file_open(-1, "one.journal", O_RDWR|O_CREAT, 0666, true, UINT64_MAX, false, NULL, NULL, NULL, NULL, &one) == 0); - assert_se(journald_file_open(-1, "two.journal", O_RDWR|O_CREAT, 0666, true, UINT64_MAX, false, NULL, NULL, NULL, NULL, &two) == 0); - assert_se(journald_file_open(-1, "three.journal", O_RDWR|O_CREAT, 0666, true, UINT64_MAX, false, NULL, NULL, NULL, NULL, &three) == 0); + assert_se(journald_file_open(-1, "one.journal", O_RDWR|O_CREAT, 0666, true, UINT64_MAX, false, NULL, m, NULL, NULL, &one) == 0); + assert_se(journald_file_open(-1, "two.journal", O_RDWR|O_CREAT, 0666, true, UINT64_MAX, false, NULL, m, NULL, NULL, &two) == 0); + assert_se(journald_file_open(-1, "three.journal", O_RDWR|O_CREAT, 0666, true, UINT64_MAX, false, NULL, m, NULL, NULL, &three) == 0); for (i = 0; i < N_ENTRIES; i++) { char *p, *q; diff --git a/src/journal/test-journal-verify.c b/src/journal/test-journal-verify.c index cf9692b3d25..6abfaeb0df9 100644 --- a/src/journal/test-journal-verify.c +++ b/src/journal/test-journal-verify.c @@ -10,6 +10,7 @@ #include "journald-file.h" #include "journal-verify.h" #include "log.h" +#include "mmap-cache.h" #include "rm-rf.h" #include "terminal-util.h" #include "tests.h" @@ -38,10 +39,14 @@ static void bit_toggle(const char *fn, uint64_t p) { } static int raw_verify(const char *fn, const char *verification_key) { + _cleanup_(mmap_cache_unrefp) MMapCache *m = NULL; JournalFile *f; int r; - r = journal_file_open(-1, fn, O_RDONLY, 0666, true, UINT64_MAX, !!verification_key, NULL, NULL, NULL, &f); + m = mmap_cache_new(); + assert_se(m != NULL); + + r = journal_file_open(-1, fn, O_RDONLY, 0666, true, UINT64_MAX, !!verification_key, NULL, m, NULL, &f); if (r < 0) return r; @@ -52,6 +57,7 @@ static int raw_verify(const char *fn, const char *verification_key) { } int main(int argc, char *argv[]) { + _cleanup_(mmap_cache_unrefp) MMapCache *m = NULL; char t[] = "/var/tmp/journal-XXXXXX"; unsigned n; JournalFile *f; @@ -61,6 +67,9 @@ int main(int argc, char *argv[]) { struct stat st; uint64_t p; + m = mmap_cache_new(); + assert_se(m != NULL); + /* journald_file_open requires a valid machine id */ if (access("/etc/machine-id", F_OK) != 0) return log_tests_skipped("/etc/machine-id not found"); @@ -73,7 +82,7 @@ int main(int argc, char *argv[]) { log_info("Generating..."); - assert_se(journald_file_open(-1, "test.journal", O_RDWR|O_CREAT, 0666, true, UINT64_MAX, !!verification_key, NULL, NULL, NULL, NULL, &df) == 0); + assert_se(journald_file_open(-1, "test.journal", O_RDWR|O_CREAT, 0666, true, UINT64_MAX, !!verification_key, NULL, m, NULL, NULL, &df) == 0); for (n = 0; n < N_ENTRIES; n++) { struct iovec iovec; @@ -95,7 +104,7 @@ int main(int argc, char *argv[]) { log_info("Verifying..."); - assert_se(journal_file_open(-1, "test.journal", O_RDONLY, 0666, true, UINT64_MAX, !!verification_key, NULL, NULL, NULL, &f) == 0); + assert_se(journal_file_open(-1, "test.journal", O_RDONLY, 0666, true, UINT64_MAX, !!verification_key, NULL, m, NULL, &f) == 0); /* journal_file_print_header(f); */ journal_file_dump(f); diff --git a/src/journal/test-journal.c b/src/journal/test-journal.c index 17c6a73db30..11647504e91 100644 --- a/src/journal/test-journal.c +++ b/src/journal/test-journal.c @@ -24,6 +24,7 @@ static void mkdtemp_chdir_chattr(char *path) { } static void test_non_empty(void) { + _cleanup_(mmap_cache_unrefp) MMapCache *m = NULL; dual_timestamp ts; JournaldFile *f; struct iovec iovec; @@ -35,9 +36,12 @@ static void test_non_empty(void) { test_setup_logging(LOG_DEBUG); + m = mmap_cache_new(); + assert_se(m != NULL); + mkdtemp_chdir_chattr(t); - assert_se(journald_file_open(-1, "test.journal", O_RDWR|O_CREAT, 0666, true, UINT64_MAX, true, NULL, NULL, NULL, NULL, &f) == 0); + assert_se(journald_file_open(-1, "test.journal", O_RDWR|O_CREAT, 0666, true, UINT64_MAX, true, NULL, m, NULL, NULL, &f) == 0); assert_se(dual_timestamp_get(&ts)); assert_se(sd_id128_randomize(&fake_boot_id) == 0); @@ -98,8 +102,8 @@ static void test_non_empty(void) { assert_se(journal_file_move_to_entry_by_seqnum(f->file, 10, DIRECTION_DOWN, &o, NULL) == 0); - journald_file_rotate(&f, NULL, true, UINT64_MAX, true, NULL); - journald_file_rotate(&f, NULL, true, UINT64_MAX, true, NULL); + journald_file_rotate(&f, m, true, UINT64_MAX, true, NULL); + journald_file_rotate(&f, m, true, UINT64_MAX, true, NULL); (void) journald_file_close(f); @@ -117,17 +121,21 @@ static void test_non_empty(void) { } static void test_empty(void) { + _cleanup_(mmap_cache_unrefp) MMapCache *m = NULL; JournaldFile *f1, *f2, *f3, *f4; char t[] = "/var/tmp/journal-XXXXXX"; test_setup_logging(LOG_DEBUG); + m = mmap_cache_new(); + assert_se(m != NULL); + mkdtemp_chdir_chattr(t); - assert_se(journald_file_open(-1, "test.journal", O_RDWR|O_CREAT, 0666, false, UINT64_MAX, false, NULL, NULL, NULL, NULL, &f1) == 0); - assert_se(journald_file_open(-1, "test-compress.journal", O_RDWR|O_CREAT, 0666, true, UINT64_MAX, false, NULL, NULL, NULL, NULL, &f2) == 0); - assert_se(journald_file_open(-1, "test-seal.journal", O_RDWR|O_CREAT, 0666, false, UINT64_MAX, true, NULL, NULL, NULL, NULL, &f3) == 0); - assert_se(journald_file_open(-1, "test-seal-compress.journal", O_RDWR|O_CREAT, 0666, true, UINT64_MAX, true, NULL, NULL, NULL, NULL, &f4) == 0); + assert_se(journald_file_open(-1, "test.journal", O_RDWR|O_CREAT, 0666, false, UINT64_MAX, false, NULL, m, NULL, NULL, &f1) == 0); + assert_se(journald_file_open(-1, "test-compress.journal", O_RDWR|O_CREAT, 0666, true, UINT64_MAX, false, NULL, m, NULL, NULL, &f2) == 0); + assert_se(journald_file_open(-1, "test-seal.journal", O_RDWR|O_CREAT, 0666, false, UINT64_MAX, true, NULL, m, NULL, NULL, &f3) == 0); + assert_se(journald_file_open(-1, "test-seal-compress.journal", O_RDWR|O_CREAT, 0666, true, UINT64_MAX, true, NULL, m, NULL, NULL, &f4) == 0); journal_file_print_header(f1->file); puts(""); @@ -156,6 +164,7 @@ static void test_empty(void) { #if HAVE_COMPRESSION static bool check_compressed(uint64_t compress_threshold, uint64_t data_size) { + _cleanup_(mmap_cache_unrefp) MMapCache *m = NULL; dual_timestamp ts; JournaldFile *f; struct iovec iovec; @@ -170,9 +179,12 @@ static bool check_compressed(uint64_t compress_threshold, uint64_t data_size) { test_setup_logging(LOG_DEBUG); + m = mmap_cache_new(); + assert_se(m != NULL); + mkdtemp_chdir_chattr(t); - assert_se(journald_file_open(-1, "test.journal", O_RDWR|O_CREAT, 0666, true, compress_threshold, true, NULL, NULL, NULL, NULL, &f) == 0); + assert_se(journald_file_open(-1, "test.journal", O_RDWR|O_CREAT, 0666, true, compress_threshold, true, NULL, m, NULL, NULL, &f) == 0); dual_timestamp_get(&ts); diff --git a/src/libsystemd/sd-journal/journal-file.c b/src/libsystemd/sd-journal/journal-file.c index 4302a99f80d..505e4f728df 100644 --- a/src/libsystemd/sd-journal/journal-file.c +++ b/src/libsystemd/sd-journal/journal-file.c @@ -3250,13 +3250,13 @@ int journal_file_open( JournalFile **ret) { bool newly_created = false; - MMapCache *m = mmap_cache; JournalFile *f; void *h; int r; assert(ret); assert(fd >= 0 || fname); + assert(mmap_cache); if (!IN_SET((flags & O_ACCMODE), O_RDONLY, O_RDWR)) return -EINVAL; @@ -3319,14 +3319,6 @@ int journal_file_open( } } - if (!m) { - m = mmap_cache_new(); - if (!m) { - r = -ENOMEM; - goto fail; - } - } - if (fname) { f->path = strdup(fname); if (!f->path) { @@ -3368,17 +3360,12 @@ int journal_file_open( goto fail; } - /* On success this incs refcnt on *m, which mmap_cache_fd_free() will dec. */ - f->cache_fd = mmap_cache_add_fd(m, f->fd, prot_from_flags(flags)); + f->cache_fd = mmap_cache_add_fd(mmap_cache, f->fd, prot_from_flags(flags)); if (!f->cache_fd) { r = -ENOMEM; goto fail; } - /* If we created *m just for this file, unref *m so only f->cache_fd's ref remains */ - if (!mmap_cache) - (void) mmap_cache_unref(m); - r = journal_file_fstat(f); if (r < 0) goto fail;