From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id A80821FF0E6 for ; Fri, 07 Aug 2026 16:01:56 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 22F802154A; Fri, 07 Aug 2026 16:01:56 +0200 (CEST) From: Maximiliano Sandoval To: "Daniel Kral" Subject: Re: [PATCH proxmox-mini-journalreader v3 1/2] add -B parameter to lists boots In-Reply-To: (Daniel Kral's message of "Fri, 07 Aug 2026 13:21:24 +0200") References: <20260806140955.479029-1-m.sandoval@proxmox.com> <20260806140955.479029-2-m.sandoval@proxmox.com> User-Agent: mu4e 1.12.9; emacs 30.1 Date: Fri, 07 Aug 2026 16:01:50 +0200 Message-ID: MIME-Version: 1.0 Content-Type: text/plain X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1786111302476 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.222 Adjusted score from AWL reputation of From: address BITCOIN_OBFU_SUBJ 1 Bitcoin + obfuscated subject DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) PDS_BTC_ID 0.499 FP reduced Bitcoin ID PDS_BTC_MSGID 0.032 Bitcoin ID with T_MSGID_NOFQDN2 RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: ICZJ26K4EFVTEMQJ3XYJGECYTFDOAW56 X-Message-ID-Hash: ICZJ26K4EFVTEMQJ3XYJGECYTFDOAW56 X-MailFrom: m.sandoval@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: pve-devel@lists.proxmox.com X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: Replies below of what was addressed in v4. "Daniel Kral" 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 >> --- >> >> 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 >> -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 >> #include >> +#include >> #include >> #include >> #include >> @@ -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