From owner-svn-src-all@FreeBSD.ORG Tue Apr 21 07:13:32 2015 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 9722B20C; Tue, 21 Apr 2015 07:13:32 +0000 (UTC) Received: from mail105.syd.optusnet.com.au (mail105.syd.optusnet.com.au [211.29.132.249]) by mx1.freebsd.org (Postfix) with ESMTP id 41BE91CD7; Tue, 21 Apr 2015 07:13:31 +0000 (UTC) Received: from c211-30-166-197.carlnfd1.nsw.optusnet.com.au (c211-30-166-197.carlnfd1.nsw.optusnet.com.au [211.30.166.197]) by mail105.syd.optusnet.com.au (Postfix) with ESMTPS id 126DB1045EA0; Tue, 21 Apr 2015 16:48:36 +1000 (AEST) Date: Tue, 21 Apr 2015 16:48:35 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Eric van Gyzen cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r281787 - head/sbin/dmesg In-Reply-To: <201504202007.t3KK7dtj000379@svn.freebsd.org> Message-ID: <20150421151131.T1396@besplex.bde.org> References: <201504202007.t3KK7dtj000379@svn.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.1 cv=A5NVYcmG c=1 sm=1 tr=0 a=KA6XNC2GZCFrdESI5ZmdjQ==:117 a=PO7r1zJSAAAA:8 a=kj9zAlcOel0A:10 a=JzwRw_2MAAAA:8 a=_cJNZYTe81WAuVWcR9kA:9 a=CjuIK1q_8ugA:10 X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 21 Apr 2015 07:13:32 -0000 On Mon, 20 Apr 2015, Eric van Gyzen wrote: > Log: > dmesg: accommodate message buffer growth between the sysctl calls > > Allocate 12.5% extra space to avoid ENOMEM when the message buffer > is growing steadily. This is bogus allocation. The message buffer has a small fixed size so that it fits in physical memory in the kernel, and rarely changes its size after it fills up. Userland can easily statically allocate a buffer 1000 if not 1000000 times as large as the full size in virtual memory. This is much easier to do than even half-baked dynamic allocation. > Modified: head/sbin/dmesg/dmesg.c > ============================================================================== > --- head/sbin/dmesg/dmesg.c Mon Apr 20 20:06:25 2015 (r281786) > +++ head/sbin/dmesg/dmesg.c Mon Apr 20 20:07:39 2015 (r281787) > @@ -118,6 +118,9 @@ main(int argc, char *argv[]) > */ > if (sysctlbyname("kern.msgbuf", NULL, &buflen, NULL, 0) == -1) > err(1, "sysctl kern.msgbuf"); > + /* Allocate extra room for growth between the sysctl calls. */ > + buflen += buflen/8; > + /* Allocate more than sysctl sees, for room to append \n\0. */ > if ((bp = malloc(buflen + 2)) == NULL) > errx(1, "malloc failed"); > if (sysctlbyname("kern.msgbuf", bp, &buflen, NULL, 0) == -1) This still fails if the size grows more than 12.5%. This adds some style bugs: - missing spaces around binary operator - verboseness. There is already a verbose comment about the method just above here - new verbose comments not made less verbose by not adding newline delimiters for the block (a single statement) of code that they describe, unlike some old single-line comments in the file. - (old style bug). Expansion of the verbose comment above gave a worse comment scope obfuscation. Code to adjust the buffer actually needs to be in a different block, but the comment about this code is appended to the main comment and the adjustment is not even appended. Non-half-baked dynamic initialization would check the error returned by the first sysctl, and if it is ENOMEM, then retry. This is actually simpler, since it doesn't need the second sysctl (except dynamically, and almost never then). Allocate some memory before starting the loop. This doesn't need any comments since it is obviously the only correct way to determine the amount of memory needed if the size can change. Dynamic allocation is still overkill. For the initial size, use MSG_BUFSIZE (+2) from . MSG_BUFSIZE is not quite unusable outside of the kernel. It is just a default, so cannot be used correctly outside of the kernel, but given that it exists, it is slightly better than an arbitrary value for the initial size, since that gives only 1 iteration of the loop in the usual case where dmesg was compiled with the same sources as the kernel and the user didn't change the default. Untested version: X buflen = MSGBUFSIZ / 2; X bp = NULL; X do { X buflen *= 2; X if ((bp = reallocf(bp, buflen + 2)) == NULL) X errx(1, "malloc failed"); X if (sysctlbyname("kern.msgbuf", bp, &buflen, NULL, 0) == 0) X break; X if (errno != ENOMEM) X err(1, "sysctl kern.msgbuf"); X } while (0); The error handling is still painful. malloc() failure can't happen, but I preserved the check for it. buflen *= 2 can overflow, but I omitted checking for it. This should be safe since the overflow only malloc() failures (that can't happen) have happened. phk has railed against the practice of starting with a small size and doubling it. Starting with a large size and hoping to never change it is better for bloatware. The above does the latter, except the "large" size is small. Doubling it is correct since anyone changing the default shouldn't make small adjustments. But small utilities don't need any dynamic allocation. They can allocate "large" buffers that are actually small using static allocation. Another not so good way to do the above is have a sysctl that returns the full msgbuf size. This cannot change. Then the only change needed in the original code is to change the first sysctl to determine the full size. All kernel parameters that are not constant because they may be changed by kernel options should have such a sysctl and shouldn't have any manifest constant (except as a default or minimum value, like POSIX minimum values for sysconf()). But using such a constant here just takes more code. It takes 1 sysctl() to determine the size, then a malloc(), then 1 sysctl() (but not in a loop) to use the size. Fully dynamical allocation starting with a reasonably large size usually only needs 1 malloc() and 1 sysctl(). Elsewhere, in much more important places, there is an enormous amount of sloppy code that abuses POSIX manifest constants like PATH_MAX instead of technically correct dynamic code using sysconf(), since static allocation is usually good enough and dynamic allocation is so painful. E.g., static allocation using PATH_MAX is: char path[PATH_MAX]; /* technically broken */ char path2[MAXPATHLEN]; /* honestly unportable */ and using sysconf() it is: long len = sysconf(_SC_PATH_MAX); /* Several lines to check for and handle errors in sysconf(). */ /* Several lines to check for and handle len being unrepresentable * size_t. */ path = malloc(len); /* Several lines to check for and handle errors in malloc(). */ and using PATH_MAX and sysconf() it is: #ifdef PATH_MAX char path[PATH_MAX]; /* assume comp. detects if too large #else /* Above code for dynamic case. */ #endif Bruce