svn commit: r245222 - head/sys/sys

Tue Jan 15 13:56:20 UTC 2013

  Bruce,

On Tue, Jan 15, 2013 at 11:13:36PM +1100, Bruce Evans wrote:
B> > can you please look at attached patch, that fixes problems you
B> > describe? It
B> >
B> > - fixes mentioned style bugs in param.h
B> > - adds int casts to MHLEN and MLEN
B> > - removes extra casts from (void *) to (struct mbuf *)
B> > - removes extra declarations of inlined functions
B> > - uninlines and moves m_get2() and m_getjcl() to uipc_mbuf.c
B> > - size argument of m_get2() swicthed back to int
B> 
B> Looks mostly good.
B> 
B> % ...
B> % Index: sys/mbuf.h
B> % ===================================================================
B> % --- sys/mbuf.h	(revision 245450)
B> % +++ sys/mbuf.h	(working copy)
B> % @@ -52,11 +52,14 @@
B> %   * stored.  Additionally, it is possible to allocate a separate buffer
B> %   * externally and attach it to the mbuf in a way similar to that of mbuf
B> %   * clusters.
B> % + *
B> % + * MLEN is data length in a normal mbuf.
B> % + * MHLEN is data length in an mbuf with pktheader.
B> % + * MINCLSIZE is a smallest amount of data that should be put into cluster.
B> %   */
B> 
B> The comment needs indent protection if you want to preserve the line
B> structure of the new comments.  I don't see any better way to format these
B> lines as bullet points.  A separate paragraph for each would be too verbose.
B> 
B> % -#define	MLEN		(MSIZE - sizeof(struct m_hdr))	/* normal data len */
B> % -#define	MHLEN		(MLEN - sizeof(struct pkthdr))	/* data len w/pkthdr */
B> % -#define	MINCLSIZE	(MHLEN + 1)	/* smallest amount to put in cluster */
B> % -#define	M_MAXCOMPRESS	(MHLEN / 2)	/* max amount to copy for compression */
B> % +#define	MLEN		((int)(MSIZE - sizeof(struct m_hdr)))
B> % +#define	MHLEN		((int)(MLEN - sizeof(struct pkthdr)))
B> % +#define	MINCLSIZE	(MHLEN + 1)
B> 
B> These never needed to be sorted non-alphabetically.

Sorting alphabetically moves MLEN to the bottom, but the above macros
are defined via MLEN. IMO, it is more easy to read when we move from
most simple macro to ones that are derived from it, not in alphabetical
order.

B> I hope this doesn't cause any problems.  MHLEN, etc., are used just a few
B> times outside of mbuf.h where we can't control this, mostly in specialized
B> code.  One interesting use in non-specialized code is in tcp_output():
B> 
B>  		if (len <= MHLEN - hdrlen - max_linkhdr) {
B> 
B> Here len has type long, MHLEN has type size_t (before this change) and type 
B> int (after this change), hdrlen has type unsigned (misspelled as that instead
B> of u_int), and max_linkhdr has type int.  So the type combinations are
B> nonsense, and there is a good chance of getting different compiler warnings
B> about this before and after the change.  Having just one unsigned type in
B> the mix tends to promote everything to unsigned, except on 64-bit systems
B> with one u_int but no size_t, the long has highest rank so everything
B> gets promoted to long.

I think here we can do:

-	unsigned ipoptlen, optlen, hdrlen;
+	int ipoptlen, optlen, hdrlen;

These three take their values from:
- ip6_optlen()
- m_len (from mbuf header)
- tcp_addoptions()
... , which all are ints.
- sizeof(), which can be casted.

I left the ipsec_optlen unsigned. Its type originates from
ipsec_hdrsiz_internal(), which returns result of sizeof().

Patch attached.

This leads to (len <= MHLEN - hdrlen - max_linkhdr) comparing
long and (int - int - int). This also fixes many other places in
tcp_output, where values od ipoptlen, optlen, hdrlen are assigned
to int variables or supplied as int arguments. 

B> % ...
B> % @@ -393,23 +396,10 @@
B> %  extern uma_zone_t	zone_jumbo16;
B> %  extern uma_zone_t	zone_ext_refcnt;
B> % 
B> % -static __inline struct mbuf	*m_getcl(int how, short type, int flags);
B> % ...
B> % -static __inline void		*m_cljget(struct mbuf *m, int how, int size);
B> % -static __inline void		 m_chtype(struct mbuf *m, short new_type);
B> % -void				 mb_free_ext(struct mbuf *);
B> % -static __inline struct mbuf	*m_last(struct mbuf *m);
B> % -int				 m_pkthdr_init(struct mbuf *m, int how);
B> % +struct mbuf	*m_get2(int how, short type, int flags, int size);
B> % +struct mbuf	*m_getjcl(int how, short type, int flags, int size);
B> % +void		 mb_free_ext(struct mbuf *);
B> 
B> This one is still missing a parameter name, unlike all (?) the others.
B> 
B> % +int		 m_pkthdr_init(struct mbuf *m, int how);
B> 
B> The 2 new non-inline prototypes should be moved to the general section
B> for non-inline prototypes.  The 2 old ones must remain early since
B> they are used in the inlines.
B> 
B> After moving the 2 new ones, also remove their parameter names to match
B> the (worse) nearby style.  This leaves only 1 nearby other for mb_free_ext()
B> to be mismatched with.
B> 
B> The general prototype section has a fairly uniform style, with the only
B> obvious bugs being:
B> - m_copyup() has parameter names
B> - m_sanity() is misindented by 1 space
B> - m_unshare() has 1 parameter name but 2 parameters.

Applied, patch attached.

-- 
Totus tuus, Glebius.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: tcp_output.diff
Type: text/x-diff
Size: 726 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/svn-src-all/attachments/20130115/e08be1e2/attachment.diff>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: mbuf.diff
Type: text/x-diff
Size: 8872 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/svn-src-all/attachments/20130115/e08be1e2/attachment-0001.diff>