From owner-freebsd-hackers@FreeBSD.ORG Fri Nov 13 15:37:12 2009 Return-Path: Delivered-To: hackers@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 88E361065679 for ; Fri, 13 Nov 2009 15:37:12 +0000 (UTC) (envelope-from jilles@stack.nl) Received: from mx1.stack.nl (relay02.stack.nl [IPv6:2001:610:1108:5010::104]) by mx1.freebsd.org (Postfix) with ESMTP id 4B28A8FC1B for ; Fri, 13 Nov 2009 15:37:12 +0000 (UTC) Received: from turtle.stack.nl (turtle.stack.nl [IPv6:2001:610:1108:5010::132]) by mx1.stack.nl (Postfix) with ESMTP id 10EFB35A82B; Fri, 13 Nov 2009 16:37:11 +0100 (CET) Received: by turtle.stack.nl (Postfix, from userid 1677) id EDB7E33C92; Fri, 13 Nov 2009 16:37:10 +0100 (CET) Date: Fri, 13 Nov 2009 16:37:10 +0100 From: Jilles Tjoelker To: Garrett Wollman Message-ID: <20091113153710.GA99645@stack.nl> References: <19196.60473.337121.565916@hergotha.csail.mit.edu> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <19196.60473.337121.565916@hergotha.csail.mit.edu> User-Agent: Mutt/1.5.18 (2008-05-17) Cc: hackers@freebsd.org Subject: Re: CFR: Exceedingly minor fixes to libc X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 13 Nov 2009 15:37:12 -0000 On Fri, Nov 13, 2009 at 12:18:49AM -0500, Garrett Wollman wrote: > If you have a moment, please take a look at the following patch. It > contains some very minor fixes to various parts of libc which were > found by the clang static analyzer. They fall into a few categories: > 1) Bug fixes in very rare situations (mostly error-handling code that > has probably never been executed). > 2) Dead store elimination. > 3) Elimination of unused variables. (Or in a few cases, making use of > them.) > Some minor style problems were also fixed in the vicinity. There > should be no functional changes except in very unusual conditions. This looks mostly ok, with a few exceptions. > [...] > Index: gen/getpwent.c > =================================================================== > --- gen/getpwent.c (revision 199242) > +++ gen/getpwent.c (working copy) > @@ -257,22 +257,19 @@ > [...] > @@ -1876,7 +1871,6 @@ > syslog(LOG_ERR, > "getpwent memory allocation failure"); > *errnop = ENOMEM; > - rv = NS_UNAVAIL; > break; > } > st->compat = COMPAT_MODE_NAME; I think this change hides the wrongness in the handling of malloc errors while leaving it as broken as it is. > [...] > Index: net/getservent.c > =================================================================== > --- net/getservent.c (revision 199242) > +++ net/getservent.c (working copy) > @@ -476,11 +476,11 @@ > struct nis_state *st; > int rv; > > - enum nss_lookup_type how; > char *name; > char *proto; > int port; > > + enum nss_lookup_type how; > struct servent *serv; > char *buffer; > size_t bufsize; > @@ -491,7 +491,8 @@ > > name = NULL; > proto = NULL; > - how = (enum nss_lookup_type)mdata; > + > + how = (intptr_t)mdata; > switch (how) { > case nss_lt_name: > name = va_arg(ap, char *); In what way is this an improvement? > Index: posix1e/acl_delete_entry.c > =================================================================== > --- posix1e/acl_delete_entry.c (revision 199242) > +++ posix1e/acl_delete_entry.c (working copy) > @@ -89,20 +89,20 @@ > return (-1); > } > > - if ((acl->ats_acl.acl_cnt < 1) || > - (acl->ats_acl.acl_cnt > ACL_MAX_ENTRIES)) { > + if ((acl_int->acl_cnt < 1) || > + (acl_int->acl_cnt > ACL_MAX_ENTRIES)) { > errno = EINVAL; > return (-1); > } If you are changing this code anyway, consider fixing a style bug (extraneous parentheses) here. > - for (i = 0; i < acl->ats_acl.acl_cnt;) { > - if (_entry_matches(&(acl->ats_acl.acl_entry[i]), entry_d)) { > + for (i = 0; i < acl_int->acl_cnt;) { > + if (_entry_matches(&(acl_int->acl_entry[i]), entry_d)) { > /* ...shift the remaining entries... */ > - for (j = i; j < acl->ats_acl.acl_cnt - 1; ++j) > - acl->ats_acl.acl_entry[j] = > - acl->ats_acl.acl_entry[j+1]; > + for (j = i; j < acl_int->acl_cnt - 1; ++j) > + acl_int->acl_entry[j] = > + acl_int->acl_entry[j+1]; > /* ...drop the count and zero the unused entry... */ > - acl->ats_acl.acl_cnt--; > - bzero(&acl->ats_acl.acl_entry[j], > + acl_int->acl_cnt--; > + bzero(&acl_int->acl_entry[j], > sizeof(struct acl_entry)); > acl->ats_cur_entry = 0; > -- Jilles Tjoelker