From 7a4ee861615101ddd2f95056cf30e69e41da86ce Mon Sep 17 00:00:00 2001 From: Chen Qi Date: Fri, 24 Feb 2023 12:36:55 +0800 Subject: [PATCH] Revert "Revert "journal: Make sd_journal_previous/next() return 0 at HEAD/TAIL"" This reverts commit 1db6dbb1dcdacfd7d2b4c84562fc6e77bc8c43a5. The original patch was reverted because of issue #25369. The issue was created because it wrongly assumed that sd_journal_seek_tail() seeks to 'current' tail. But in fact, only when a subsequent sd_journal_previous() is called that it's pointing to the tail at that time. The concept of 'tail' in sd_journal_seek_tail() only has a logical meaning, and a sd_journal_previous is needed. In fact, if we look at the codes in journalctl, we can see sd_journal_seek_tail() is followed by sd_journal_previous(). By contrary, a sd_journal_next() after a 'logical' tail does not make much sense. So the original patch is correct, and projects that are using 'sd_journal_next()' right after 'sd_journal_seek_tail()' should do fixes as in https://gitlab.freedesktop.org/pipewire/pipewire/-/issues/2823#note_1637715. --- src/journal/test-journal-interleaving.c | 42 ++++++++++--------------- src/libsystemd/sd-journal/sd-journal.c | 8 ++--- 2 files changed, 20 insertions(+), 30 deletions(-) diff --git a/src/journal/test-journal-interleaving.c b/src/journal/test-journal-interleaving.c index 08b793c6182..308569d786d 100644 --- a/src/journal/test-journal-interleaving.c +++ b/src/journal/test-journal-interleaving.c @@ -188,24 +188,19 @@ static void test_skip_one(void (*setup)(void)) { /* Seek to head, move to previous, then iterate down. */ assert_ret(sd_journal_open_directory(&j, t, 0)); assert_ret(sd_journal_seek_head(j)); - /* FIXME: calling sd_journal_previous() thrice (or >= number of journal files) here works as - * mostly equivalent to calling sd_journal_next(). */ - assert_ret(sd_journal_previous(j)); - assert_ret(sd_journal_previous(j)); - assert_ret(sd_journal_previous(j)); /* pointing the first entry */ + assert_ret(sd_journal_previous(j)); /* no-op */ + assert_ret(sd_journal_next(j)); /* pointing the first entry */ test_check_numbers_down(j, 9); sd_journal_close(j); /* Seek to head, walk several steps, then iterate down. */ assert_ret(sd_journal_open_directory(&j, t, 0)); assert_ret(sd_journal_seek_head(j)); - assert_ret(sd_journal_previous(j)); - assert_ret(sd_journal_previous(j)); - assert_ret(sd_journal_previous(j)); /* pointing the first entry */ - assert_ret(sd_journal_next(j)); /* pointing the second entry */ - /* FIXME: as calling sd_journal_previous() thrice works equivalent to sd_journal_next(), - * we need to call sd_journal_previous() here. */ - assert_ret(sd_journal_previous(j)); /* pointing the first entry */ + assert_ret(sd_journal_previous(j)); /* no-op */ + assert_ret(sd_journal_previous(j)); /* no-op */ + assert_ret(sd_journal_previous(j)); /* no-op */ + assert_ret(sd_journal_next(j)); /* pointing the first entry */ + assert_ret(sd_journal_previous(j)); /* no-op */ assert_ret(sd_journal_previous(j)); /* no-op */ test_check_numbers_down(j, 9); sd_journal_close(j); @@ -220,22 +215,19 @@ static void test_skip_one(void (*setup)(void)) { /* Seek to tail, move to next, then iterate up. */ assert_ret(sd_journal_open_directory(&j, t, 0)); assert_ret(sd_journal_seek_tail(j)); - /* FIXME: calling sd_journal_next() thrice (or >= number of journal files) here works as mostly - * equivalent to calling sd_journal_previous(). */ - assert_ret(sd_journal_next(j)); - assert_ret(sd_journal_next(j)); - assert_ret(sd_journal_next(j)); /* pointing the last entry */ + assert_ret(sd_journal_next(j)); /* no-op */ + assert_ret(sd_journal_previous(j)); /* pointing the first entry */ test_check_numbers_up(j, 9); sd_journal_close(j); /* Seek to tail, walk several steps, then iterate up. */ assert_ret(sd_journal_open_directory(&j, t, 0)); assert_ret(sd_journal_seek_tail(j)); - assert_ret(sd_journal_next(j)); - assert_ret(sd_journal_next(j)); - assert_ret(sd_journal_next(j)); /* pointing the last entry */ - assert_ret(sd_journal_previous(j)); /* pointing the previous of the last entry. */ - assert_ret(sd_journal_next(j)); /* pointing the last entry */ + assert_ret(sd_journal_next(j)); /* no-op */ + assert_ret(sd_journal_next(j)); /* no-op */ + assert_ret(sd_journal_next(j)); /* no-op */ + assert_ret(sd_journal_previous(j)); /* pointing the last entry. */ + assert_ret(sd_journal_next(j)); /* no-op */ assert_ret(sd_journal_next(j)); /* no-op */ test_check_numbers_up(j, 9); sd_journal_close(j); @@ -251,8 +243,7 @@ static void test_skip_one(void (*setup)(void)) { /* Seek to tail, skip to head in a more complext way, then iterate down. */ assert_ret(sd_journal_open_directory(&j, t, 0)); assert_ret(sd_journal_seek_tail(j)); - // FIXME: sd_journal_next() cannot be called here. - //assert_ret(sd_journal_next(j)); + assert_ret(sd_journal_next(j)); /* no-op */ assert_ret(r = sd_journal_previous_skip(j, 4)); assert_se(r == 4); assert_ret(r = sd_journal_previous_skip(j, 5)); @@ -284,8 +275,7 @@ static void test_skip_one(void (*setup)(void)) { /* Seek to head, skip to tail in a more complex way, then iterate up. */ assert_ret(sd_journal_open_directory(&j, t, 0)); assert_ret(sd_journal_seek_head(j)); - // FIXME: sd_journal_previous() cannot be called here. - //assert_ret(sd_journal_previous(j)); + assert_ret(sd_journal_previous(j)); /* no-op */ assert_ret(r = sd_journal_next_skip(j, 4)); assert_se(r == 4); assert_ret(r = sd_journal_next_skip(j, 5)); diff --git a/src/libsystemd/sd-journal/sd-journal.c b/src/libsystemd/sd-journal/sd-journal.c index bc587319b82..47532e2cbc2 100644 --- a/src/libsystemd/sd-journal/sd-journal.c +++ b/src/libsystemd/sd-journal/sd-journal.c @@ -660,9 +660,9 @@ static int find_location_for_match( /* FIXME: missing: find by monotonic */ if (j->current_location.type == LOCATION_HEAD) - return journal_file_next_entry_for_data(f, d, DIRECTION_DOWN, ret, offset); + return direction == DIRECTION_DOWN ? journal_file_next_entry_for_data(f, d, DIRECTION_DOWN, ret, offset) : 0; if (j->current_location.type == LOCATION_TAIL) - return journal_file_next_entry_for_data(f, d, DIRECTION_UP, ret, offset); + return direction == DIRECTION_UP ? journal_file_next_entry_for_data(f, d, DIRECTION_UP, ret, offset) : 0; if (j->current_location.seqnum_set && sd_id128_equal(j->current_location.seqnum_id, f->header->seqnum_id)) return journal_file_move_to_entry_by_seqnum_for_data(f, d, j->current_location.seqnum, direction, ret, offset); if (j->current_location.monotonic_set) { @@ -755,9 +755,9 @@ static int find_location_with_matches( /* No matches is simple */ if (j->current_location.type == LOCATION_HEAD) - return journal_file_next_entry(f, 0, DIRECTION_DOWN, ret, offset); + return direction == DIRECTION_DOWN ? journal_file_next_entry(f, 0, DIRECTION_DOWN, ret, offset) : 0; if (j->current_location.type == LOCATION_TAIL) - return journal_file_next_entry(f, 0, DIRECTION_UP, ret, offset); + return direction == DIRECTION_UP ? journal_file_next_entry(f, 0, DIRECTION_UP, ret, offset) : 0; if (j->current_location.seqnum_set && sd_id128_equal(j->current_location.seqnum_id, f->header->seqnum_id)) return journal_file_move_to_entry_by_seqnum(f, j->current_location.seqnum, direction, ret, offset); if (j->current_location.monotonic_set) { -- 2.47.3