Re: [PATCH proxmox-mini-journalreader v3 1/2] add -B parameter to lists boots

[Maximiliano Sandoval <m.sandoval@proxmox.com>]

Replies below of what was addressed in v4.

"Daniel Kral" <d.kral@proxmox.com> writes:

> Thanks for tackling this!
>
> Works as expected, some comments inline.
>
> The style-nits are from running a clang-format on it as the rest of the
> code seemed like it used that already? Might be nice to add a
> tidy/format target, but out of scope of this series.
>
> On Thu Aug 6, 2026 at 4:09 PM CEST, Maximiliano Sandoval wrote:
>> The parameter will print all boots, in practice it should be piped into tail.
>>
>> Sample output:
>>
>> 2026-07-27 08:28	2026-07-27 15:56	3e3813556d53477f9a139a7cb2f3e8f2	7.0.14-6-pve
>> 2026-07-27 15:57	2026-07-27 17:03	5a0324e1ac3f4f24907a21ab81b67384	7.0.14-7-pve
>> 2026-07-28 08:26	2026-07-28 17:07	e62bc71e85274f21a1bfb954be38c071	7.0.14-7-pve
>> 2026-07-29 08:43	2026-07-29 08:48	7f668e4499644ea1b8339d6b8c29d53d	7.0.14-7-pve
>> 2026-07-29 08:49	2026-07-29 17:09	9ff5eb6f0485466383e0b89512f937c9	7.0.14-8-pve
>>
>> glib is added as a dependency for simplicity of memory management.
>>
>> As per SD_JOURNAL_QUERY_UNIQUE(3), sd_journal_query_unique ignore matches hence
>> why we need to iterate twice:
>>
>>      Note that these functions currently are not influenced by matches
>>      set with sd_journal_add_match() but this might change in a later
>>      version of this software.
>>
>> Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
>> ---
>>
>> Notes:
>>     We usually require the output from `last` or similar in support to query the
>>     previously booted kernel versions. We add this helper to avoid adding a
>>     dependency on either last or wtmpdb on the ISO images.
>>     
>>     This seems quick enough for ~400 boots:
>>     
>>     time sudo ./proxmox-mini-journalreader -B | wc -l
>>     417
>>     
>>     real	0m0,143s
>>     user	0m0,004s
>>     sys	0m0,004s
>
> This information should be in the patch message to give more context for
> what use case this was added for future readers.
>

Moved the rationale, left `time`s in the notes.

