BPF extensions

[Jeffrey Hsu]

if (bpf_enabled(ifp))
	bpf_ptap(ifp->if_bpf, m, eh, ETHER_HDR_LEN);
+void
+bpfattach2(struct ifnet *ifp, u_int dlt, u_int hdrlen, struct bpf_if 
**driverp)

Same here.  Use a more descriptive name than attach2.


I've used bpfattach_dlt, that should perfectly reflect what the
function is about.
Great.

The patch (http://leaf.dragonflybsd.org/~joerg/bpf.diff) is updated
to reflect this change, the new comments for bpf_[m]_tap* and the
macros. The /* XXX */ comment has been replaced by the spl protection,
it's needed.
More comments?


Joerg Sonnenberger wrote:
On Mon, Jan 24, 2005 at 12:19:19PM -0800, Jeffrey Hsu wrote:

-       u_int hdr = dst->sa_family;
+       uint32_t hdr = dst->sa_family;
sa_family_t is defined to be a uint8_t, so either use uint8_t or 
sa_family_t.


This is intentional, because we have to prepend a 32 bit value.
That's why it can't just take the address of dst->sa_family.
The only possible problem would be byte order, but that's currently
completely ignored, so this doesn't change the status quo.
Despite what the preceding comment in the code says, it seems to me what
you really have to prepend a natural word size, not necessarily 32-bits.
So 'int' seems like the correct data type.
Plus, there's the following bcopy() of ICHDRLEN, which is defined
as 'sizeof(u_int)', so at the very least, you'd have to change that
to to 'sizeof(uint32_t)' to match.
+ * Incoming linkage from device drivers, when packet is in
+ * an mbuf chain and to be prepended by a contiguous header.
+ */
+void
+bpf_mtap2(struct bpf_if *bp, const void *data, u_int dlen, struct mbuf *m)
Please name this something more descriptive than mtap2.  Perhaps
something like bpf_mtap_packet().


I took the naming from FreeBSD. bpf_mtap_packet is fine with me though.
After sleeping on it, I think 'bpf_ptap()' is best.

Also, the comment is slightly unclear:
1. packets are always in a mbuf chain.
2. define "incoming linkage" or use more descriptive wording


The comment is equal to bpf_[m]tap, so the reasoning applies to both.

For bpf_mtap:
/*
 * Process the package in the mbuf chain m.  The packet is parsed
 * by each listener's filter and if accepted, stashed into the
 * corresponding buffer.
 */
For bpf_mtap2:
 * Process the package in the mbuf chain m with the header in data
 * prepended.  The package ...
For bpf_tap:
 * Process the package pkt of length pktlen. The package ...
Can you construct the dummy mbuf header bpf_ptap() and then call bpf_mtap()?
The comment for bpf_ptap() would then describe its true function,
which is to construct a dummy mbuf header for a packet before
calling bpf_mtap().
+#define BPF_MTAP_BPF(_bp, _m) do {                             \
+       if (_bp != NULL)                                        \
+               bpf_mtap((_bp), (_m));                          \
+} while (0)
+#define        BPF_MTAP(_ifp,_m) BPF_MTAP_BPF((_ifp)->if_bpf, (_m))
+#define        BPF_MTAP2(_ifp,_data,_dlen,_m) do {                     \
+       if ((_ifp)->if_bpf)                                     \
+               bpf_mtap2((_ifp)->if_bpf,(_data),(_dlen),(_m)); \
-       /* Check for a BPF tap */
-       if (ifp->if_bpf != NULL) {
-               struct m_hdr mh;
-
-               /* This kludge is OK; BPF treats the "mbuf" as read-only */
-               mh.mh_next = m;
-               mh.mh_data = (char *)eh;
-               mh.mh_len = ETHER_HDR_LEN;
-               bpf_mtap(ifp, (struct mbuf *)&mh);
-       }
+       BPF_MTAP2(ifp, eh, ETHER_HDR_LEN, m);
I like directly using

      if (ifp->if_bpf != NULL)
              bpf_mtap_packet(ifp->if_bpf, eh, ETHER_HDR_LEN, m)
in the code, otherwise I have to go looking up what BPF_MTAP2() means
whenever I see it in the code.  And, the resulting code is short enough
that a macro doesn't really make it shorter.


For the support of multiple DLT types on one interface, I have to pass
the bpf_if directly (ifp->if_bpf by default). The macro version BPF_MTAP
allows me to hide that for the normal use of drivers only supporting
one DLT anyway. I prefer the macro (with the hiding of if (ifp->if_bpf != NULL))
because I've read way to many network drivers now and know the meaning.
I consider the ifp->if_bpf != NULL check just a convienence, it could be
done e.g. in bpf_mtap too. That's the reason why I consider the macro
reasonable. That applies to the other uses (BPF_MTAP_PACKET, BPF_TAP) too.
How about

static __inline boolean_t
bpf_enabled(struct ifnet *ifp)
{
	return (ifp->if_bpf != NULL);
}
then the code changes from

BPF_MTAP2(ifp, eh, ETHER_HDR_LEN, m);

to

if (bpf_enabled(ifp))
	bpf_ptap(ifp->if_bpf, m, eh, ETHER_HDR_LEN);
(Note, I've reordered the arguments to move the important parameter, the
packet being tapped to right after the interface.)
I think not having to go look up what BPF_MTAP2, BPF_MTAP3, etc. means is
more important than being able to omit the the 'if_bpf' field in the call to
bpf_ptap().  The code is clearer to the reader if he can see that a call
is going to be made if bpf is enabled, rather than an opaque macro in capital
letters.
If not specifying the 'if_bpf' field is really important to you, than you
can change bpf_ptap() to take a ifnet pointer as the first argument and
just do the dereference inside bpf_ptap():
if (bpf_enabled(ifp))
	bpf_ptap(ifp, m, eh, ETHER_HDR_LEN);
though I consider passing in more info than you need to be bad form.