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.

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson