glebius added a subscriber: glebius.
glebius added a comment.
Kristof, big thanks for working on this. See my comments.
INLINE COMMENTS
sys/netpfil/pf/pf.c:366 This function can also be used not only for fragment
rbtree, but can also substitute the PF_ANEQ, PF_AEQ and could be considered to
substitute even pf_match_addr(). So, lots of code can be generalized using this
function. That's good.
I'd suggest not to inline it, and rename it to pf_addr_cmp. "cmp" is a widely
used abbreviation clear to anyone.
sys/netpfil/pf/pf.c:396 Please add a panic() for default switch case.
sys/netpfil/pf/pf_norm.c:64 Please use C99 types in new code instead of
80-ish BSD types: uint32_t, uint16_t, uint8_t.
sys/netpfil/pf/pf_norm.c:72 When hacking pf, I strongly dislike this struct
foo and struct foo_cmp, that must be kept synchronized. I skipped fixing that
in 2012 and now I regret that. Let's now do it better in the new code. You can
just embed pf_fragment_cmp into pf_fragment. Alternatively, you can do this
trick:
#define fr_cmp_offset fr_entry
and in code you do:
bcmp(a, b, offsetof(struct pf_fragment, fr_cmp_offset))
I prefer the trick over embedding, but that's up to you.
sys/netpfil/pf/pf_norm.c:340 Empty line here is style(9) requirement,
shouldn't be removed.
sys/netpfil/pf/pf_norm.c:403 Let's put PF_FRAG_ASSERT() at the beginning of
this function. Kinda documenting its locking requirements.
sys/netpfil/pf/pf_norm.c:459 Dots in end of comments throught the function,
please. Thanks :)
sys/netpfil/pf/pf_norm.c:735 Please use KASSERT:
m = m_getptr(m, hdrlen + offsetof(struct ip6_frag, ip6f_nxt), &off);
KASSERT(m, ("%s: short mbuf chain", __func__));
sys/netpfil/pf/pf_norm.c:757 Same here.
REVISION DETAIL
https://reviews.freebsd.org/D1765
To: kristof
Cc: glebius, freebsd-net
_______________________________________________
freebsd-net@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to "freebsd-net-unsubscr...@freebsd.org"