>>     
>>     Open questions:
>>     
>>     - Should it list the latest boots first? It amounts to reversing the sign on
>>     boot_info_cmp.
>
> I guess going from oldest to newest is the default for the rest of
> proxmox-mini-journalreader (and journalctl) as well, so if we really
> need it the other way around adding an option for that might be better.
>
>>     - We assume the first line in the journal is the "Linux version" line. This is
>>     true as of now in practice. One could add a match like the following:
>>     
>>          sd_journal_add_match(j, "_TRANSPORT=kernel", strlen("_TRANSPORT=kernel"));
>>     
>>       if this is undesirable.
>
> Looking at the relevant code [0], the Linux version line might actually
> not be the first line... At least cgroup_init_early() could
> theoretically print warnings earlier than the Linux version line, even
> though that is unlikely.
>
> So if we want to be very robust we would need to check the first few
> lines of each boot until we hit the Linux version line, though I don't
> think it's worth the added complexity here.
>
> [0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/init/main.c?h=v7.1#n1038
>

Lets leave it like this for now. I have not seen a single journal that
does not start like this.

>>     
>>     Differences from v2:
>>      - Mark regex_extract_match and format_timestamp functions as static char *
>>      - Move the regex out of the for loop so it is only compiled once
>>      - Add G_GNUC_UNUSED hint so that user_data does not produce an unused warning. We need
>>        that parameter as part of the expected signature
>>      - Remove duplicated check for regex != NULL
>>     
>>     Differences from v1:
>>      - Replace uint with size_t
>>      - Remove the use of a second journal
>>      - Copy only 32 chars instead of 33. We were copying \0 which is added by
>>        g_strndup anyways and 32 is the actual length of a BOOT_ID
>>      - use strlen("BOOT_ID=") instead of sizeof("BOOT_ID") so it does not work by accident
>>      - Use proper signature for GCompareFunc
>>      - Add a couple of guards for empty messages
>>
>>  debian/control           |   2 +-
>>  src/Makefile             |   2 +-
>>  src/mini-journalreader.c | 152 ++++++++++++++++++++++++++++++++++++++-
>>  3 files changed, 153 insertions(+), 3 deletions(-)
>>
>> diff --git a/debian/control b/debian/control
>> index 8280f6d..d7d8cc0 100644
>> --- a/debian/control
>> +++ b/debian/control
>> @@ -2,7 +2,7 @@ Source: proxmox-mini-journalreader
>>  Section: admin
>>  Priority: optional
>>  Maintainer: Proxmox Support Team <support@proxmox.com>
>> -Build-Depends: debhelper-compat (= 13), libsystemd-dev, pkg-config, scdoc,
>> +Build-Depends: debhelper-compat (= 13), libsystemd-dev, pkg-config, scdoc, libglib2.0-dev,
>>  Standards-Version: 4.6.2
>>  
>>  Package: proxmox-mini-journalreader
>> diff --git a/src/Makefile b/src/Makefile
>> index e64e066..c89e74a 100644
>> --- a/src/Makefile
>> +++ b/src/Makefile
>> @@ -5,7 +5,7 @@ LIBEXEC_DIR ?= $(DESTDIR)/usr/libexec/
>>  MAN1_DIR ?= $(DESTDIR)/usr/share/man/man1
>>  MANPAGE ?= $(PROGRAM).1
>>  
>> -LIBS := libsystemd
>> +LIBS := libsystemd glib-2.0
>>  CFLAGS += -Wall -Wextra -Wl,-z,relro -g -O2 --std=gnu11
>>  CFLAGS += -fstack-protector-strong -D_FORTIFY_SOURCE=2
>>  CFLAGS += $(shell pkg-config --cflags $(LIBS))
>> diff --git a/src/mini-journalreader.c b/src/mini-journalreader.c
>> index fc78f65..8a7d473 100644
>> --- a/src/mini-journalreader.c
>> +++ b/src/mini-journalreader.c
>> @@ -21,6 +21,7 @@
>>  
>>  #include <errno.h>
>>  #include <fnmatch.h>
>> +#include <glib.h>
>>  #include <stdbool.h>
>>  #include <stdint.h>
>>  #include <stdio.h>
>> @@ -37,6 +38,146 @@ bool json = false;
>>  bool structured = false;
>>  bool first_line = true;
>>  
>> +typedef struct {
>> +    char *boot_id;
>> +    char *version;
>> +    uint64_t first_timestamp;
>> +    uint64_t last_timestamp;
>> +} BootInfo;
>> +
>> +static void boot_info_free(BootInfo *info) {
>> +    g_free(info->boot_id);
>> +    g_free(info->version);
>> +    g_free(info);
>> +}
>
> nit: maybe then also add a boot_info_{create,init,...}() to put the
>      allocs visually near the frees?
>

Imo it does not improve much the situation, since it is only allocated
in one place.

>> +
>> +static int boot_info_cmp(gconstpointer a, gconstpointer b) {
>> +    const BootInfo *entry1 = a;
>> +    const BootInfo *entry2 = b;
>> +
>> +    if (entry1->first_timestamp < entry2->first_timestamp) return -1;
>> +    if (entry1->first_timestamp > entry2->first_timestamp) return 1;
>> +    return 0;
>
> style-nit: if chain without {}s
>
>> +}
>> +
>> +static char *regex_extract_match(GRegex *regex, const char *message) {
>> +    g_autoptr(GMatchInfo) match_info = NULL;
>> +    int start_pos = 0, end_pos = 0;
>> +
>> +    g_return_val_if_fail(message != NULL, g_strdup("unknown"));
>> +
>> +    g_regex_match(regex, message, G_REGEX_MATCH_DEFAULT, &match_info);
>> +
>> +    if (!g_match_info_matches(match_info))
>> +        return g_strdup("unknown");
>
> style-nit: if without {}
>
>> +
>> +    g_match_info_fetch_pos(match_info, 1, &start_pos, &end_pos);
>> +
>> +    return g_strndup(message + start_pos, end_pos - start_pos);
>> +}
>> +
>> +static char *format_timestamp(uint64_t timestamp) {
>> +    g_autoptr(GDateTime) time = NULL;
>> +
>> +    if (timestamp == 0) {
>> +        return g_strdup ("-");
>
> style nit: GLib-style function invocation
>
>> +    }
>> +
>> +    time = g_date_time_new_from_unix_local_usec(timestamp);
>> +    return g_date_time_format(time, "%Y-%m-%d %H:%M");
>> +}
>> +
>> +static void print_boot(BootInfo *info, G_GNUC_UNUSED gpointer user_data) {
>> +    g_return_if_fail(info != NULL);
>> +
>> +    g_autofree char *start = format_timestamp(info->first_timestamp);
>> +    g_autofree char *end = format_timestamp(info->last_timestamp);
>> +    g_print("%s\t%s\t%s\t%s\n", start, end, info->boot_id, info->version);
>> +}
>> +
>> +static int list_boots_and_kernels(sd_journal *j) {
>> +    const void *data;
>> +    const size_t prefix_len = strlen("_BOOT_ID=");
>> +    size_t len;
>> +    int r;
>> +    g_autoptr(GPtrArray) boot_ids = NULL;
>> +    g_autoptr(GPtrArray) boot_array = NULL;
>> +    g_autoptr(GRegex) regex = NULL;
>> +    g_autoptr(GError) error = NULL;
>> +
>> +    boot_ids = g_ptr_array_new_with_free_func(g_free);
>> +
>> +    r = sd_journal_query_unique(j, "_BOOT_ID");
>> +    if (r < 0) {
>> +        g_printerr("Failed to query _BOOT_ID identifier: %s\n", strerror(-r));
>> +        return 1;
>> +    }
>> +    SD_JOURNAL_FOREACH_UNIQUE(j, data, len) {
>> +        g_ptr_array_add(boot_ids, g_strndup((const char *)data, len));
>> +    }
>> +
>> +    boot_array = g_ptr_array_new_full(boot_ids->len, (GDestroyNotify)boot_info_free);
>> +
>> +    sd_journal_flush_matches(j);
>> +
>> +    regex = g_regex_new("Linux version ([a-zA-Z0-9.-]+)", G_REGEX_OPTIMIZE, G_REGEX_MATCH_DEFAULT, &error);
>> +    if (error) {
>> +        g_critical("regex compilation failed: %s", error->message);
>> +        return 1;
>> +    }
>> +
>> +    for (size_t i = 0; i < boot_ids->len; i++) {
>> +        uint64_t first_ts = 0, last_ts = 0;
>> +        size_t msg_len;
>> +        const char *message = NULL;
>> +        const char *match = g_ptr_array_index(boot_ids, i);
>
> nit: maybe use a more telling name like `boot_id`?
>

We use boot_id just later. Will leave as is since we use it in add_match
bellow.

>> +        g_autofree char* version = NULL;
>
> style nit: placement of the * type specifier
>
>> +        g_autofree char *boot_id = NULL;
>> +
>> +        // Strip _BOOT_ID= from the message
>> +        boot_id = g_strndup(match + prefix_len, 32);
>> +
>> +        sd_journal_add_match(j, match, strlen(match));
>> +
>> +        r = sd_journal_seek_head(j);
>> +        if (r >= 0) {
>> +            r = sd_journal_next(j);
>> +            if (r > 0) {
>> +                sd_journal_get_realtime_usec(j, &first_ts);
>> +            }
>> +        }
>> +
>> +        r = sd_journal_get_data(j, "MESSAGE", (const void **)&message, &msg_len);
>> +        if (r >= 0 && message != NULL && msg_len > 0)
>> +            version = regex_extract_match(regex, message);
>> +        else
>> +            version = g_strdup("unknown");
>
> style-nit: if chain without {}s
>
>> +
>> +        r = sd_journal_seek_tail(j);
>> +        if (r >= 0) {
>> +            r = sd_journal_previous(j);
>> +            if (r > 0) {
>> +                sd_journal_get_realtime_usec(j, &last_ts);
>> +            }
>> +        }
>> +
>> +        sd_journal_flush_matches(j);
>> +
>> +        BootInfo *boot_info = g_new0 (BootInfo, 1);
>
> style nit: GLib-style function invocation
>
>> +        boot_info->boot_id = g_steal_pointer(&boot_id);
>> +        boot_info->version = g_steal_pointer(&version);
>> +        boot_info->first_timestamp = first_ts;
>> +        boot_info->last_timestamp = last_ts;
>> +
>> +        g_ptr_array_add(boot_array, boot_info);
>> +    }
>> +
>> +    g_ptr_array_sort_values(boot_array, (GCompareFunc)boot_info_cmp);
>> +    g_ptr_array_foreach(boot_array, (GFunc)print_boot, NULL);
>
> misses a sd_journal_close(j) at the end, though see comment at the end

Done, but on the outer scope since the outerscope opened the journal.

>> +
>> +    return 0;
>> +}
>> +
>>  // helper to print errors on stderr
>>  // if we're in json mode, print closing json body if possible
>>  static void print_error_and_exit(const char *fmt, ...) {
>> @@ -471,6 +612,7 @@ _Noreturn static void usage(char *error) {
>>          "  -J\t\t\tprint as json with one object of separate fields per entry\n"
>>          "  -I\t\t\twith -J, also emit a record listing the distinct syslog identifiers\n"
>>          "  -U\t\t\twith -J, also emit a record listing the distinct systemd units\n"
>> +        "  -B\t\t\tList boots. The columns are: the date of the first entry in the boot and last entry, the boot id, and the kernel version\n"
>
> This should at least state that it does ignore all the other parameters,
> so it essentially has a different functionality than the rest of the
> parameters.
>
>>          "  -h\t\t\tthis help\n"
>>          "\n"
>>          "Passing no range option will dump all the available journal\n"
>> @@ -711,11 +853,12 @@ int main(int argc, char *argv[]) {
>>      bool kernel = false;
>>      bool list_identifiers = false;
>>      bool list_units = false;
>> +    bool list_boots = false;
>>      int c;
>>  
>>      progname = argv[0];
>>  
>> -    while ((c = getopt(argc, argv, "b:e:d:n:f:t:p:i:u:jJIUkh")) != -1) {
>> +    while ((c = getopt(argc, argv, "b:e:d:n:f:t:p:i:u:jJIUkh:B")) != -1) {
>>          switch (c) {
>>          case 'b':
>>              begin = arg_to_timestamp_usec(optarg);
>> @@ -762,6 +905,9 @@ int main(int argc, char *argv[]) {
>>          case 'U':
>>              list_units = true;
>>              break;
>> +        case 'B':
>> +            list_boots = true;
>> +            break;
>>          case 'h':
>>              usage(NULL);
>>          case '?':
>> @@ -816,6 +962,10 @@ int main(int argc, char *argv[]) {
>>          return 1;
>>      }
>>  
>> +    if (list_boots) {
>> +        return list_boots_and_kernels(j);
>> +    }
>> +
>
> As this new flag does ignore the rest of the parameters (except -d), it
> might be nicer to factor out the rest of the functionality into a
> separate function to clearly show that there are essentially two
> different 'modes' that proxmox-mini-journalreader can operate in.
> Also we get the missing sd_journal_close() for free then by not
> returning straight away.
>
> On another note, a few of the parameters might be nice to actually
> acknowledge for listing the boots and kernel versions, e.g.
>
>    -n: limit to last number of boots
>    -{b,e}: begin and end where to print boots for
>    -{j,J}: print the output as JSON
>
> Though for the current usage -n would be the nicest so we don't have to
> use tail, but no hard feelings about this.
>

Implemented -n, -b, -e, -j.

>>      // restrict the traversal before seeking. Each filter is a separate group, conjoined (AND) with
>>      // the others; within a group matches are OR'd (same field, or an explicit disjunction for the
>>      // unit), so the groups must be split by a conjunction

-- 
Maximiliano