journalctl: several fixes and cleanups for --boot= option handling #32491

yuwata · 2024-04-26T02:48:00Z

split-out of #32448.

src/journal/journalctl-util.c

options Prompted by systemd#32491

YHNdnzj · 2024-04-29T08:41:22Z

src/journal/journalctl-util.c

+                r = journal_find_boot_by_id(j, arg_boot_id);
+                if (r < 0)
+                        return log_error_errno(r, "Failed to find journal entry from the specified boot ID (%s): %m",
+                                               SD_ID128_TO_STRING(arg_boot_id));


switch to SD_ID128_FORMAT_STR + SD_ID128_FORMAT_VAL while at it?

Personally, I prefer '%s' + SD_ID128_TO_STRING(). Here is not a performance critical path, so there should be no difference.

src/journal/journalctl-util.c

src/journal/journalctl-misc.c

YHNdnzj · 2024-04-29T09:03:33Z

src/journal/journalctl.c

@@ -957,7 +957,7 @@ static int parse_argv(int argc, char *argv[]) {
                return log_error_errno(SYNTHETIC_ERRNO(EINVAL),
                                       "Please specify either --reverse or --follow, not both.");

-        if (arg_lines >= 0 && arg_lines_oldest && (arg_reverse || arg_follow))
+        if (arg_action == ACTION_SHOW && arg_lines >= 0 && arg_lines_oldest && (arg_reverse || arg_follow))


Hmm, I think asking for the first N lines and then reverse that is really a niche use case, that I don't want to support. Let's leave this out for now I'd say, so that this is less confusing for different verbs. We can always revisit this if people really want it.

Before the commit, arg_lines is only used when showing journal entries. Using the option in --list-boots is completely new here. And when showing list of boot IDs, the semantics is completely simple, and --lines= and --reverse can be and should be completely independent. If we keep the condition, even when showing list of boots, the options needlessly and spuriously interfere each other.

I think asking for the first N lines and then reverse that is really a niche use case

I do not think it is niche. Even if it is, there is no reason to refuse the combination. Both behavior and implementation is super simple to understand.

Personally, I'd like to also support the combination even when showing journal entries, even if it is niche. But, that's not simple to implement, and I'd like to leave that to a later PR.

options Prompted by #32491

src/journal/journalctl-util.c

It is always equivalent to getuid(). Let's call getuid() in the function instead.

Follow-up for 8e976dc.

- rename to parse_id_descriptor(), to make it usable for other kind of ID later. - add missing assertions, - prefix arguments for storing results with 'ret_', - drop unnecessary 'else'.

No functional change, just refactoring and prepration for later changes.

Follow-up for 592855c.

Otherwise, if several matches already set, then the first seek to head or tail may move the cursor to an invalid place, hence they provide wrong ID(s). Also, reading journal after calling these function may provide unexpected data. Currently, the caller does not install any matches before calling the functions, and does not read any journal entry after journal_get_boots() succeeds or journal_find_boot_by_offset() succeeds with 0. Hence, this should not change any behavior. Just for safety.

Fixes a regression introduced by e44f060. After the offending commit, if a boot ID suffixed with an offset is specified to --boot=, the boot ID was ignored. This fixes the issue. To fix the issue, this merges journal_find_boot_by_id() and journal_find_boot_by_offset().

Currently, the parsed timestamp is only used when advance_older is false. Hence, this does not change any behavior. But, let's fix it anyway.

No boot ID in journal should be definitly spurious. Let's warn about that and exit with failure.

Also mention that -r/--reverse is supported by the command.

YHNdnzj · 2024-05-10T12:41:42Z

src/journal/journalctl-misc.c

+                if (arg_lines_needs_seek_end())
+                        /* With --lines=N, we only know the negative index, and the older ID is located earlier. */
+                        index = -i;
+                else if (arg_lines >= 0)
+                        /* With --lines=+N, we only know the positive index, and the newer ID is located earlier. */
+                        index = i + 1;
+                else
+                        /* Otherwise, show negative index. Note, in this case, newer ID is located earlier. */
+                        index = i + 1 - (int) n_boots;


Hmm, is the comment correct? I think the index is sorted in ascending order, hence older IDs are always shown earlier?

Correct. The last commit adds additional argument for journal_get_boots() that controls the ordering.

options Prompted by systemd#32491 (cherry picked from commit 821bf13)

yuwata added the journal label Apr 26, 2024

yuwata added this to the v256 milestone Apr 26, 2024

github-actions bot added util-lib tests please-review PR is ready for (re-)review by a maintainer labels Apr 26, 2024

yuwata force-pushed the journalctl-fix-boot branch 2 times, most recently from dbe7471 to cc869c3 Compare April 26, 2024 06:03

YHNdnzj reviewed Apr 26, 2024

View reviewed changes

src/journal/journalctl-util.c Outdated Show resolved Hide resolved

yuwata force-pushed the journalctl-fix-boot branch from cc869c3 to c705b5f Compare April 27, 2024 08:55

github-actions bot added login documentation systemctl labels Apr 27, 2024

yuwata changed the title ~~journalctl: several fixes for --boot= option handling~~ journalctl: several fixes and cleanups for --boot= option handling Apr 27, 2024

YHNdnzj added a commit to YHNdnzj/systemd that referenced this pull request Apr 29, 2024

journalctl: also check arg_file_stdin with other journal location

a18d60a

options Prompted by systemd#32491

YHNdnzj mentioned this pull request Apr 29, 2024

journalctl: also check arg_file_stdin with other journal location options #32549

Merged

YHNdnzj reviewed Apr 29, 2024

View reviewed changes

bluca added reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks and removed please-review PR is ready for (re-)review by a maintainer labels Apr 29, 2024

bluca pushed a commit that referenced this pull request Apr 29, 2024

journalctl: also check arg_file_stdin with other journal location

821bf13

options Prompted by #32491

yuwata force-pushed the journalctl-fix-boot branch from c705b5f to 7acfa10 Compare April 30, 2024 02:26

github-actions bot added please-review PR is ready for (re-)review by a maintainer and removed reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks labels Apr 30, 2024

bluca approved these changes May 3, 2024

View reviewed changes

src/journal/journalctl-util.c Outdated Show resolved Hide resolved

bluca added needs-rebase good-to-merge/with-minor-suggestions and removed please-review PR is ready for (re-)review by a maintainer labels May 3, 2024

yuwata added 4 commits May 10, 2024 09:35

logs-show: drop uid argument from add_matches_for_user_units()

8ac0810

It is always equivalent to getuid(). Let's call getuid() in the function instead.

logs-show: add missing strempty()

131701d

Follow-up for 8e976dc.

journalctl: several cleanups for parse_boot_descriptor()

fdd325f

- rename to parse_id_descriptor(), to make it usable for other kind of ID later. - add missing assertions, - prefix arguments for storing results with 'ret_', - drop unnecessary 'else'.

journalctl: split out journal_acquire_boot() from add_boot()

781ddf1

No functional change, just refactoring and prepration for later changes.

yuwata added 7 commits May 10, 2024 11:43

journalctl: fix --boot=0 with --file=- (from stdin)

ae0e6de

Follow-up for 592855c.

logs-show: fix stored timestamp when advance_older is true

afcd9c6

Currently, the parsed timestamp is only used when advance_older is false. Hence, this does not change any behavior. But, let's fix it anyway.

logs-show: use GREEDY_REALLOC_APPEND()

8f2bcb1

journalctl: fail and show error message when no boot ID found

5da5d84

No boot ID in journal should be definitly spurious. Let's warn about that and exit with failure.

journalctl: make --list-boots support -n/--lines= option

d0936a7

Also mention that -r/--reverse is supported by the command.

yuwata force-pushed the journalctl-fix-boot branch from 7acfa10 to d0936a7 Compare May 10, 2024 02:45

github-actions bot removed the needs-rebase label May 10, 2024

yuwata added good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed and removed good-to-merge/with-minor-suggestions labels May 10, 2024

bluca merged commit cf2b044 into systemd:main May 10, 2024
43 of 48 checks passed

github-actions bot removed the good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed label May 10, 2024

yuwata deleted the journalctl-fix-boot branch May 10, 2024 11:45

YHNdnzj reviewed May 10, 2024

View reviewed changes

BtbN pushed a commit to BtbN/systemd that referenced this pull request May 14, 2024

journalctl: also check arg_file_stdin with other journal location

2c1ce9f

options Prompted by systemd#32491 (cherry picked from commit 821bf13)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

journalctl: several fixes and cleanups for --boot= option handling #32491

journalctl: several fixes and cleanups for --boot= option handling #32491

yuwata commented Apr 26, 2024 •

edited

YHNdnzj Apr 29, 2024

yuwata Apr 30, 2024

YHNdnzj Apr 29, 2024

yuwata Apr 30, 2024

YHNdnzj May 10, 2024

yuwata May 10, 2024

journalctl: several fixes and cleanups for --boot= option handling #32491

journalctl: several fixes and cleanups for --boot= option handling #32491

Conversation

yuwata commented Apr 26, 2024 • edited

YHNdnzj Apr 29, 2024

Choose a reason for hiding this comment

yuwata Apr 30, 2024

Choose a reason for hiding this comment

YHNdnzj Apr 29, 2024

Choose a reason for hiding this comment

yuwata Apr 30, 2024

Choose a reason for hiding this comment

YHNdnzj May 10, 2024

Choose a reason for hiding this comment

yuwata May 10, 2024

Choose a reason for hiding this comment

yuwata commented Apr 26, 2024 •

edited