Re: BPF extensions

[Jeffrey Hsu <hsu@xxxxxxxxxxx>]

Joerg Sonnenberger wrote:
> Hi all,
> a patch waits at http://leaf.dragonflybsd.org/~joerg/bpf.diff
> to be tested e.g. with wi(4) devices. It adds the long pushed
> back support for multiple data link types on one interface
> and does some general cleanup of the BPF usage.
>
> If you want to the the multiple data link types with wi(4),
> keep in mind to rebuild libpcap with current header files or
> do a buildworld.
>
> TTC: Wednesday
>
> Joerg

-       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.

+       static const uint32_t af = AF_INET;
+
+       BPF_MTAP2(ifp, &af, sizeof(af), m);

This doesn't have to be static, as all it does is waste space in the initialized
data section.  Heap might be better here.  It all depends on whether the
code to initialize a constant AF_INET is much bigger than the code to
load from memory, which I suspect is not the case.

+ * 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().

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

+void
+bpfattach2(struct ifnet *ifp, u_int dlt, u_int hdrlen, struct bpf_if **driverp)

Same here.  Use a more descriptive name than attach2.

+#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.