Re: BPF extensions
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
討論串 (同標題文章)
完整討論串 (本文為第 4 之 7 篇)：