svn commit: r230217 - stable/8/sys/kern

[Bruce Evans]

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