Skip to content

Commit 74fb5be

Browse files
vcaputovcaputo
committed
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.
1 parent 333d067 commit 74fb5be

File tree

6 files changed

+58
-34
lines changed

6 files changed

+58
-34
lines changed

‎src/journal/test-journal-flush.c‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,19 +14,22 @@
1414
#include "string-util.h"
1515

1616
int main(int argc, char *argv[]) {
17+
_cleanup_(mmap_cache_unrefp) MMapCache *m = NULL;
1718
_cleanup_free_ char *fn = NULL;
1819
char dn[] = "/var/tmp/test-journal-flush.XXXXXX";
1920
JournaldFile *new_journal = NULL;
2021
sd_journal *j = NULL;
2122
unsigned n = 0;
2223
int r;
2324

25+
m = mmap_cache_new();
26+
assert_se(m != NULL);
2427
assert_se(mkdtemp(dn));
2528
(void) chattr_path(dn, FS_NOCOW_FL, FS_NOCOW_FL, NULL);
2629

2730
fn = path_join(dn, "test.journal");
2831

29-
r = journald_file_open(-1, fn, O_CREAT|O_RDWR, 0644, false, 0, false, NULL, NULL, NULL, NULL, &new_journal);
32+
r = journald_file_open(-1, fn, O_CREAT|O_RDWR, 0644, false, 0, false, NULL, m, NULL, NULL, &new_journal);
3033
assert_se(r >= 0);
3134

3235
if (argc > 1)

‎src/journal/test-journal-interleaving.c‎

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -34,8 +34,13 @@ _noreturn_ static void log_assert_errno(const char *text, int error, const char
3434
} while (false)
3535

3636
static JournaldFile *test_open(const char *name) {
37+
_cleanup_(mmap_cache_unrefp) MMapCache *m = NULL;
3738
JournaldFile *f;
38-
assert_ret(journald_file_open(-1, name, O_RDWR|O_CREAT, 0644, true, UINT64_MAX, false, NULL, NULL, NULL, NULL, &f));
39+
40+
m = mmap_cache_new();
41+
assert_se(m != NULL);
42+
43+
assert_ret(journald_file_open(-1, name, O_RDWR|O_CREAT, 0644, true, UINT64_MAX, false, NULL, m, NULL, NULL, &f));
3944
return f;
4045
}
4146

@@ -198,15 +203,19 @@ static void test_skip(void (*setup)(void)) {
198203

199204
static void test_sequence_numbers(void) {
200205

206+
_cleanup_(mmap_cache_unrefp) MMapCache *m = NULL;
201207
char t[] = "/var/tmp/journal-seq-XXXXXX";
202208
JournaldFile *one, *two;
203209
uint64_t seqnum = 0;
204210
sd_id128_t seqnum_id;
205211

212+
m = mmap_cache_new();
213+
assert_se(m != NULL);
214+
206215
mkdtemp_chdir_chattr(t);
207216

208217
assert_se(journald_file_open(-1, "one.journal", O_RDWR|O_CREAT, 0644,
209-
true, UINT64_MAX, false, NULL, NULL, NULL, NULL, &one) == 0);
218+
true, UINT64_MAX, false, NULL, m, NULL, NULL, &one) == 0);
210219

211220
append_number(one, 1, &seqnum);
212221
printf("seqnum=%"PRIu64"\n", seqnum);
@@ -223,7 +232,7 @@ static void test_sequence_numbers(void) {
223232
memcpy(&seqnum_id, &one->file->header->seqnum_id, sizeof(sd_id128_t));
224233

225234
assert_se(journald_file_open(-1, "two.journal", O_RDWR|O_CREAT, 0644,
226-
true, UINT64_MAX, false, NULL, NULL, NULL, one, &two) == 0);
235+
true, UINT64_MAX, false, NULL, m, NULL, one, &two) == 0);
227236

228237
assert_se(two->file->header->state == STATE_ONLINE);
229238
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) {
254263
seqnum = 0;
255264

256265
assert_se(journald_file_open(-1, "two.journal", O_RDWR, 0,
257-
true, UINT64_MAX, false, NULL, NULL, NULL, NULL, &two) == 0);
266+
true, UINT64_MAX, false, NULL, m, NULL, NULL, &two) == 0);
258267

