On Fri, 23 Jan 2026 09:40:17 +0100 Andre Albsmeier <[email protected]> wrote:
> On Thu, 22-Jan-2026 at 08:31:15 -0700, Warner Losh wrote: > > On Thu, Jan 22, 2026 at 1:46 AM Andre Albsmeier <[email protected]> wrote: > > > > > On Fri, 19-Dec-2025 at 20:37:41 +0000, Jamie Landeg-Jones wrote: > > > > void <[email protected]> wrote: > > > > > > > > >... > > > > > and so dmsg has things like > > > > > > > > > > [1353919] swp_pager_getswapspace(32): failed > > > > > > > > > > the [1353919] I guess being the timestamp. > > > > > > > > > > ... > > > > > > > > > Yes! You can get the value from sysctl kern.boottime. > > > > > > > > Here is a small program I wrote a while back, just call it > > > > as a dmesg substitute. > > > > ... > > > > Cheers, Jamie > > > > > > > > #!/bin/sh -efu > > > > set -efu > > > > > > > > boottime="$(/sbin/sysctl -n kern.boottime | /usr/local/bin/gawk '{printf > > > "%d", gensub ("^.* sec = ([1-9][0-9]*), .*$", "\\1", 1)}')" > > > > > > > > [ -z "$(printf '%s' "$boottime" | /usr/bin/egrep '^0$|^[1-9][0-9]*$')" ] > > > && { printf 'Invalid boottime retrieved.\n' >& 2; exit 1; } > > > > > > > > /sbin/dmesg "$@" | /usr/local/bin/gawk -v boottime="$boottime" ' > > > > > > > > { > > > > uptime = gensub ("^\\[([1-9][0-9]*)\\] .*$", "\\1", 1) > > > > if (uptime == $0) realtime = "??? ?? ??:??;??" > > > > else realtime = strftime ("%b %d %T", uptime + boottime) > > > > > > > > print realtime " " $0 > > > > }' > > > > > > FWIW, this is a patch for dmesg(1) I use. You can enable timestamp > > > conversion using -t and optionally specify a different output format > > > using -f "fmt". > > > > > > If interested for inclusion in base, I could try to re-format it > > > and come up with a manpage diff... > > > > > > > Yes. I've done a quick review inline. > > Thanks... > > > > --- sbin/dmesg/dmesg.c 2023-12-27 17:49:57.000000000 +0100 > > > +++ sbin/dmesg/dmesg.c 2026-01-22 09:20:28.987020000 +0100 > > > @@ -60,6 +60,7 @@ > > > #include <unistd.h> > > > #include <vis.h> > > > #include <sys/syslog.h> > > > +#include <time.h> > > > > > > static struct nlist nl[] = { > > > #define X_MSGBUF 0 > > > @@ -76,18 +77,23 @@ > > > main(int argc, char *argv[]) > > > { > > > struct msgbuf *bufp, cur; > > > + struct timeval boottime; > > > + char timebuf[64]; > > > > > > > Why 64? > > Well, any recommendation? Is the a _MAX define somewhere? > I always use 64 but if there is something better I'll use it. > > > > > > > > char *bp, *ep, *memf, *nextp, *nlistf, *p, *q, *visbp; > > > + const char *timefmt = "%d %b %T"; > > > kvm_t *kd; > > > + time_t time; > > > size_t buflen, bufpos; > > > long pri; > > > - int ch, clear; > > > + int ch, clear, timeconv; > > > > > > > timeconv should be a bool. > > OK... > > > > > > > > bool all; > > > > > > all = false; > > > clear = false; > > > + timeconv = false; > > > (void) setlocale(LC_CTYPE, ""); > > > memf = nlistf = NULL; > > > - while ((ch = getopt(argc, argv, "acM:N:")) != -1) > > > + while ((ch = getopt(argc, argv, "actf:M:N:")) != -1) > > > switch(ch) { > > > case 'a': > > > all = true; > > > @@ -95,6 +101,12 @@ > > > case 'c': > > > clear = true; > > > break; > > > + case 't': > > > + timeconv = true; > > > + break; > > > + case 'f': > > > + timefmt = optarg; > > > + break; > > > case 'M': > > > memf = optarg; > > > break; > > > @@ -109,6 +121,13 @@ > > > if (argc != 0) > > > usage(); > > > > > > + if( timeconv ) { > > > > > > > So the spacing doesn't meet style(9). "if (timeconv) {" is the preferred > > spacing. > > Yes, that's why I offered to re-format it... > > > > > > > > + int mib[6] = { CTL_KERN, KERN_BOOTTIME }; > > > + size_t l = sizeof boottime; > > > > > > > sizeof(boottime) is the preferred spelling here. > > > > > > > + if (sysctl(mib, 2, &boottime, &l, 0, 0) < 0) > > > + err(1, "sysctl kern.boottime"); > > > + } > > > + > > > if (memf == NULL) { > > > /* > > > * Running kernel. Use sysctl. This gives an unwrapped > > > buffer > > > @@ -200,7 +219,22 @@ > > > } > > > > > > (void)strvisx(visbp, p, nextp - p, 0); > > > - (void)printf("%s", visbp); > > > + bufpos = strspn( visbp+1, "0123456789" ) + 1; > > > > > > > No check for overflow here. *bufpos may point to a NUL and +1 will point > > it past that NULL. > > bufpos is no pointer but size_t. visbp[ bufpos ] will never point past > '\0' as I start at visbp + 1 for the strspn(). > > But the test of visbp[ bufpos+1 ] below is not OK. I'll fix that... > > > It shouldn't, but it might. And you should do something more like > > > > if (!timeconv) { > > <the existing printf>; > > } else { > > decode the buffer > > } > > I din't want to duplicate <the existing printf> too often in case > timeconv is desired but fails later for whatever reasons. So would > is the recommendation here? > > 1. Whenever it fails, do <the existing printf> and continue? > > 2. Place <the existing printf> at the end and "goto" it? > > 3. ?? > > > > > But you should check things in the right order so you can't overflow. Your > > code already has a fallback for printing things... > > > > *ALSO* The parsing is wrong. When show_timestamp=2 it will have a > > fractional part (so .xxxx) that should be parsed as well. This also throws > > off the math that just adds to the second of time timeval. > > Right, the conversion gets aborted in this case (I never used > show_timestamp=2 ;-)). I'll have a look at it... > > > > > > + if( timeconv != true > > > + || bufpos < 2 > > > + || visbp[0] != '[' > > > + || visbp[bufpos] != ']' > > > + || visbp[bufpos+1] != ' ' ) { > > > + (void)printf("%s", visbp); > > > + continue; > > > + } > > > + > > > + visbp[bufpos] = '\0'; > > > + time = boottime.tv_sec + strtoull( visbp+1, NULL, 10 ); > > > + if( strftime( timebuf, sizeof timebuf, timefmt, localtime( > > > &time ) ) != 0 ) > > > > > > > Same spacing. comment about parens. And note that we're strongly > > encouraging {} even for single line statements. > > OK... > > > ... > > So that's my feedback on the first round of patches. You should do a github > > pull request > > or a Phabricator review. Do not put this in a bugzilla bug. Bugzilla is not > > a code review tool > > and I suspect that feedback on the revision will be needed (if nothing else > > than for the man > > page that we've not seen yet). github is likely easiest for you, and I'll > > keep an eye out there. > > Phabricator is a lot more work to get started. If you are going to do a > > bunch of reviews, it > > may make sense to climb the hill (or for other parts of the code whose > > maintainers don't do > > github). If it's just this one, and you have a github account, I'd > > recommend that as the path > > I used Phabricator once (https://reviews.freebsd.org/D46313) but I think > I'll follow your advice and try the github way... > > -Andre I think opening review on Phabricator alone is easily missed. So I basically file a PR on Bugzilla, open review and introduce the D number of the review on Bugzilla PR. And pull request on GitHub alone is also easily missed for anyone without GitHub account like me. Upcoming (hopefully) moving from GitHub to Forgejo managed by FreeBSD project could change the situation, but it's still not official. I'm thinking about how my workflow should be on the switch. -- Tomoaki AOKI <[email protected]>
