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 f0a7024002a0bc10ba89ec1627e73da932e56a10..dc05126b5722969900d2c560de80fd0c1a2579d8 100644 (file)
--- 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 48be6a14bceb751ca2569110973c1b0f35090c78..c543b87b6954833ece973e6bcfb5496bb1a2afea 100644 (file)
--- 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 f7fc4332a9e60b000d9d5e8e7b5a39f93d47ce5c..115ce807c983819018e868cbbd3f55af6deb06c0 100644 (file)
--- 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 cf9692b3d257137dbcf6aaab3090f63ff3c98685..6abfaeb0df975d5d07bb60b5ab8b996795c43c9e 100644 (file)
--- 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 17c6a73db30a1c6f540d91a4162bc3c827f59e5a..11647504e9176ed71d11829168d292154b01fa59 100644 (file)
--- 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 4302a99f80d8b6afe6676765751495c63233c996..505e4f728df005835fac1b1c12ac9c612fe3922d 100644 (file)
--- 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;