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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 11 commits into from
May 10, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
8ac0810
logs-show: drop uid argument from add_matches_for_user_units()
yuwata Apr 25, 2024
131701d
logs-show: add missing strempty()
yuwata Apr 27, 2024
fdd325f
journalctl: several cleanups for parse_boot_descriptor()
yuwata Apr 27, 2024
781ddf1
journalctl: split out journal_acquire_boot() from add_boot()
yuwata Apr 25, 2024
ae0e6de
journalctl: fix --boot=0 with --file=- (from stdin)
yuwata Apr 26, 2024
87dfaba
logs-show: flush matches before and after finding boots
yuwata Apr 26, 2024
a467515
journalctl: fix support of --boot=ID±offset format
yuwata Apr 26, 2024
afcd9c6
logs-show: fix stored timestamp when advance_older is true
yuwata Apr 26, 2024
8f2bcb1
logs-show: use GREEDY_REALLOC_APPEND()
yuwata Apr 26, 2024
5da5d84
journalctl: fail and show error message when no boot ID found
yuwata Apr 25, 2024
d0936a7
journalctl: make --list-boots support -n/--lines= option
yuwata Apr 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 10 additions & 4 deletions man/journalctl.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -851,10 +851,16 @@
<varlistentry>
<term><option>--list-boots</option></term>

<listitem><para>Show a tabular list of boot numbers (relative to the current boot), their IDs, and
the timestamps of the first and last message pertaining to the boot.</para>

<xi:include href="version-info.xml" xpointer="v209"/></listitem>
<listitem>
<para>Show a tabular list of boot numbers (relative to the current boot), their IDs, and the
timestamps of the first and last message pertaining to the boot. When specified with
<option>-n/--lines=<optional>+</optional><replaceable>N</replaceable></option> option, only the
first (when the number prefixed with <literal>+</literal>) or the last (without prefix)
<replaceable>N</replaceable> entries will be shown. When specified with
<option>-r/--reverse</option>, the list will be shown in the reverse order.</para>

<xi:include href="version-info.xml" xpointer="v209"/>
</listitem>
</varlistentry>

<varlistentry>
Expand Down
53 changes: 16 additions & 37 deletions src/journal/journalctl-filter.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#include "journal-internal.h"
#include "journalctl.h"
#include "journalctl-filter.h"
#include "journalctl-util.h"
#include "logs-show.h"
#include "missing_sched.h"
#include "nulstr-util.h"
Expand All @@ -23,42 +24,13 @@ static int add_boot(sd_journal *j) {
if (!arg_boot)
return 0;

/* Take a shortcut and use the current boot_id, which we can do very quickly.
* We can do this only when we logs are coming from the current machine,
* so take the slow path if log location is specified. */
if (arg_boot_offset == 0 && sd_id128_is_null(arg_boot_id) &&
!arg_directory && !arg_file && !arg_root)
return add_match_this_boot(j, arg_machine);

if (sd_id128_is_null(arg_boot_id)) {
r = journal_find_boot_by_offset(j, arg_boot_offset, &arg_boot_id);
if (r < 0)
return log_error_errno(r, "Failed to find journal entry from the specified boot offset (%+i): %m",
arg_boot_offset);
if (r == 0)
return log_error_errno(SYNTHETIC_ERRNO(ENODATA),
"No journal boot entry found from the specified boot offset (%+i).",
arg_boot_offset);
} else {
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));
if (r == 0)
return log_error_errno(SYNTHETIC_ERRNO(ENODATA),
"No journal boot entry found from the specified boot ID (%s).",
SD_ID128_TO_STRING(arg_boot_id));
}
assert(!sd_id128_is_null(arg_boot_id));

r = add_match_boot_id(j, arg_boot_id);
if (r < 0)
return log_error_errno(r, "Failed to add match: %m");

r = sd_journal_add_conjunction(j);
if (r < 0)
return log_error_errno(r, "Failed to add conjunction: %m");
return r;

return 0;
return sd_journal_add_conjunction(j);
}

