From owner-svn-src-head@freebsd.org Wed Oct 5 03:48:41 2016 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 9FC62AF4606; Wed, 5 Oct 2016 03:48:41 +0000 (UTC) (envelope-from cy.schubert@komquats.com) Received: from smtp-out-no.shaw.ca (smtp-out-no.shaw.ca [64.59.134.13]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client CN "Client", Issuer "CA" (not verified)) by mx1.freebsd.org (Postfix) with ESMTPS id 4A6B9929; Wed, 5 Oct 2016 03:48:40 +0000 (UTC) (envelope-from cy.schubert@komquats.com) Received: from spqr.komquats.com ([96.50.22.10]) by shaw.ca with SMTP id rdC3bR4Y4fI0ardC4bqWca; Tue, 04 Oct 2016 21:48:33 -0600 X-Authority-Analysis: v=2.2 cv=JOx5iICb c=1 sm=1 tr=0 a=jvE2nwUzI0ECrNeyr98KWA==:117 a=jvE2nwUzI0ECrNeyr98KWA==:17 a=L9H7d07YOLsA:10 a=9cW_t1CCXrUA:10 a=s5jvgZ67dGcA:10 a=CH0kA5CcgfcA:10 a=SAfkavq5AAAA:8 a=6I5d2MoRAAAA:8 a=YxBL1-UpAAAA:8 a=OHVipD7GlL4V5qlsAQ8A:9 a=CjuIK1q_8ugA:10 a=QZZ0i9YZlR7GoCpEi5UP:22 a=IjZwj45LgO3ly-622nXo:22 a=Ia-lj3WSrqcvXOmTRaiG:22 Received: from slippy.cwsent.com (slippy8 [10.2.2.6]) by spqr.komquats.com (Postfix) with ESMTPS id 0E4A8E7; Tue, 4 Oct 2016 20:48:30 -0700 (PDT) Received: from slippy (localhost [127.0.0.1]) by slippy.cwsent.com (8.15.2/8.15.2) with ESMTP id u953mErl056310; Tue, 4 Oct 2016 20:48:15 -0700 (PDT) (envelope-from Cy.Schubert@cschubert.com) Message-Id: <201610050348.u953mErl056310@slippy.cwsent.com> X-Mailer: exmh version 2.8.0 04/21/2012 with nmh-1.6 Reply-to: Cy Schubert From: Cy Schubert X-os: FreeBSD X-Sender: cy@cwsent.com X-URL: http://www.cschubert.com/ To: Kevin Lo cc: Gleb Smirnoff , src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r305824 - in head/sys: contrib/ipfilter/netinet kern net netinet netinet6 netipsec sys In-Reply-To: Message from Kevin Lo of "Wed, 05 Oct 2016 11:20:58 +0800." <20161005032057.GA74690@ns.kevlo.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Date: Tue, 04 Oct 2016 20:48:14 -0700 X-CMAE-Envelope: MS4wfJxJ3iZ/MplaFe+fMC9LLnrotxY707bt0td3Ra8VhVewGK88bbyzJEG4PADlPiLpGw+tXuQRIC02pliddlsV2oYBdhJmZuzBhaJZoY4/ANEAaNCzfMpW flVVSN6uEK8U8J4Ik1Q+MrHEPyKnPbKOf6lw0ljhxNwMZBtxqnkc7dfn1kBcQBx+QrqltD6yjVcqrJP0Slz9VMjDU5gxWJkRBYQ8UXhLBmOs0X4WO7uy64D6 R9ussjT6osjeXCK9ZPIq31A1zlYRy/Wtl32OBuG0CmtACgViSJRW67jTZJWRhg0SIBackAz2NIUu0FfLneEVcQ== X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 05 Oct 2016 03:48:41 -0000 In message <20161005032057.GA74690@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 FreeBSD UNIX: Web: http://www.FreeBSD.org The need of the many outweighs the greed of the few.