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

[Vegard Nossum]

2009/3/5 Frederic Weisbecker <fweisbec@xxxxxxxxx>:
> 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/