linux-kernel - Re: [PATCH 5/5 v3] vsprintf: unify the format decoding layer for its 3 users

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <19f34abd0903042353i58c8e72eh33be515bf09a36f@mail.gmail.com>
Date:	Thu, 5 Mar 2009 08:53:14 +0100
From:	Vegard Nossum <vegard.nossum@...il.com>
To:	Frederic Weisbecker <fweisbec@...il.com>
Cc:	Ingo Molnar <mingo@...e.hu>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Lai Jiangshan <laijs@...fujitsu.com>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Steven Rostedt <rostedt@...dmis.org>,
	Peter Zijlstra <peterz@...radead.org>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 5/5 v3] vsprintf: unify the format decoding layer for its 
	3 users

2009/3/5 Frederic Weisbecker <fweisbec@...il.com>:
> An new optimization is making its way to ftrace. Its purpose is to
> make ftrace_printk() consuming less memory and be faster.
>
> Written by Lai Jiangshan, the approach is to delay the formatting
> job from tracing time to output time.
> Currently, a call to ftrace_printk will format the whole string and
> insert it into the ring buffer. Then you can read it on /debug/tracing/trace
> file.
>
> The new implementation stores the address of the format string and
> the binary parameters into the ring buffer, making the packet more compact
> and faster to insert.
> Later, when the user exports the traces, the format string is retrieved
> with the binary parameters and the formatting job is eventually done.
>
> The new implementation rewrites a lot of format decoding bits from
> vsnprintf() function, making now 3 differents functions to maintain
> in their duplicated parts of printf format decoding bits.
>
> Suggested by Ingo Molnar, this patch tries to factorize the most
> possible common bits from these functions.
> The real common part between them is the format decoding. Although
> they do somewhat similar jobs, their way to export or import the parameters
> is very different. Thus, only the decoding layer is extracted, unless you see
> other parts that could be worth factorized.
>

Just one small nit from me :-)

> @@ -726,18 +961,9 @@ static char *pointer(const char *fmt, char *buf, char *end, void *ptr, int field
>  int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
>  {
>        unsigned long long num;
> -       int base;
>        char *str, *end, c;
> -
> -       int flags;              /* flags to number() */
> -
> -       int field_width;        /* width of output field */
> -       int precision;          /* min. # of digits for integers; max
> -                                  number of chars for from string */
> -       int qualifier;          /* 'h', 'l', or 'L' for integer fields */
> -                               /* 'z' support added 23/7/1999 S.H.    */
> -                               /* 'z' changed to 'Z' --davidm 1/25/99 */
> -                               /* 't' added for ptrdiff_t */
> +       int read;
> +       struct printf_spec spec = {0};
>
>        /* Reject out-of-range values early.  Large positive sizes are
>           used for unknown buffer sizes. */
> @@ -758,184 +984,144 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
>                size = end - buf;
>        }
>
> -       for (; *fmt ; ++fmt) {
> -               if (*fmt != '%') {
> -                       if (str < end)
> -                               *str = *fmt;
> -                       ++str;
> -                       continue;
> -               }
> +       while (*fmt) {
> +               char *old_fmt = (char *)fmt;

This looks a bit strange. We can't ever change the format string, so
it doesn't make sense to try to use it as a non-const pointer anyway.
Why not make the variable "const char *" and drop the cast?

The same for another case.


Vegard

-- 
"The animistic metaphor of the bug that maliciously sneaked in while
the programmer was not looking is intellectually dishonest as it
disguises that the error is the programmer's own creation."
	-- E. W. Dijkstra, EWD1036
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/