Re: [PATCH v5 02/21] lib/vsprintf: Print time and date in human readable format via %pt

[Petr Mladek <pmladek@suse.com>]

On Thu 2018-11-29 12:59:40, Andy Shevchenko wrote:
> There are users which print time and date represented by content of
> struct rtc_time in human readable format.
> 
> Instead of open coding that each time introduce %ptR[dt][r] specifier.
> 
> diff --git a/lib/test_printf.c b/lib/test_printf.c
> index 53527ea822b5..97b7d14961d6 100644
> --- a/lib/test_printf.c
> +++ b/lib/test_printf.c
> @@ -418,6 +418,11 @@ struct_va_format(void)
>  {
>  }
>  
> +static void __init
> +struct_rtc_time(void)
> +{
> +}

Just by chance, do you have any plans to add the test code? ;-)

I understand that you did now want to spend time on it before
the real change was accepted.


>  static void __init
>  struct_clk(void)
>  {
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 37a54a6dd594..b7114799b91f 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> +static noinline_for_stack
> +char *rtc_str(char *buf, char *end, const struct rtc_time *tm, const char *fmt)
> +{
> +	bool have_t = true, have_d = true;
> +	bool raw = false;
> +	int count = 2;
> +	bool found;
> +
> +	switch (fmt[count]) {
> +	case 'd':
> +		have_t = false;
> +		count++;
> +		break;
> +	case 't':
> +		have_d = false;
> +		count++;
> +		break;
> +	}
> +
> +	found = true;
> +	do {
> +		switch (fmt[count++]) {
> +		case 'r':
> +			raw = true;
> +			break;
> +		default:
> +			found = false;
> +			break;
> +		}
> +	} while (found);

I guess that the while cycle is remainder from an older version and
should not be here. It handles only the final 'r' now. 


> +	if (have_d)
> +		buf = date_str(buf, end, tm, raw);
> +	if (have_d && have_t) {
> +		/* Respect ISO 8601 */
> +		if (buf < end)
> +			*buf = 'T';

I checked several conversion patches and the original code did not use
the ISO format. The change makes sense (even though I personally
do not like the format much ;-)

Anyway, people might expect that the conversion is 1:1. The change
should get mentioned in the affected patches so that people are
not later surprised.


> +		buf++;
> +	}
> +	if (have_t)
> +		buf = time_str(buf, end, tm, raw);
> +
> +	return buf;
> +}
> +
> +static noinline_for_stack
> +char *timeanddate(char *buf, char *end, void *ptr, struct printf_spec spec,
> +		  const char *fmt)

Please, rename the function to time_and_date(). It is the style used
in this source file and it is much easier to read.

Otherwise, the patch looks fine. It helps to remove the many variants
of the code and unifies the output format.

Best Regards,
Petr