static int add_dmesg(sd_journal *j) {
Expand Down Expand Up @@ -208,7 +180,7 @@ static int add_units(sd_journal *j) {
if (r < 0)
return r;
} else {
r = add_matches_for_user_unit(j, u, getuid());
r = add_matches_for_user_unit(j, u);
if (r < 0)
return r;
r = sd_journal_add_disjunction(j);
Expand All @@ -227,7 +199,7 @@ static int add_units(sd_journal *j) {
return r;

SET_FOREACH(u, units) {
r = add_matches_for_user_unit(j, u, getuid());
r = add_matches_for_user_unit(j, u);
if (r < 0)
return r;
r = sd_journal_add_disjunction(j);
Expand Down Expand Up @@ -457,12 +429,19 @@ int add_filters(sd_journal *j, char **matches) {

assert(j);

/* add_boot() must be called first!
* It may need to seek the journal to find parent boot IDs. */
r = add_boot(j);
/* First, search boot ID, as that may set and flush matches and seek journal. */
r = journal_acquire_boot(j);
if (r < 0)
return r;

/* Clear unexpected matches for safety. */
sd_journal_flush_matches(j);

/* Then, add filters in the below. */
r = add_boot(j);
if (r < 0)
return log_error_errno(r, "Failed to add filter for boot: %m");

r = add_dmesg(j);
if (r < 0)
return log_error_errno(r, "Failed to add filter for dmesg: %m");
Expand Down
31 changes: 24 additions & 7 deletions src/journal/journalctl-misc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,11 +111,16 @@ int action_list_boots(void) {
if (r < 0)
return r;

r = journal_get_boots(j, &boots, &n_boots);
r = journal_get_boots(
j,
/* advance_older = */ arg_lines_needs_seek_end(),
/* max_ids = */ arg_lines >= 0 ? (size_t) arg_lines : SIZE_MAX,
&boots, &n_boots);
if (r < 0)
return log_error_errno(r, "Failed to determine boots: %m");
if (r == 0)
return 0;
return log_full_errno(arg_quiet ? LOG_DEBUG : LOG_ERR, SYNTHETIC_ERRNO(ENODATA),
"No boot found.");

table = table_new("idx", "boot id", "first entry", "last entry");
if (!table)
Expand All @@ -131,13 +136,25 @@ int action_list_boots(void) {
(void) table_set_sort(table, (size_t) 0);
(void) table_set_reverse(table, 0, arg_reverse);

FOREACH_ARRAY(i, boots, n_boots) {
for (int i = 0; i < (int) n_boots; i++) {
int index;

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;
Comment on lines +142 to +150
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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


r = table_add_many(table,
TABLE_INT, (int)(i - boots) - (int) n_boots + 1,
TABLE_INT, index,
TABLE_SET_ALIGN_PERCENT, 100,
TABLE_ID128, i->id,
TABLE_TIMESTAMP, i->first_usec,
TABLE_TIMESTAMP, i->last_usec);
TABLE_ID128, boots[i].id,
TABLE_TIMESTAMP, boots[i].first_usec,
TABLE_TIMESTAMP, boots[i].last_usec);
if (r < 0)
return table_log_add_error(r);
}
Expand Down
48 changes: 48 additions & 0 deletions src/journal/journalctl-util.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,11 @@

#include <unistd.h>

#include "id128-util.h"
#include "journal-util.h"
#include "journalctl.h"
#include "journalctl-util.h"
#include "logs-show.h"
#include "rlimit-util.h"
#include "sigbus.h"
#include "terminal-util.h"
Expand Down Expand Up @@ -70,3 +72,49 @@ bool journal_boot_has_effect(sd_journal *j) {

return true;
}

int journal_acquire_boot(sd_journal *j) {
int r;

assert(j);

if (!arg_boot) {
/* Clear relevant field for safety. */
arg_boot_id = SD_ID128_NULL;
arg_boot_offset = 0;
return 0;
}

/* Take a shortcut and use the current boot_id, which we can do very quickly.
* We can do this only when the logs are coming from the current machine,
* so take the slow path if log location is specified. */
if (arg_boot_offset == 0 && sd_id128_is_null(arg_boot_id) &&
!arg_directory && !arg_file && !arg_file_stdin && !arg_root) {
r = id128_get_boot_for_machine(arg_machine, &arg_boot_id);
if (r < 0)
return log_error_errno(r, "Failed to get boot ID%s%s: %m",
isempty(arg_machine) ? "" : " of container ", strempty(arg_machine));
} else {
sd_id128_t boot_id;

r = journal_find_boot(j, arg_boot_id, arg_boot_offset, &boot_id);
if (r < 0)
return log_error_errno(r, "Failed to find journal entry from the specified boot (%s%+i): %m",
sd_id128_is_null(arg_boot_id) ? "" : SD_ID128_TO_STRING(arg_boot_id),
arg_boot_offset);
if (r == 0)
return log_error_errno(SYNTHETIC_ERRNO(ENODATA),
"No journal boot entry found from the specified boot (%s%+i).",
sd_id128_is_null(arg_boot_id) ? "" : SD_ID128_TO_STRING(arg_boot_id),
arg_boot_offset);

log_debug("Found boot %s for %s%+i",
SD_ID128_TO_STRING(boot_id),
sd_id128_is_null(arg_boot_id) ? "" : SD_ID128_TO_STRING(arg_boot_id),
arg_boot_offset);

arg_boot_id = boot_id;
}

return 1;
}
1 change: 1 addition & 0 deletions src/journal/journalctl-util.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,3 +8,4 @@
char* format_timestamp_maybe_utc(char *buf, size_t l, usec_t t);
int acquire_journal(sd_journal **ret);
bool journal_boot_has_effect(sd_journal *j);
int journal_acquire_boot(sd_journal *j);
35 changes: 17 additions & 18 deletions src/journal/journalctl.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,15 +104,21 @@ STATIC_DESTRUCTOR_REGISTER(arg_output_fields, set_freep);
STATIC_DESTRUCTOR_REGISTER(arg_compiled_pattern, pattern_freep);
STATIC_DESTRUCTOR_REGISTER(arg_image_policy, image_policy_freep);

static int parse_boot_descriptor(const char *x, sd_id128_t *boot_id, int *offset) {
static int parse_id_descriptor(const char *x, sd_id128_t *ret_id, int *ret_offset) {
sd_id128_t id = SD_ID128_NULL;
int off = 0, r;

assert(x);
assert(ret_id);
assert(ret_offset);

if (streq(x, "all")) {
*boot_id = SD_ID128_NULL;
*offset = 0;
*ret_id = SD_ID128_NULL;
*ret_offset = 0;
return 0;
} else if (strlen(x) >= SD_ID128_STRING_MAX - 1) {
}

if (strlen(x) >= SD_ID128_STRING_MAX - 1) {
char *t;

t = strndupa_safe(x, SD_ID128_STRING_MAX - 1);
Expand All @@ -134,12 +140,8 @@ static int parse_boot_descriptor(const char *x, sd_id128_t *boot_id, int *offset
return r;
}

if (boot_id)
*boot_id = id;

if (offset)
*offset = off;

*ret_id = id;
*ret_offset = off;
return 1;
}

Expand Down Expand Up @@ -517,19 +519,16 @@ static int parse_argv(int argc, char *argv[]) {
arg_boot_offset = 0;

if (optarg) {
r = parse_boot_descriptor(optarg, &arg_boot_id, &arg_boot_offset);
r = parse_id_descriptor(optarg, &arg_boot_id, &arg_boot_offset);
if (r < 0)
return log_error_errno(r, "Failed to parse boot descriptor '%s'", optarg);

arg_boot = r;

/* Hmm, no argument? Maybe the next
* word on the command line is
* supposed to be the argument? Let's
* see if there is one and is parsable
* as a boot descriptor... */
} else if (optind < argc) {
r = parse_boot_descriptor(argv[optind], &arg_boot_id, &arg_boot_offset);
/* Hmm, no argument? Maybe the next word on the command line is supposed to be the
* argument? Let's see if there is one and is parsable as a boot descriptor... */
r = parse_id_descriptor(argv[optind], &arg_boot_id, &arg_boot_offset);
if (r >= 0) {
arg_boot = r;
optind++;
Expand Down Expand Up @@ -958,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))
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

return log_error_errno(SYNTHETIC_ERRNO(EINVAL),
"--lines=+N is unsupported when --reverse or --follow is specified.");

Expand Down
63 changes: 47 additions & 16 deletions src/libsystemd/sd-journal/test-journal-interleaving.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -459,7 +459,7 @@ TEST(skip) {

static void test_boot_id_one(void (*setup)(void), size_t n_boots_expected) {
char t[] = "/var/tmp/journal-boot-id-XXXXXX";
sd_journal *j;
_cleanup_(sd_journal_closep) sd_journal *j = NULL;
_cleanup_free_ BootId *boots = NULL;
size_t n_boots;

Expand All @@ -468,27 +468,58 @@ static void test_boot_id_one(void (*setup)(void), size_t n_boots_expected) {
setup();

assert_ret(sd_journal_open_directory(&j, t, SD_JOURNAL_ASSUME_IMMUTABLE));
assert_se(journal_get_boots(j, &boots, &n_boots) >= 0);
assert_se(journal_get_boots(
j,
/* advance_older = */ false, /* max_ids = */ SIZE_MAX,
&boots, &n_boots) >= 0);
assert_se(boots);
assert_se(n_boots == n_boots_expected);
sd_journal_close(j);

FOREACH_ARRAY(b, boots, n_boots) {
assert_ret(sd_journal_open_directory(&j, t, SD_JOURNAL_ASSUME_IMMUTABLE));
assert_se(journal_find_boot_by_id(j, b->id) == 1);
sd_journal_close(j);
for (size_t i = 0; i < n_boots; i++) {
sd_id128_t id;

/* positive offset */
assert_se(journal_find_boot(j, SD_ID128_NULL, (int) (i + 1), &id) == 1);
assert_se(sd_id128_equal(id, boots[i].id));

/* negative offset */
assert_se(journal_find_boot(j, SD_ID128_NULL, (int) (i + 1) - (int) n_boots, &id) == 1);
assert_se(sd_id128_equal(id, boots[i].id));

for (size_t k = 0; k < n_boots; k++) {
int offset = (int) k - (int) i;

/* relative offset */
assert_se(journal_find_boot(j, boots[i].id, offset, &id) == 1);
assert_se(sd_id128_equal(id, boots[k].id));
}
}

for (int i = - (int) n_boots + 1; i <= (int) n_boots; i++) {
sd_id128_t id;
for (size_t i = 0; i <= n_boots_expected + 1; i++) {
_cleanup_free_ BootId *boots_limited = NULL;
size_t n_boots_limited;

assert_se(journal_get_boots(
j,
/* advance_older = */ false, /* max_ids = */ i,
&boots_limited, &n_boots_limited) >= 0);
assert_se(boots_limited || i == 0);
assert_se(n_boots_limited == MIN(i, n_boots_expected));
assert_se(memcmp_safe(boots, boots_limited, n_boots_limited * sizeof(BootId)) == 0);
}

assert_ret(sd_journal_open_directory(&j, t, SD_JOURNAL_ASSUME_IMMUTABLE));
assert_se(journal_find_boot_by_offset(j, i, &id) == 1);
if (i <= 0)
assert_se(sd_id128_equal(id, boots[n_boots + i - 1].id));
else
assert_se(sd_id128_equal(id, boots[i - 1].id));
sd_journal_close(j);
for (size_t i = 0; i <= n_boots_expected + 1; i++) {
_cleanup_free_ BootId *boots_limited = NULL;
size_t n_boots_limited;

assert_se(journal_get_boots(
j,
/* advance_older = */ true, /* max_ids = */ i,
&boots_limited, &n_boots_limited) >= 0);
assert_se(boots_limited || i == 0);
assert_se(n_boots_limited == MIN(i, n_boots_expected));
for (size_t k = 0; k < n_boots_limited; k++)
assert_se(memcmp(&boots[n_boots - k - 1], &boots_limited[k], sizeof(BootId)) == 0);
}

test_done(t);
Expand Down