Date: Tue, 04 Oct 2016 20:48:14 -0700 From: Cy Schubert <Cy.Schubert@komquats.com> To: Kevin Lo <kevlo@FreeBSD.org> Cc: Gleb Smirnoff <glebius@FreeBSD.org>, 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 Message-ID: <201610050348.u953mErl056310@slippy.cwsent.com> In-Reply-To: Message from Kevin Lo <kevlo@FreeBSD.org> of "Wed, 05 Oct 2016 11:20:58 %2B0800." <20161005032057.GA74690@ns.kevlo.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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 <Cy.Schubert@cschubert.com>
FreeBSD UNIX: <cy@FreeBSD.org> Web: http://www.FreeBSD.org
The need of the many outweighs the greed of the few.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201610050348.u953mErl056310>