Eliminate KAME custom circular queues in reassembly code.
AbandonedPublic
Actions

Authored by jtl on Aug 22 2018, 4:27 PM.

Details

Reviewers

jhb
bz

Summary

Move the ip6asfrag struct definition into frag6.c. This is the only in-tree consumer of this structure. (This will be a separate commit.)

Remove the KAME custom circular queue for fragments and replace it with a standard TAILQ.

Remove the KAME custom circular queue for fragmented packets and replace it with a standard TAILQ.

Diff Detail

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 19118
Build 18743: arc lint + arc unit

Event Timeline

jtl created this revision.Aug 22 2018, 4:27 PM

Herald added a subscriber: ae. · View Herald TranscriptAug 22 2018, 4:27 PM

Harbormaster completed remote builds in B19051: Diff 47108.Aug 22 2018, 4:27 PM

Just one suggestion. Overall, the intent of the code is clearer now IMO.

sys/netinet6/frag6.c
841–842	When possible I prefer to not do side effects in conditionals (with while() loops it's not always possible to do that w/o duplicating code which is worse), but in this case I would probably prefer the assignment on a separate line before the if (also matches the existing code slightly better).

This revision is now accepted and ready to land.Aug 23 2018, 9:48 AM

jtl added inline comments.Aug 23 2018, 1:09 PM

sys/netinet6/frag6.c
841–842	I can make that change.

Limit the ipq structure to the kernel to eliminate a buildworld failure. (And, why should we make userspace code import the sys/queue.h header for a structure they don't need anyway?)
Address @jhb's nit.

This revision now requires review to proceed.Aug 23 2018, 10:24 PM

Harbormaster completed remote builds in B19118: Diff 47210.Aug 23 2018, 10:24 PM

FYI, something didn't look right doing my tests. So, I'm going to delay committing this until I can satisfy myself that it behaves correctly. That will almost certainly mean I miss the code freeze. C'est la vie!

I skipped the #if 0 blocks ; we should nuke them (separately).
I still find the code very confusing and am not sure if all the HEAD/TAIL operations are correct and on the right part.

I wish we wouldn't call the same thing "i", "hash", and "bucket" if I am not mislead.
I'd really love to have the insert/++/inc() and remove/--/dec() bits all together with a proper name as a macro or inline function not passing in any globals (element and "hash" should be ok usually for insert).

I need to do a second pass over all this but figured I'd leave my brain dump here in case you want to do some non-functional (separate) changes, which can go in upfront.

sys/netinet6/frag6.c
96	I understand you just moved this over; why do we take a pointer cast it to a double pointer of same type and dereference it again? #define IP6_REASS_MBUF(ip6af) ((ip6af)->ip6af_m) is exactly the same, right? Or am I too tired?
97	Moving this, can you please move it below all the variable declarations?
113	Again personal taste vs. keeping existing code but I really prefer the & not to be part of the macro. It makes all the TAILQ* calls look wrong. Again that would not be a functional change to intermix with other work.
553	XXX-BZ come back to this

Just as a comment: I finally figured out what's so confusing when reading this these days. The ip6q[] is the buckets and not of struct ip6q anymore; sadly struct ip6q is a public interface used inside the MAC framework so don't want to break that by renaming it.

I am currently revising this and D16850 and some other fixes (e.g., handle atomic frags according to 8200 spec) and some future errata. I'll contact @jtl for as how to quickly do the updated versions out. Might be easier for me to post the revised versions step by step than to get these to reviews updated.

bz mentioned this in rS353965: frag6: replace KAME hand-rolled queues with queue(9) TAILQs.Oct 23 2019, 11:01 PM

Basically commited by @bz .