Re: BPF extensions

[Joerg Sonnenberger <joerg@xxxxxxxxxxxxxxxxx>]

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.

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

Well, the memory waste should be equal, in one case it is code size
(additional move), in the other case it is a variable in the data segment.
The code amount for pushing to the stack should be equal. I don't have
any strong preference here, GCC might optimize it to static anyway, since
the variable is an invariant.

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

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

> 
> +void
> +bpfattach2(struct ifnet *ifp, u_int dlt, u_int hdrlen, struct bpf_if 
> **driverp)
> 
> Same here.  Use a more descriptive name than attach2.

bpfattach_bpfif?

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

Joerg