Re: [PATCH v2 2/4] log: Correct formatting of timestamps

[Stefano Brivio <sbrivio@redhat.com>]

On Wed, 7 Aug 2024 11:06:52 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Tue, Aug 06, 2024 at 09:08:07PM +0200, Stefano Brivio wrote:
> > On Tue,  6 Aug 2024 16:18:37 +1000
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >   
> > > logtime_fmt_and_arg() is a rather odd macro, producing both a format
> > > string and an argument, which can only be used in quite specific printf()
> > > like formulations.  It also has a significant bug: it tries to display 4
> > > digits after the decimal point (so down to tenths of milliseconds) using
> > > %04i.  But the field width in printf() is always a *minimum* not maximum
> > > field width, so this will not truncate the given value, but will redisplay
> > > the entire tenth-of-milliseconds difference again after the decimal point.  
> > 
> > Weird, not for me (glibc 2.38). But anyway, yes, it's much better this
> > way, and definitely, it's specified as minimum width (I never noticed).
> > 
> > Just one nit:
> >   
> > > Replace the macro with an snprintf() like function which will format the
> > > timestamp, and use an explicit % to correct the display.
> > > 
> > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > ---
> > >  log.c | 36 ++++++++++++++++++++++++------------
> > >  1 file changed, 24 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/log.c b/log.c
> > > index eb3a780a..e60852f7 100644
> > > --- a/log.c
> > > +++ b/log.c
> > > @@ -46,14 +46,24 @@ int		log_trace;		/* --trace mode enabled */
> > >  bool		log_conf_parsed;	/* Logging options already parsed */
> > >  bool		log_runtime;		/* Daemonised, or ready in foreground */
> > >  
> > > +#define LL_STRLEN	(sizeof("-9223372036854775808"))
> > > +#define LOGTIME_STRLEN	(LL_STRLEN + 5)
> > > +
> > >  /**
> > > - * logtime_fmt_and_arg() - Build format and arguments to print relative log time
> > > - * @x:		Current timestamp
> > > + * logtime_fmt() - Format timestamp into a string for the log
> > > + * @buf:	Buffer into which to format the time
> > > + * @size:	Size of @buf
> > > + * @ts:		Time to format
> > > + *
> > > + * Return: number of characters written to @buf (excluding \0)
> > >   */
> > > -#define logtime_fmt_and_arg(x)						\
> > > -	"%lli.%04lli",							\
> > > -	(timespec_diff_us((x), &log_start) / 1000000LL),		\
> > > -	(timespec_diff_us((x), &log_start) / 100LL)
> > > +int logtime_fmt(char *buf, size_t size, const struct timespec *ts)  
> > 
> > Shouldn't this be static?  
> 
> Yes, yes it should.
> 
> > The rest of the series looks good to me, so I can also fix this up
> > on merge, as you prefer.  
> 
> If you can fix on merge that would be great.

Done. Series applied.

-- 
Stefano