svn commit: r305824 - in head/sys: contrib/ipfilter/netinet kern net netinet netinet6 netipsec sys

[Cy Schubert]

In message <20161005032057.GA74690 at ns.kevlo.org>, Kevin Lo writes:
> On Tue, Oct 04, 2016 at 12:09:20PM -0700, Gleb Smirnoff wrote:
> > 
> >   Kevin,
> 
> Hi Gleb,
> 
> > On Thu, Sep 15, 2016 at 07:41:48AM +0000, Kevin Lo wrote:
> > K> Log:
> > K>   Remove the 4.3BSD compatible macro m_copy(), use m_copym() instead.
> > ...
> > K> Modified: head/sys/contrib/ipfilter/netinet/fil.c
> > K> ========================================================================
> ======
> > K> --- head/sys/contrib/ipfilter/netinet/fil.c	Thu Sep 15 02:48:56 201
> 6	(r305823)
> > K> +++ head/sys/contrib/ipfilter/netinet/fil.c	Thu Sep 15 07:41:48 201
> 6	(r305824)
> > K> @@ -3226,7 +3226,7 @@ filterdone:
> > K>  		fdp = fin->fin_dif;
> > K>  		if ((fdp != NULL) && (fdp->fd_ptr != NULL) &&
> > K>  		    (fdp->fd_ptr != (void *)-1)) {
> > K> -			mc = M_COPY(fin->fin_m);
> > K> +			mc = M_COPYM(fin->fin_m);
> > K>  			if (mc != NULL)
> > K>  				ipf_fastroute(mc, &mc, fin, fdp);
> > K>  		}
> > K> 
> > K> Modified: head/sys/contrib/ipfilter/netinet/ip_compat.h
> > K> ========================================================================
> ======
> > K> --- head/sys/contrib/ipfilter/netinet/ip_compat.h	Thu Sep 15 02:4
> 8:56 2016	(r305823)
> > K> +++ head/sys/contrib/ipfilter/netinet/ip_compat.h	Thu Sep 15 07:4
> 1:48 2016	(r305824)
> > K> @@ -211,7 +211,7 @@ struct  ether_addr {
> > K>  #  define	MSGDSIZE(m)	mbufchainlen(m)
> > K>  #  define	M_LEN(m)	(m)->m_len
> > K>  #  define	M_ADJ(m,x)	m_adj(m, x)
> > K> -#  define	M_COPY(x)	m_copy((x), 0, M_COPYALL)
> > K> +#  define	M_COPYM(x)	m_copym((x), 0, M_COPYALL, M_NOWAIT)
> > K>  #  define	M_DUP(m)	m_dup(m, M_NOWAIT)
> > K>  #  define	IPF_PANIC(x,y)	if (x) { printf y; panic("ipf_panic"); 
> }
> > K>  typedef struct mbuf mb_t;
> > K> @@ -366,7 +366,7 @@ typedef	struct	mb_s	{
> > K>  # define	MSGDSIZE(m)	msgdsize(m)
> > K>  # define	M_LEN(m)	(m)->mb_len
> > K>  # define	M_ADJ(m,x)	(m)->mb_len += x
> > K> -# define	M_COPY(m)	dupmbt(m)
> > K> +# define	M_COPYM(m)	dupmbt(m)
> > K>  # define	M_DUP(m)	dupmbt(m)
> > K>  # define	GETKTIME(x)	gettimeofday((struct timeval *)(x), NUL
> L)
> > K>  # define	MTOD(m, t)	((t)(m)->mb_data)
> > 
> > IMHO, for contributed ipfilter we should only modify ip_compat.h and ip_fil
> _freebsd.c.
> > In case of removal of m_copy() the macro should remain named M_COPY(), but 
> it should be
> > defined to call to function of m_copym(). So fil.c can be left unmodified, 
> and ip_compat.h
> > will have only 1 line change. The userland part of ip_compat.h which define
> s M_COPY() to
> > dupmbt() doesn't need to be renamed as well.
> 
> Actually your comments were addressed in my original patch, but rwatson@
> pointed out that switching M_COPY() to M_COPYM() for consistency:
> https://reviews.freebsd.org/D7878#163304

There is no really easy answer to this. Yes, it's nice, even important, to 
have consistency across the entire tree. On the flip side, keeping the 
number of non-functional changes to contributed software to a minimum aids 
merge of future releases. There are advantages and disadvantages to each. 
One could even go so far as to modify contributed software to comply with 
FreeBSD style(9) standards (extreme example). However, merge of new 
releases would constitute a POLA violation to future developers merging new 
releases into the tree. I think leaving as vanilla as possible aids in 
future maintenance and upgrades. My vote would be that the macro remain 
named M_COPY().

The downside of this is that someone not well versed with a certain contrib 
code might be astonished at the use of a different macro. There are pluses 
and minuses to either approach but I think that the merge argument may be a 
stronger argument for M_COPY() remaining in contrib code.

It's not inconceivable that mis-merges can be and are the cause of some 
interesting bugs.

That's my $0.02.


-- 
Cheers,
Cy Schubert <Cy.Schubert at cschubert.com>
FreeBSD UNIX:  <cy at FreeBSD.org>   Web:  http://www.FreeBSD.org

	The need of the many outweighs the greed of the few.