Re: [PATCH v7 07/10] vsprintf: Consolidate handling of unknown pointer specifiers

From: Sergey Senozhatsky
Date: Thu Apr 18 2019 - 09:36:24 EST


On (04/17/19 13:53), Petr Mladek wrote:
> A reasonable compromise seems to be writing the unknown format specifier
> into the original string with a question mark, for example (%pC?).
> It should be self-explaining enough. Note that it is in brackets
> to follow the (null) style.

Hmm, seems that error string now sometimes try to `guess' what was the
error, but the guess can be misleading.

A very small example.
flags_string() can have a number of fmt specifiers - p, v, g.

switch (fmt[1]) {
case 'p':
flags = *(unsigned long *)flags_ptr;
/* Remove zone id */
flags &= (1UL << NR_PAGEFLAGS) - 1;
names = pageflag_names;
break;
case 'v':
flags = *(unsigned long *)flags_ptr;
names = vmaflag_names;
break;
case 'g':
flags = *(gfp_t *)flags_ptr;
names = gfpflag_names;
break;
default:
WARN_ONCE(1, "Unsupported flags modifier: %c\n", fmt[1]);
return buf;
}

The new error message, however, will hint '%pG', which may or may not
be helpful.

> -char *flags_string(char *buf, char *end, void *flags_ptr, const char *fmt)
> +char *flags_string(char *buf, char *end, void *flags_ptr,
> + struct printf_spec spec, const char *fmt)
> {
> unsigned long flags;
> const struct trace_print_flags *names;
> @@ -1760,8 +1767,7 @@ char *flags_string(char *buf, char *end, void *flags_ptr, const char *fmt)
> names = gfpflag_names;
> break;
> default:
> - WARN_ONCE(1, "Unsupported flags modifier: %c\n", fmt[1]);
> - return buf;
> + return string_nocheck(buf, end, "(%pG?)", spec);
> }

Wouldn't it be better to use fmt[1] instead?

-ss