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>
index | next in thread | previous in thread | raw e-mail
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
help
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120117150710.W1086>