Date: Tue, 17 Jan 2012 16:11:57 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Andriy Gapon <avg@FreeBSD.org> Cc: Kevin Lo <kevlo@FreeBSD.org>, 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 Message-ID: <20120117150710.W1086@besplex.bde.org> In-Reply-To: <4F143843.9020505@FreeBSD.org> References: <201201161440.q0GEeNYI038439@svn.freebsd.org> <4F143843.9020505@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120117150710.W1086>