Re: BPF extensions

[Jeffrey Hsu <hsu@xxxxxxxxxxx>]

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.