- User Since
- Aug 29 2014, 12:11 PM (215 w, 4 d)
Fri, Oct 12
Maybe both? We also don't want to not get notified and have an interface hanging around for ever given pf never releases a reference?
In the first block of you patch you might want to switch the order and set the kif to NULL before releasing the reference.
Thu, Oct 11
Wed, Oct 10
The kmod.mk changes can be reduced to two linker invocations instead of three,
also needing one less intermediate file to cleanup.
Argh the kmod changes are the old ones. I'll update in a second
Still wonder if pf should hold a reference to the interface as well.
That was kind-of the model I had in mind for the other callbacks as well ;-)
OK, this only makes sense after the callback functions are virtualized; before the above comment did make sense but was expecting a different architechtural solution (whether true or not at the time of writing)
OK. I was thinking (when reading the UNINIT change) that that only starts to make sense after virtualising these here; and then I wondered why I'd want to virtualise if these called functions would be perfectly fine and happy to determine if pfsync is enabled (RUNNING) in this vnet or not. That way there'd be less virtualisation.
Actually, why is this needed?
I assume the "other way round" is only true after the function pointers have been virtualised, otherwise not.
And that makes me wonder..
I only scrolled through but it looks good to me.
Mon, Oct 8
While you are at it, what about https://reviews.freebsd.org/D13865 ?
Thu, Oct 4
Tue, Oct 2
Mon, Oct 1
Quick question from scrolling through
Sat, Sep 29
Thu, Sep 27
Ok, I started three times already and got an INTR every time; let me ask the question: why are we doing this all manually here rather than calling the (from here) the generic ioctl to set the MTU and avoid the code duplication?
Mon, Sep 24
Moving functions should be a separate commit and not be mixed with the functional changes.
Fri, Sep 21
Thu, Sep 20
Wed, Sep 19
I was wondering for a second about the v6 path; at first glance it seems that udp6_common_ctlinput() via in6_pcbnotify() calls it with a write lock held; wouldn't hurt to double check that please?
Tue, Sep 18
Sep 14 2018
Sep 10 2018
I should probably add that I never liked having two options, but it's been inherited from what we had (I think v6 had it and I did the same for legacy). But since our option space has been way too fine grained to my believe anyway for too many things, which is a totally different issue.
Sep 6 2018
Sep 4 2018
Sep 3 2018
Post commit Pointyhat to: bz
Aug 27 2018
Just came across this commit tracing other problems ...
Aug 24 2018
Go and have a look at https://svnweb.freebsd.org/base/head/sys/netpfil/ipfw/ip_fw2.c?annotate=336676#l1453 and be inspired for more?
The functional changes seem fine even though they address multiple independent problems and should probably be fixed in two independent commits?
Ignoring the unrelated noise removing the #if 0 block the functional changes seem fine.
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.
Aug 23 2018
Aug 20 2018
I think it looks good; given you've done the testing.
Aug 17 2018
Aug 16 2018
Aug 10 2018
I've only scrolled through. I really don't get why ipv6z (needless to say ipv4z is defined but not used) needs to be special. The fact that we encode the interface index is unhelpful in a config file as that might change between boots or other operations, such as oder of vnet creation, order of interface creation, etc.
Aug 6 2018
Jul 29 2018
There's an exclamation mark missing at the ##else; like [style(9)]:
#ifdef COMPAT_43 /* A large region here, or other conditional code. */ #else /* !COMPAT_43 */ /* Or here. */ #endif /* COMPAT_43 */
Apart from that, thanks! :) I haven't done a any logical review. I assume Kevin has done that.
Looks like there is a couple of overlong lines in there which should be wrapped.
Can you put comments at else and endif parts as the blocks are at times rather long and nested, so it would make it easier to find the other bit?
Jul 26 2018
I guess I want the real problem to be identified and understood (or a good description for this change and why) rather than patching something that simply masks a different problem.
Hmm RFC 2863 says:
Jul 24 2018
Jul 20 2018
Jul 16 2018
Jul 11 2018
- Merge branch 'p2' into p3
- Looks like there was an unlock lost during the merge.
- Merge branch 'p1' into p2
- Merge branch 'p0' into p1
- Maintanance on new code; add virtualisation where needed.
Given this is multiple architectures, how are you dealing with colliding/different values? Hmm I see you don't touch that part.
How are you dealing with architecture specific ones (not #defined but now part of the switch statement)?
I guess what I am saying is: if we clean this up, can we also break everything and cleanup the #defines along somehow to only have one set of those as well?
Jul 10 2018
Update to more recent head.