Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sdjournal: SeekTail should be followed by Previous, not Next #345

Merged
merged 1 commit into from
Mar 3, 2021

Conversation

stevenjm
Copy link
Contributor

sd_journal_next is supposed to return 0 (i.e. EOF) if called at the end of the journal, which would make this issue immediately apparent. However, there is an open bug that causes it to advance to the wrong message instead (not the last one in the journal) after a call to sd_journal_seek_tail, which causes code which follows the documented use here to produce non-obviously incorrect results (see systemd/systemd#9934).

Instead, sd_journal_previous will correctly seek to the last journal entry, with the unavoidable race condition that it may not actually be the last entry if the journal is written to between calls to sd_journal_seek_tail and sd_journal_previous.

This can be verified by comparing the output of journalctl -o json | tail -1 | jq -r .MESSAGE_ID with that of the following simple Go program

package main

import (
        "fmt"

        "github.com/coreos/go-systemd/sdjournal"
)

func main() {
        j, err := sdjournal.NewJournal()
        if err != nil {
                panic(err)
        }

        err = j.SeekTail()
        if err != nil {
                panic(err)
        }

        n, err := j.Previous()
        if err != nil {
                panic(err)
        }

        fmt.Printf("went back by %d\n", n)

        e, err := j.GetEntry()
        if err != nil {
                panic(err)
        }

        fmt.Printf("found message id %s\n", e.Fields["MESSAGE_ID"])
}

and noting that the message IDs are equal (again, with an unavoidable race condition if a message is written in between).

sd_journal_next is supposed to return 0 (i.e. EOF) if called at the
end of the journal, which would make this issue immediately
apparent. However, there is an open bug that causes it to advance
to the *wrong* message instead (not the last one in the journal)
after a call to sd_journal_seek_tail, which causes code which
follows the documented use here to produce non-obviously incorrect
results (see <systemd/systemd#9934>).

Instead, sd_journal_previous will correctly seek to the last
journal entry, with the unavoidable race condition that it may not
actually be the last entry if the journal is written to between
calls to sd_journal_seek_tail and sd_journal_previous.

This can be verified by comparing the output of `journalctl -o json
| tail -1 | jq -r .MESSAGE_ID` with that of the following simple Go
program

```
package main

import (
        "fmt"

        "github.com/coreos/go-systemd/sdjournal"
)

func main() {
        j, err := sdjournal.NewJournal()
        if err != nil {
                panic(err)
        }

        err = j.SeekTail()
        if err != nil {
                panic(err)
        }

        n, err := j.Previous()
        if err != nil {
                panic(err)
        }

        fmt.Printf("went back by %d\n", n)

        e, err := j.GetEntry()
        if err != nil {
                panic(err)
        }

        fmt.Printf("found message id %s\n", e.Fields["MESSAGE_ID"])
}
```

and noting that the message IDs are equal (again, with an
unavoidable race condition if a message is written in between).
Copy link

@pires pires left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@lucab
Copy link
Contributor

lucab commented Feb 23, 2021

LGTM. Can you please rebase on current master so that newer GH actions can run on this?

@lucab lucab closed this Mar 3, 2021
@lucab lucab reopened this Mar 3, 2021
@lucab
Copy link
Contributor

lucab commented Mar 3, 2021

Nevermind, I managed to manually re-sync the CI. All checks are green, merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants