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

[Stefano Brivio <sbrivio@redhat.com>]

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?

The rest of the series looks good to me, so I can also fix this up
on merge, as you prefer.

-- 
Stefano