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.

In D17512#373738, @arichardson wrote:

I wonder if this could be done in a single pass by adding a dummy symbol in the pcpu/vnet macros and using . = . + (DEFINED(pcpu_dummy) ? 1 : 0?

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.

While you are at it, what about https://reviews.freebsd.org/D13865 ?

Quick question from scrolling through

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?

Moving functions should be a separate commit and not be mixed with the functional changes.

LGTM

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?

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.

LGTM

LGTM

Post commit Pointyhat to: bz

Just came across this commit tracing other problems ...

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.

LGTM

I think it looks good; given you've done the testing.

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.

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?

In D16459#349406, @eugen_grosbein.net wrote:

In D16459#349396, @bz wrote:

Hmm RFC 2863 says:

ifAlias OBJECT-TYPE

SYNTAX      DisplayString (SIZE(0..64))

Nice catch. Does it mean that complying SNMP application must support at most 64 (or 65) octects lenght ifAlias or it must support at least so? Note that Cisco supports "at least" or more: http://cfn.cloudapps.cisco.com/ITDIT/CFN/Dispatch?act=featdesc&task=display&featureId=742

So where is our real problem?

"Test plan" section describes our problems with bsnmpd:

• bsnmpd spams logs indefinitly if a system has interface with description over 65 characters;

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:

• Merge branch 'p2' into p3

• Looks like there was an unlock lost during the merge.

In D16205#344159, @imp wrote:

In D16205#344062, @bz wrote:

Given this is multiple architectures, how are you dealing with colliding/different values? Hmm I see you don't touch that part.

There are no such values. RB_* are 100% the same everywhere.

• 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?