Date: Fri, 17 May 2013 11:38:13 +0400 From: Gleb Smirnoff <glebius@FreeBSD.org> To: Julian Elischer <julian@FreeBSD.org> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r250700 - in head/sys: conf net netinet6 sys Message-ID: <20130517073813.GY17164@FreeBSD.org> In-Reply-To: <20130517073721.GX17164@FreeBSD.org> References: <201305161620.r4GGKIFI012296@svn.freebsd.org> <20130517073721.GX17164@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Sorry, I am slowpoke. Everything had already been noticed by
other reviewers.
On Fri, May 17, 2013 at 11:37:21AM +0400, Gleb Smirnoff wrote:
T> On Thu, May 16, 2013 at 04:20:18PM +0000, Julian Elischer wrote:
T> J> Modified: head/sys/sys/mbuf.h
T> J> ==============================================================================
T> J> --- head/sys/sys/mbuf.h Thu May 16 15:28:38 2013 (r250699)
T> J> +++ head/sys/sys/mbuf.h Thu May 16 16:20:17 2013 (r250700)
T> J> @@ -129,6 +129,8 @@ struct pkthdr {
T> J> u_int16_t vt_vtag; /* Ethernet 802.1p+q vlan tag */
T> J> u_int16_t vt_nrecs; /* # of IGMPv3 records in this chain */
T> J> } PH_vt;
T> J> + u_int16_t fibnum; /* this packet should use this fib */
T> J> + u_int16_t pad2; /* align to 32 bits */
T> J> SLIST_HEAD(packet_tags, m_tag) tags; /* list of packet tags */
T> J> };
T> J> #define ether_vtag PH_vt.vt_vtag
T> J> @@ -171,6 +173,7 @@ struct mbuf {
T> J> #define m_type m_hdr.mh_type
T> J> #define m_flags m_hdr.mh_flags
T> J> #define m_nextpkt m_hdr.mh_nextpkt
T> J> +#define m_fibnum m_hdr.mh_nextpkt
T> J> #define m_act m_nextpkt
T> J> #define m_pkthdr M_dat.MH.MH_pkthdr
T> J> #define m_ext M_dat.MH.MH_dat.MH_ext
T>
T> The define definitely looks incorrect.
T>
T> J> +static int inline
T> J> +rt_m_getfib(struct mbuf *m)
T> J> +{
T> J> + KASSERT(m->m_flags & M_EXT , ("attempt to set FIB on non header mbuf"));
T> J> + return (m->m_pkthdr.fibnum);
T> J> +}
T>
T> The KASSERT looks incorrect. Text doesn't match the test. IMO, text is
T> correct, but the test is not.
T>
T> J> #define M_SETFIB(_m, _fib) do { \
T> J> - _m->m_flags &= ~M_FIB; \
T> J> - _m->m_flags |= (((_fib) << M_FIBSHIFT) & M_FIB); \
T> J> + KASSERT((_m)->m_flags & M_EXT, ("No FIB on non header mbuf")); \
T> J> + ((_m)->m_pkthdr.fibnum) = (_fib); \
T> J> } while (0)
T>
T> Same for this KASSERT.
T>
T> IMO, any change to struct mbuf should be announced and posted for review
T> prior to committing.
T>
T> --
T> Totus tuus, Glebius.
--
Totus tuus, Glebius.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20130517073813.GY17164>