On Fri, Jan 23, 2026 at 2:21 AM Tomoaki AOKI <[email protected]> wrote:
> 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. > Phabricator at this point has no oversight (though there's some work to change that) and thousands of open PRs. It's impossible to find useful things to land or review for huge swaths of the tree. > And pull request on GitHub alone is also easily missed for anyone > without GitHub account like me. > True, but they are easily found in comparison to Phabricator. > 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 > non the switch. > So it's not a move from github. Github is there to allow us to accept more submissions in a way that has less friction and a higher likelihood of success than phabricator or bugzilla patches for new contributors to the src tree. It doesn't matter at all to the other workflows the project has setup that tend to be the main work flows. Those workflows are very hard for non-committers to use, hence the adding of github as an additional submission method. Moving from cgit to Forgejo is more fundamental. It moves the primary workflow of the project from a weird, bespoke, hodge-podge of tools and isolated use case solutions to an integrated workflow that committer and non-committer alike can access with an acceptable amount of friction. Warner
