From owner-svn-src-stable@FreeBSD.ORG Tue Jan 17 06:54:29 2012 Return-Path: Delivered-To: svn-src-stable@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 3B1B6106564A; Tue, 17 Jan 2012 06:54:29 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from fallbackmx10.syd.optusnet.com.au (fallbackmx10.syd.optusnet.com.au [211.29.132.251]) by mx1.freebsd.org (Postfix) with ESMTP id C52B38FC12; Tue, 17 Jan 2012 06:54:28 +0000 (UTC) Received: from mail03.syd.optusnet.com.au (mail03.syd.optusnet.com.au [211.29.132.184]) by fallbackmx10.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id q0H5C24L029340; Tue, 17 Jan 2012 16:12:02 +1100 Received: from c211-30-171-136.carlnfd1.nsw.optusnet.com.au (c211-30-171-136.carlnfd1.nsw.optusnet.com.au [211.30.171.136]) by mail03.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id q0H5Bve1030887 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Tue, 17 Jan 2012 16:11:58 +1100 Date: Tue, 17 Jan 2012 16:11:57 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Andriy Gapon In-Reply-To: <4F143843.9020505@FreeBSD.org> Message-ID: <20120117150710.W1086@besplex.bde.org> References: <201201161440.q0GEeNYI038439@svn.freebsd.org> <4F143843.9020505@FreeBSD.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: Kevin Lo , svn-src-all@FreeBSD.org, src-committers@FreeBSD.org, svn-src-stable-8@FreeBSD.org, svn-src-stable@FreeBSD.org Subject: Re: svn commit: r230217 - stable/8/sys/kern X-BeenThere: svn-src-stable@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: SVN commit messages for all the -stable branches of the src tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 17 Jan 2012 06:54:29 -0000 On Mon, 16 Jan 2012, Andriy Gapon wrote: > on 16/01/2012 16:40 Kevin Lo said the following: >> Log: >> Fix build breakage by adding missing mb_put_padbyte() >> >> Modified: >> stable/8/sys/kern/subr_mchain.c >> >> Modified: stable/8/sys/kern/subr_mchain.c >> ============================================================================== >> --- stable/8/sys/kern/subr_mchain.c Mon Jan 16 14:31:01 2012 (r230216) >> +++ stable/8/sys/kern/subr_mchain.c Mon Jan 16 14:40:22 2012 (r230217) >> @@ -125,6 +125,21 @@ mb_reserve(struct mbchain *mbp, int size >> } >> >> int >> +mb_put_padbyte(struct mbchain *mbp) >> +{ >> + caddr_t dst; >> + char x = 0; >> + >> + dst = mtod(mbp->mb_cur, caddr_t) + mbp->mb_cur->m_len; >> + >> + /* only add padding if address is odd */ >> + if ((unsigned long)dst & 1) >> + return mb_put_mem(mbp, (caddr_t)&x, 1, MB_MSYSTEM); >> + else >> + return 0; > > Broken style above? No. Broken style in almost all of the above. Also a technical bug which give brokenness on some as yet unsupported arches: 1. Use of caddr_t for dst. caddr_t was endemic in mbuf code, but has been mostly eradicated there, except in a couple of APIs where it may be needed for backwards compatibility, and in all of subr_mchain.c. So here it is bug for bug compatible with nearby code. Here and probably mostly elsewhere, caddr_t is used mainly to do pointer arithmetic on it. It is assumed that caddr_t is precisely `char *', so that the pointer arithmetic increases it by 1 per increment. For that use, `char *' should be used directly. 2. Initialization in declaration for x. 3. Meaningless variable name for x. This bug is endemic in subr_mchain.c. This at first confused me into thinking that x was a dummy not-really used variable. It actually holds the padding byte. Generally in subr_mchain.c, for calls to mb_put_mem(), it holds the source data in a uintN_t variable. Here we should also use uint8_t for the variable and spell the size of the variable as sizeof(x), so that we look like other calls. mb_put_mem() has many style bugs too. It takes a caddr_t source address, but should take a `void *' source address. Its case statement has misindented labels... I had to look at it to see what x does. 4. Non-capitalized comment. 5. Non-terminated comment. The last 2 bugs are not endemic in subr_mchain.c, partly because subr_mchain.c has very few comments and even fewer comments for individual statements (only 8, including 1 for the copyright; no others are for individual statements; 1 is banal; all except the copyright have style bugs). 6. `unsigned long' is not abbreviated as u_long. 7. `unsigned long' is a logically and possibly physically wrong type to use here. We really want dst to be an integer so that we can mask it. But mtod() starts with a pointer and produces a pointer. We should have used it to produce a `char *', but we actually used it to produce a caddr_t. We already assumed that caddr_t is precisely `char *'. Now we only need to assume that it is a pointer. We want to convert it to an integer. For that, we should cast it to uintptr_t. Instead, we cast it to `unsigned long'. This assumes that either uintptr_t is unsigned long (as happens on all supported 64-bit arches) or that uintptr_t has the same size as unsigned long and the compiler doesn't warn about this type mismatch (as happens on all supported 32-bit arches). Perhaps we should actually use an alignment macro to do this, but I don't know of any suitable one. _ALIGN() is the main one, and it only rounds up. And its implementation has lots of style bugs (*). 8. Missing parentheses around return value of mb_put_mem(). This bug was never common in mbuf code, but it is endemic in subr_mchain.c 9. Missing parentheses around return value of 0. 10. Missing indentation of return of value of 0. These bugs were all copied from -current. The last one has been fixed, at least in -current. It is the smallest one with the smallest closure. Another 1000 commits to all branches should be enough to fix them all. Bruce