259268
assert_se(sd_id128_equal(two->file->header->seqnum_id, seqnum_id));
260269

‎src/journal/test-journal-stream.c‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,7 @@ static void verify_contents(sd_journal *j, unsigned skip) {
6060
}
6161

6262
static void run_test(void) {
63+
_cleanup_(mmap_cache_unrefp) MMapCache *m = NULL;
6364
JournaldFile *one, *two, *three;
6465
char t[] = "/var/tmp/journal-stream-XXXXXX";
6566
unsigned i;
@@ -69,13 +70,16 @@ static void run_test(void) {
6970
size_t l;
7071
dual_timestamp previous_ts = DUAL_TIMESTAMP_NULL;
7172

73+
m = mmap_cache_new();
74+
assert_se(m != NULL);
75+
7276
assert_se(mkdtemp(t));
7377
assert_se(chdir(t) >= 0);
7478
(void) chattr_path(t, FS_NOCOW_FL, FS_NOCOW_FL, NULL);
7579

76-
assert_se(journald_file_open(-1, "one.journal", O_RDWR|O_CREAT, 0666, true, UINT64_MAX, false, NULL, NULL, NULL, NULL, &one) == 0);
77-
assert_se(journald_file_open(-1, "two.journal", O_RDWR|O_CREAT, 0666, true, UINT64_MAX, false, NULL, NULL, NULL, NULL, &two) == 0);
78-
assert_se(journald_file_open(-1, "three.journal", O_RDWR|O_CREAT, 0666, true, UINT64_MAX, false, NULL, NULL, NULL, NULL, &three) == 0);
80+
assert_se(journald_file_open(-1, "one.journal", O_RDWR|O_CREAT, 0666, true, UINT64_MAX, false, NULL, m, NULL, NULL, &one) == 0);
81+
assert_se(journald_file_open(-1, "two.journal", O_RDWR|O_CREAT, 0666, true, UINT64_MAX, false, NULL, m, NULL, NULL, &two) == 0);
82+
assert_se(journald_file_open(-1, "three.journal", O_RDWR|O_CREAT, 0666, true, UINT64_MAX, false, NULL, m, NULL, NULL, &three) == 0);
7983

8084
for (i = 0; i < N_ENTRIES; i++) {
8185
char *p, *q;

‎src/journal/test-journal-verify.c‎

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
#include "journald-file.h"
1111
#include "journal-verify.h"
1212
#include "log.h"
13+
#include "mmap-cache.h"
1314
#include "rm-rf.h"
1415
#include "terminal-util.h"
1516
#include "tests.h"
@@ -38,10 +39,14 @@ static void bit_toggle(const char *fn, uint64_t p) {
3839
}
3940

4041
static int raw_verify(const char *fn, const char *verification_key) {
42+
_cleanup_(mmap_cache_unrefp) MMapCache *m = NULL;
4143
JournalFile *f;
4244
int r;
4345

44-
r = journal_file_open(-1, fn, O_RDONLY, 0666, true, UINT64_MAX, !!verification_key, NULL, NULL, NULL, &f);
46+
m = mmap_cache_new();
47+
assert_se(m != NULL);
48+
49+
r = journal_file_open(-1, fn, O_RDONLY, 0666, true, UINT64_MAX, !!verification_key, NULL, m, NULL, &f);
4550
if (r < 0)
4651
return r;
4752

@@ -52,6 +57,7 @@ static int raw_verify(const char *fn, const char *verification_key) {
5257
}
5358

5459
int main(int argc, char *argv[]) {
60+
_cleanup_(mmap_cache_unrefp) MMapCache *m = NULL;
5561
char t[] = "/var/tmp/journal-XXXXXX";
5662
unsigned n;
5763
JournalFile *f;
@@ -61,6 +67,9 @@ int main(int argc, char *argv[]) {
6167
struct stat st;
6268
uint64_t p;
6369

70+
m = mmap_cache_new();
71+
assert_se(m != NULL);
72+
6473
/* journald_file_open requires a valid machine id */
6574
if (access("/etc/machine-id", F_OK) != 0)
6675
return log_tests_skipped("/etc/machine-id not found");
@@ -73,7 +82,7 @@ int main(int argc, char *argv[]) {
7382

7483
log_info("Generating...");
7584

76-
assert_se(journald_file_open(-1, "test.journal", O_RDWR|O_CREAT, 0666, true, UINT64_MAX, !!verification_key, NULL, NULL, NULL, NULL, &df) == 0);
85+
assert_se(journald_file_open(-1, "test.journal", O_RDWR|O_CREAT, 0666, true, UINT64_MAX, !!verification_key, NULL, m, NULL, NULL, &df) == 0);
7786

7887
for (n = 0; n < N_ENTRIES; n++) {
7988
struct iovec iovec;
@@ -95,7 +104,7 @@ int main(int argc, char *argv[]) {
95104

96105
log_info("Verifying...");
97106

98-
assert_se(journal_file_open(-1, "test.journal", O_RDONLY, 0666, true, UINT64_MAX, !!verification_key, NULL, NULL, NULL, &f) == 0);
107+
assert_se(journal_file_open(-1, "test.journal", O_RDONLY, 0666, true, UINT64_MAX, !!verification_key, NULL, m, NULL, &f) == 0);
99108
/* journal_file_print_header(f); */
100109
journal_file_dump(f);
101110

‎src/journal/test-journal.c‎

Lines changed: 20 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ static void mkdtemp_chdir_chattr(char *path) {
2424
}
2525

2626
static void test_non_empty(void) {
27+
_cleanup_(mmap_cache_unrefp) MMapCache *m = NULL;
2728
dual_timestamp ts;
2829
JournaldFile *f;
2930
struct iovec iovec;
@@ -35,9 +36,12 @@ static void test_non_empty(void) {
3536

3637
test_setup_logging(LOG_DEBUG);
3738

39+
m = mmap_cache_new();
40+
assert_se(m != NULL);
41+
3842
mkdtemp_chdir_chattr(t);
3943

40-
assert_se(journald_file_open(-1, "test.journal", O_RDWR|O_CREAT, 0666, true, UINT64_MAX, true, NULL, NULL, NULL, NULL, &f) == 0);
44+
assert_se(journald_file_open(-1, "test.journal", O_RDWR|O_CREAT, 0666, true, UINT64_MAX, true, NULL, m, NULL, NULL, &f) == 0);
4145

4246
assert_se(dual_timestamp_get(&ts));
4347
assert_se(sd_id128_randomize(&fake_boot_id) == 0);
@@ -98,8 +102,8 @@ static void test_non_empty(void) {
98102

99103
assert_se(journal_file_move_to_entry_by_seqnum(f->file, 10, DIRECTION_DOWN, &o, NULL) == 0);
100104

101-
journald_file_rotate(&f, NULL, true, UINT64_MAX, true, NULL);
102-
journald_file_rotate(&f, NULL, true, UINT64_MAX, true, NULL);
105+
journald_file_rotate(&f, m, true, UINT64_MAX, true, NULL);
106+
journald_file_rotate(&f, m, true, UINT64_MAX, true, NULL);
103107

104108
(void) journald_file_close(f);
105109

@@ -117,17 +121,21 @@ static void test_non_empty(void) {
117121
}
118122

119123
static void test_empty(void) {
124+
_cleanup_(mmap_cache_unrefp) MMapCache *m = NULL;
120125
JournaldFile *f1, *f2, *f3, *f4;
121126
char t[] = "/var/tmp/journal-XXXXXX";
122127

123128
test_setup_logging(LOG_DEBUG);
124129

130+
m = mmap_cache_new();
131+
assert_se(m != NULL);
132+
125133
mkdtemp_chdir_chattr(t);
126134

127-
assert_se(journald_file_open(-1, "test.journal", O_RDWR|O_CREAT, 0666, false, UINT64_MAX, false, NULL, NULL, NULL, NULL, &f1) == 0);
128-
assert_se(journald_file_open(-1, "test-compress.journal", O_RDWR|O_CREAT, 0666, true, UINT64_MAX, false, NULL, NULL, NULL, NULL, &f2) == 0);
129-
assert_se(journald_file_open(-1, "test-seal.journal", O_RDWR|O_CREAT, 0666, false, UINT64_MAX, true, NULL, NULL, NULL, NULL, &f3) == 0);
130-
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);
135+
assert_se(journald_file_open(-1, "test.journal", O_RDWR|O_CREAT, 0666, false, UINT64_MAX, false, NULL, m, NULL, NULL, &f1) == 0);
136+
assert_se(journald_file_open(-1, "test-compress.journal", O_RDWR|O_CREAT, 0666, true, UINT64_MAX, false, NULL, m, NULL, NULL, &f2) == 0);
137+
assert_se(journald_file_open(-1, "test-seal.journal", O_RDWR|O_CREAT, 0666, false, UINT64_MAX, true, NULL, m, NULL, NULL, &f3) == 0);
138+
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);
131139

132140
journal_file_print_header(f1->file);
133141
puts("");
@@ -156,6 +164,7 @@ static void test_empty(void) {
156164

157165
#if HAVE_COMPRESSION
158166
static bool check_compressed(uint64_t compress_threshold, uint64_t data_size) {
167+
_cleanup_(mmap_cache_unrefp) MMapCache *m = NULL;
159168
dual_timestamp ts;
160169
JournaldFile *f;
161170
struct iovec iovec;
@@ -170,9 +179,12 @@ static bool check_compressed(uint64_t compress_threshold, uint64_t data_size) {
170179

171180
test_setup_logging(LOG_DEBUG);
172181

182+
m = mmap_cache_new();
183+
assert_se(m != NULL);
184+
173185
mkdtemp_chdir_chattr(t);
174186

175-
assert_se(journald_file_open(-1, "test.journal", O_RDWR|O_CREAT, 0666, true, compress_threshold, true, NULL, NULL, NULL, NULL, &f) == 0);
187+
assert_se(journald_file_open(-1, "test.journal", O_RDWR|O_CREAT, 0666, true, compress_threshold, true, NULL, m, NULL, NULL, &f) == 0);
176188

177189
dual_timestamp_get(&ts);
178190

‎src/libsystemd/sd-journal/journal-file.c‎

Lines changed: 2 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -3250,13 +3250,13 @@ int journal_file_open(
32503250
JournalFile **ret) {
32513251

32523252
bool newly_created = false;
3253-
MMapCache *m = mmap_cache;
32543253
JournalFile *f;
32553254
void *h;
32563255
int r;
32573256

32583257
assert(ret);
32593258
assert(fd >= 0 || fname);
3259+
assert(mmap_cache);
32603260

32613261
if (!IN_SET((flags & O_ACCMODE), O_RDONLY, O_RDWR))
32623262
return -EINVAL;
@@ -3319,14 +3319,6 @@ int journal_file_open(
33193319
}
33203320
}
33213321

3322-
if (!m) {
3323-
m = mmap_cache_new();
3324-
if (!m) {
3325-
r = -ENOMEM;
3326-
goto fail;
3327-
}
3328-
}
3329-
33303322
if (fname) {
33313323
f->path = strdup(fname);
33323324
if (!f->path) {
@@ -3368,17 +3360,12 @@ int journal_file_open(
33683360
goto fail;
33693361
}
33703362

3371-
/* On success this incs refcnt on *m, which mmap_cache_fd_free() will dec. */
3372-
f->cache_fd = mmap_cache_add_fd(m, f->fd, prot_from_flags(flags));
3363+
f->cache_fd = mmap_cache_add_fd(mmap_cache, f->fd, prot_from_flags(flags));
33733364
if (!f->cache_fd) {
33743365
r = -ENOMEM;
33753366
goto fail;
33763367
}
33773368

3378-
/* If we created *m just for this file, unref *m so only f->cache_fd's ref remains */
3379-
if (!mmap_cache)
3380-
(void) mmap_cache_unref(m);
3381-
33823369
r = journal_file_fstat(f);
33833370
if (r < 0)
33843371
goto fail;

0 commit comments

Comments
 (0)