From owner-freebsd-net@FreeBSD.ORG Sun Nov 5 10:40:47 2006 Return-Path: X-Original-To: freebsd-net@FreeBSD.org Delivered-To: freebsd-net@FreeBSD.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id D921716A512 for ; Sun, 5 Nov 2006 10:40:47 +0000 (UTC) (envelope-from bde@zeta.org.au) Received: from mailout1.pacific.net.au (mailout1-3.pacific.net.au [61.8.2.210]) by mx1.FreeBSD.org (Postfix) with ESMTP id E705043DCF for ; Sun, 5 Nov 2006 10:40:30 +0000 (GMT) (envelope-from bde@zeta.org.au) Received: from mailproxy1.pacific.net.au (mailproxy1.pacific.net.au [61.8.2.162]) by mailout1.pacific.net.au (Postfix) with ESMTP id 1546D24D1B4; Sun, 5 Nov 2006 21:40:29 +1100 (EST) Received: from katana.zip.com.au (katana.zip.com.au [61.8.7.246]) by mailproxy1.pacific.net.au (Postfix) with ESMTP id 51C7B8C07; Sun, 5 Nov 2006 21:40:27 +1100 (EST) Date: Sun, 5 Nov 2006 21:40:26 +1100 (EST) From: Bruce Evans X-X-Sender: bde@delplex.bde.org To: Brooks Davis In-Reply-To: <20061105010936.GA6669@lor.one-eyed-alien.net> Message-ID: <20061105202925.D44623@delplex.bde.org> References: <20061102142543.GC70915@lor.one-eyed-alien.net> <200611031220.53791.max@love2party.net> <20061105010936.GA6669@lor.one-eyed-alien.net> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: Max Laier , MQ , freebsd-net@FreeBSD.org Subject: Re: Reentrant problem with inet_ntoa in the kernel X-BeenThere: freebsd-net@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Networking and TCP/IP with FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 05 Nov 2006 10:40:48 -0000 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 : >>> 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