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