From owner-svn-src-head@FreeBSD.ORG Sun Mar 14 11:26:16 2010 Return-Path: Delivered-To: svn-src-head@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 09833106564A; Sun, 14 Mar 2010 11:26:16 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail06.syd.optusnet.com.au (mail06.syd.optusnet.com.au [211.29.132.187]) by mx1.freebsd.org (Postfix) with ESMTP id 8F0408FC08; Sun, 14 Mar 2010 11:26:15 +0000 (UTC) Received: from c122-107-126-6.carlnfd1.nsw.optusnet.com.au (c122-107-126-6.carlnfd1.nsw.optusnet.com.au [122.107.126.6]) by mail06.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id o2EBQBKk007189 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Sun, 14 Mar 2010 22:26:12 +1100 Date: Sun, 14 Mar 2010 22:26:11 +1100 (EST) From: Bruce Evans X-X-Sender: bde@delplex.bde.org To: Garrett Cooper In-Reply-To: <7d6fde3d1003131313l3559b6dbt1566fb23be150408@mail.gmail.com> Message-ID: <20100314212000.N23495@delplex.bde.org> References: <201003131108.o2DB8vmu001454@svn.freebsd.org> <7d6fde3d1003131313l3559b6dbt1566fb23be150408@mail.gmail.com> MIME-Version: 1.0 Content-Type: MULTIPART/MIXED; BOUNDARY="0-2088975880-1268565971=:23495" Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org, Bruce Cran Subject: Re: svn commit: r205118 - head/sbin/sysctl X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 14 Mar 2010 11:26:16 -0000 This message is in MIME format. The first part should be readable text, while the remaining parts are likely unreadable without MIME-aware tools. --0-2088975880-1268565971=:23495 Content-Type: TEXT/PLAIN; charset=X-UNKNOWN; format=flowed Content-Transfer-Encoding: QUOTED-PRINTABLE On Sat, 13 Mar 2010, Garrett Cooper wrote: >> Log: >> =A0Free the memory allocated via strdup. Why bother? 1. Memory is infinite :-). 2. Memory is infinite relative to sysctl's requirements, especially for this strdup. Normally this strdup is never reached. It is normally reached a whole 1 time for sysctl -a (e.g., on ref9-amd64 now), since there is a whole 1 sysctl with a timeval format in the configured kernel, and not many more (if any more) than 1 in /sys. >> Modified: head/sbin/sysctl/sysctl.c >> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D >> --- head/sbin/sysctl/sysctl.c =A0 Sat Mar 13 11:06:47 2010 =A0 =A0 =A0 = =A0(r205117) >> +++ head/sbin/sysctl/sysctl.c =A0 Sat Mar 13 11:08:57 2010 =A0 =A0 =A0 = =A0(r205118) >> @@ -382,6 +382,7 @@ S_timeval(int l2, void *p) >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (*p2 =3D=3D '\n') >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0*p2 =3D '\0'; >> =A0 =A0 =A0 =A0fputs(p1, stdout); >> + =A0 =A0 =A0 free(p1); >> =A0 =A0 =A0 =A0return (0); >> =A0} The \xa0's are ugly. > strdup(3) can fail in that section of code, thus the fputs(3) > could segfault: Who cares? 1. strdup() can't fail, since memory is infinite :-) 2. ... since memory is infinite relative to sysctl's requirements (see abov= e). 3. Most systems are (mis)configured with MALLOC_OPTIONS=3DA..., so if strdu= p() fails it doesn't return, and all the careful code that checks malloc() and strdup() for failing becomes just an obfuscation. MALLOC_OPTIONS=3DA... seriously breaks programs that are likely to run out of memory and handle this, but such programs became rare when address spaces exceeded 64K and rarer when they exceeded 640K...; they are now infinite :-), so problems are rarely seen. The strdup() is bogus anyway. ctime() returns a non-const string so the the string should be modified directly. It is just as easy to avoid the modification. The function with this has lots more style bugs: % static int % S_timeval(int l2, void *p) Style bug: *p should be const. % { % =09struct timeval *tv =3D (struct timeval*)p; Style bug: initialization in declaration. % =09time_t tv_sec; % =09char *p1, *p2; %=20 % =09if (l2 !=3D sizeof(*tv)) { % =09=09warnx("S_timeval %d !=3D %zu", l2, sizeof(*tv)); % =09=09return (1); % =09} % =09printf(hflag ? "{ sec =3D %'jd, usec =3D %'ld } " : % =09=09"{ sec =3D %jd, usec =3D %ld } ", % =09=09(intmax_t)tv->tv_sec, tv->tv_usec); Style bugs: non-KNF indentation of continued lines. % =09tv_sec =3D tv->tv_sec; % =09p1 =3D strdup(ctime(&tv_sec)); Style bug: strdup() just adds bloat (especially when you add memory management and error handling for it). % =09for (p2=3Dp1; *p2 ; p2++) Style bug: missing spaces around binary operator. % =09=09if (*p2 =3D=3D '\n') % =09=09=09*p2 =3D '\0'; To avoid the modification, break here and print the (p2 - p1) characters starting at p1 using "%.*s" format. This leads to complete de-bloatation: 1: the newline is always at the end, so it can be located using strlen() 2: strlen() is unnecessary to, since the string always has length 25. Thus "%.*s", ..., [p2 - p1 OR strlen(p1) -1] reduces to "%.24s". % =09fputs(p1, stdout); There is also a style rule against using fputs()... % =09free(p1); So the above lines (here they are again, indented a bit, with some error handling added for not-quite maximal verboseness): =09% =09p1 =3D strdup(ctime(&tv_sec)); =09%=09if (p1 =3D=3D NULL) =09%=09=09err(1, "strdup"); =09% =09for (p2=3Dp1; *p2 ; p2++) =09% =09=09if (*p2 =3D=3D '\n') =09% =09=09=09*p2 =3D '\0'; =09% =09fputs(p1, stdout); =09% =09free(p1); are a verbose way of writing: =09printf("%.24s", ctime(&tv_sec)); To reduce verboseness a little more, merge this printf() with the preceding one. To increase verboseness for both, add error handling for fputs() and printf= (). % =09return (0); % } --0-2088975880-1268565971=:23495--