Re: kern.msgbuf_show_timestamp=1

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