svn commit: r245222 - head/sys/sys

[Bruce Evans]

On Wed, 9 Jan 2013, Gleb Smirnoff wrote:

> On Wed, Jan 09, 2013 at 09:09:09AM +0000, Hans Petter Selasky wrote:
> H> Log:
> H>   Fix compile warning when using GCC:
> H>   Comparison between signed and unsigned.

        o Add 3 style bugs:
 	 2 sets of excessive parentheses
 	 1 line longer than 80 columns

> H> Modified: head/sys/sys/mbuf.h
> H> ==============================================================================
> H> --- head/sys/sys/mbuf.h	Wed Jan  9 08:52:44 2013	(r245221)
> H> +++ head/sys/sys/mbuf.h	Wed Jan  9 09:09:09 2013	(r245222)
> H> @@ -557,7 +557,7 @@ m_get2(int how, short type, int flags, i
> H>  	args.flags = flags;
> H>  	args.type = type;
> H>
> H> -	if (size <= MHLEN || (size <= MLEN && (flags & M_PKTHDR) == 0))
> H> +	if (size <= ((int)MHLEN) || (size <= ((int)MLEN) && (flags & M_PKTHDR) == 0))
> H>  		return ((struct mbuf *)(uma_zalloc_arg(zone_mbuf, &args, how)));
> H>  	if (size <= MCLBYTES)
> H>  		return ((struct mbuf *)(uma_zalloc_arg(zone_pack, &args, how)));
>
> The function is new and present only in head. I suggest just change its
> argument to u_int or size_t, instead of casting.

Or fix the definitions of MHLEN and MLEN so that they have type int.
They use sizeof() internally, and arguably suffer from the unsign
extension given by this.  Parameters giving small integral values
should be plain ints if possible (to minimize surprises from sign
extension), and basic parameters for mbufs are, but derived ones aren't:

% sys/param.h:#ifndef	MSIZE
% sys/param.h:#define MSIZE		256		/* size of an mbuf */
% sys/param.h:#endif	/* MSIZE */

MSIZE has type int (unless it is already misdefined with a different type).

Its definition has many style bugs:
- space instead of tab after #define
- the comment on its #endif shouldn't exist since its #ifdef section is
   short and simple.
- tab instead of space before comment on #endif
- backwards comment on #endif.

% sys/param.h:#define MCLBYTES	(1 << MCLSHIFT)	/* size of an mbuf cluster */

MCLBYTES has type int (it has the type of 1, irrespective of the type of
MCLSHIFT).

% sys/mbuf.h:#define	MLEN		(MSIZE - sizeof(struct m_hdr))	/* normal data len */
% sys/mbuf.h:#define	MHLEN		(MLEN - sizeof(struct pkthdr))	/* data len w/pkthdr */
% sys/mbuf.h:#define	MINCLSIZE	(MHLEN + 1)	/* smallest amount to put in cluster */
% sys/mbuf.h:#define	M_MAXCOMPRESS	(MHLEN / 2)	/* max amount to copy for compression */

These use sizeof() internally, so they suffer from unsign extension.  Their
type is hard to predict in general< thanks to C's broken "value preserving"
extension rules.  But on non-exotic machines, size_t has a higher rank
than it, so the common type of operands of type int and size_t is size_t
and the result has type size_t.

Re-quoting some of the above:

> H> +	if (size <= ((int)MHLEN) || (size <= ((int)MLEN) && (flags & M_PKTHDR) == 0))

If MHLEN and MLEN are converted to int in their declarations, then the result
is the same here, and all uses of these macros might be affected.

This is theoretically better than casting size_t to int or passing in a
size_t for size, since on the exotic machines the type of these macros
is into so it would be mismatched with the type of 'size' in the opposite
way.

> The function is new and present only in head. I suggest just change its
> argument to u_int or size_t, instead of casting.

That might be better, but mbuf APIs consistently use int for size args now:

% static __inline struct mbuf	*m_get2(int how, short type, int flags,
% 				    int size);
% ...
% static __inline struct mbuf	*m_getjcl(int how, short type, int flags,
% 				    int size);
% ...
% static __inline int		 m_init(struct mbuf *m, uma_zone_t zone,
% 				    int size, int how, short type, int flags);
% ...
% static __inline void		*m_cljget(struct mbuf *m, int how, int size);

Inlines in sys/mbuf.h are generally badly written.  These forward
declarations of inlines are just useless.  They aren't even sorted.

% 
% static __inline int
% m_gettype(int size)
% ...
% static __inline uma_zone_t
% m_getzone(int size)
% ...
% static __inline struct mbuf *
% m_get2(int how, short type, int flags, int size)
% {
% 	struct mb_args args;
% 	struct mbuf *m, *n;
% 	uma_zone_t zone;
% 
% 	args.flags = flags;
% 	args.type = type;
% 
% 	if (size <= MHLEN || (size <= MLEN && (flags & M_PKTHDR) == 0))
% 		return ((struct mbuf *)(uma_zalloc_arg(zone_mbuf, &args, how)));

Inlines with large code are only effective if the args are often constant
so that most of the code is optimized away.  But in all (?) 1 (?) call to
m_get2(), the size arg is variable.  The other args are constant, but most
of the tests are of the size arg.

Style bugs here and later: uma_zalloc_arg() returns void *, so casting it
to struct mbuf * is just verbose.

% 	if (size <= MCLBYTES)
% 		return ((struct mbuf *)(uma_zalloc_arg(zone_pack, &args, how)));
%

Style bug: extra blank line.

% 	if (size > MJUM16BYTES)
% 		return (NULL);

Technical problem: MJUM16BYTES has type plain it, so if you change the type
of (size' to size_t, then you should get a warning for this comparison.

m_cljset() has a local variable 'int size'.

The most fundamental size variable in the mbuf API is probably mh_len.
It has type plain int.  It must not be replaced by size_t size that
would just waste space on arches where size_t is larger than int.  It
could be replaced by u_int, but I prefer plain int.  ext_size has type
u_int.  tso_segsz has type uint16_t.

The types of size variables and macros would have to be constently changed
to either size_t or int to avoid these warnings.  This is like const
poisoning.  In particular, MHLEN etc. must be cast to size_t in case
the machine is exotic so that the their expression has type int.

It is surprising that there aren't more of these warnings already.
Repeating some context again:

> H> -	if (size <= MHLEN || (size <= MLEN && (flags & M_PKTHDR) == 0))
> H> +	if (size <= ((int)MHLEN) || (size <= ((int)MLEN) && (flags & M_PKTHDR) == 0))
> H>  		return ((struct mbuf *)(uma_zalloc_arg(zone_mbuf, &args, how)));
> H>  	if (size <= MCLBYTES)
> H>  		return ((struct mbuf *)(uma_zalloc_arg(zone_pack, &args, how)));

How did this fix work without also casting MCLBYTES?  It has the same type
as MHLEN and MLEN.  Naybe the compiler is smart about ranges -- after
passing the first test it can know than size >= 0, at least after you
changed the first test to make it filter out all cases with size < 0.

But later in m_getjcl(), there is the same test (size <= MCLBYTES) with
no preceding tests (except possibly in callers) to filter out cases where
size < 0, so I would expect a warning from this too.

Otherwise, mbuf.h doesn't do many comparisons that should trigger
type mismatch warnings.  It does many switch (size)'s where 'size' has
type int and the case statements have a mixture of plain ints and
size_t's for their types.  Now I think binary conversion does the
right thing -- everything the case values get converted to match the
type of the case selector, so all the size_t's get converted to int,
and this is the right thing all the size_t's actually fit in an int.

Bruce