Date: Sun, 5 Nov 2006 21:40:26 +1100 (EST) From: Bruce Evans <bde@zeta.org.au> To: Brooks Davis <brooks@one-eyed-alien.net> Cc: Max Laier <max@love2party.net>, MQ <antinvidia@gmail.com>, freebsd-net@FreeBSD.org Subject: Re: Reentrant problem with inet_ntoa in the kernel Message-ID: <20061105202925.D44623@delplex.bde.org> In-Reply-To: <20061105010936.GA6669@lor.one-eyed-alien.net> References: <be0088ce0611020026y4fe07749pd5a984f8744769b@mail.gmail.com> <20061102142543.GC70915@lor.one-eyed-alien.net> <be0088ce0611030146u5e97e08cmbd36e94d772c8a94@mail.gmail.com> <200611031220.53791.max@love2party.net> <be0088ce0611031901g100a8e29k2e9728a8806cf385@mail.gmail.com> <20061105010936.GA6669@lor.one-eyed-alien.net>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sat, 4 Nov 2006, Brooks Davis wrote:
> On Sat, Nov 04, 2006 at 03:01:00AM +0000, MQ wrote:
>> 2006/11/3, Max Laier <max@love2party.net>:
>>> On Friday 03 November 2006 10:46, MQ wrote:
>>>> By the way, implementing a printf/log which understands ipv4 address is
>>>> tedious, perhaps.
It certainly is.
>>> Actually, it's a ~15 line patch, I will see if I can get to it later
>>> today.
More like ~15 files:
libkern/inet_ntoa.c:
-current: remove
RELENG_6: keep for compatibility
RELENG_old: no change (but complicates merges)
conf/files: remove reference in -current only
kern/subr_prof.c: implement in -current and RELENG_6
man9/printf.9: document in -current and RELENG_6
gcc/c-format.c: implement format checking in -current and RELENG_6. Maintain
this forever.
gcc/gcc.1: in RELENG_6 fix old bug -- the -fformat-extensions options,
which is needed to actually use the new %? and the old %[bD], has never
been documented. Maintain this in RELENG_6.
gcc/doc/gcc.1: in -current, merge with the fixed gcc/gcc.1. While here,
also merge FreeBSD extensions which were actully documented. Maintain
this forever.
gcc/doc/*.texi: document the new %?. While here, document the old
-fformat-extensions and %[bD]. Maintain this forever.
>>> If we reuse the number buffer we can even get away without
>>> increase in stack space usage. Whether or not this is a good idea is on
>>> another page,
It's OK for a specialized function, but bad for printf.
>>> but %I and %lI (for IPv6) would be available and we already
>>> have %D and %b as special cases in the kernel.
%I should be reserved for integer operands with the size of the operand
given as a parameter.
>> I know for sure that the kernel stack is much smaller than that in
>> userland programms, but is the kernel stack too small to contain
>> another 32 bytes at most? I've no idea, and is there any way to trace
>> the usage of the kernel stack? KGDB?
32 isn't much, but many multiples of 32 would be ugly at best. The
problem is that gcc will allocate 16 bytes for the buffer whether or
not the error-handling code that actually uses the buffer is executed,
and whether or not the uses of the buffer are nested (in -current, the
uses can't be nested since normal interrupt handlers are in separate
threads). It is enough to have local declarations of the buffer in
nested function calls.
Speaking of ugly allocations, the kernel has some stupid code like:
{
int table[] == { 1, 2, ..., };
use(table);
}
where the table is const (but not declared const). Such code not only
wastes stack, but causes gcc to initialize the table from static storage
on every call to the function. (IIRC, gcc is not very smart about
optimizing away either the stack use or the copying, even if the table
is declared const.) The above wouldn't even have compiled in K&R C.
The kernel also has too many uses of the C99 feature variable-sized
auto arrays. I looked at a few of these and they were mostly like
"int vmstuff[n];" where we have to know that the variable parameter n
is less than about 16 to know that the stack won't explode, and we do
know that at compile time but then we can write the above more safely
and clearly, though less quickly, as
"int vmstuff[16];" [CTASSERT(MAX_n <= 16;]".
>> The reason why I don't like the idea that adding the facility into
>> printf is we already had the converter inet_ntoa, adding the facility
>> seems duplicated. But if implemented, and use a local buffer, this
>> should be a really good way to solve the reentrant problem.
The kernel inet_ntoa() would be removed if a printf format for it
was implemented.
> Given that except for a few cases that should have obviously used
> inet_ntoa_r, inet_ntoa is only used to feed printf and log, the right
> answer is clearly to implement it in printf and log and the nuke this
> fundamental broken API.
I think you misspelled "wrong" [answer]. Specialized formats don't
belong in printf().
printf()'s behaviour of not using any storage in the unbuffered case is
often convenient, but hardly anyone seems to worry about the huge (;-))
wastage of storage caused by using sprintf() or better yet, strcat(),
when the output needs to be adjusted a bit before printing it.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20061105202925.D44623>