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.

diff --git a/src/journal/test-journal-interleaving.c b/src/journal/test-journal-interleaving.c
index 08b793c6182fd6827aad1313906403f00c0ddb80..308569d786d220c58dc3a56ed019eb0085db00e2 100644 (file)
--- 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 bc587319b82990ced4c73cb795d72efaca383fcd..47532e2cbc28c41fa903c9f45c507c4836cd2d27 100644 (file)
--- 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) {