netgraph/ng_bridge: Reestablish old ABI

[donner]

Summary

In https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=240787#c15
Julian noted, that the ABI change should be downward compatible.

Test Plan

Compile an binary using the old API to generate the netgraph messages.
Compile the kernel with the new version of ng_bridge.
The old binary should work and the new node should react accordingly.

[julian]

It should be backward compatible for 12 at least. Probably not needed for 13 but no harm in carrying it for one release.

[julian]

Can you use both binaries? e.g. a script uses one and manually you use a newer one?

[julian]

That's why I added the cookies to the API.

[donner]

In D21961#479867, @julian wrote:

Can you use both binaries? e.g. a script uses one and manually you use a newer one?

Nope. I'm currently on vacation and do not have a full test bed available.

It's quite hard to build such (user land) binaries, because there was never a need for.
Usually the node is used in ngctl-scripts which is agnostic to the cookies (it's detecting the control structures by introspection)

It's possible, that kernel (netgraph) modules have compiled messages for the bridge node. In this case, the precompiled netgraph module would be able to communicate with the new code using the old ABI.

But I'm not aware of any such thing. If I'd had such an alien module, I'd be prepared to recompile it after changing the kernel. With is patch the alien module would still work even if the kernel is updated by freebsd-update (without going through the source).

[donner]

In D21961#479866, @julian wrote:

It should be backward compatible for 12 at least. Probably not needed for 13 but no harm in carrying it for one release.

I'd propose to change the "#define NGM_BRIDGE_TABLE_API" into "/* set ... in OPTIONS if you need it */" for 13 and remove the code in 14.

[julian]

I wasn't saing you shoudl force yourself to do the test.. just whether it is theoretically possible,, anyhow I'm happy with the result. Don't let me stop you from committing it and I would say that the only case for keeping it in 13 is for the brief period while an upgrade is occurring. (for example I know people who have their interface connected into a bridge so that if the bridge wasn't configured correctly they'd be off the air.. It would basically be for compatibility up the 12 line rather than being needed in the 13 line.

[donner]

In D21961#479926, @julian wrote:

I wasn't saing you shoudl force yourself to do the test.. just whether it is theoretically possible,, anyhow I'm happy with the result. Don't let me stop you from committing it and I would say that the only case for keeping it in 13 is for the brief period while an upgrade is occurring. (for example I know people who have their interface connected into a bridge so that if the bridge wasn't configured correctly they'd be off the air.. It would basically be for compatibility up the 12 line rather than being needed in the 13 line.

Because there is an API change, their scripts might fail anyways.
If they explicitly set "ipfw" config (which was useless), the script will fail.
If they explicitly parse the output of "gettable", the script will fail (due to reporting hook names instead of numbers).
For all other cases (incl. the examples in the man page) the API is compatible.

[donner]

In D21961#479926, @julian wrote:

Don't let me stop you from committing it ...

Don't worry, I do not have commiting rights.

[bz]

Seems this needs someone to commit it; I'll try to do this afternoon (timezone unspecified ;-)).

[bz]

sys/netgraph/ng_bridge.c

458 ↗

(On Diff #63102)

I was editing this down to wrap lines, remove white space, etc. and noticed that this dos not seem to be right unless I misread something?

If we get a NGM_BRIDGE_GET_CONFIG / NGM_BRIDGE_SET_CONFIG / NGM_BRIDGE_GET_TABLE on the NGM_BRIDGE_COOKIE_TBL and fall through then the same cases will be run again and we'll leak memory and and overwrite the original COOKIE_TBL work in the BGM_BRIDGE_COOKIE branch given the next NG_MKRESPONSE() will just allocate new memory again and assign it to resp once more? It's probably only really bad for the two GET commands and the SET command would probably fail?

[julian]

one possible solution to stop the commands from being run a second time. replace 'break' with a goto.. OR poison message->cmd so it wont match next time?

sys/netgraph/ng_bridge.c

412 ↗

(On Diff #63102)

replace with "goto skip"

425 ↗

(On Diff #63102)

replace with "goto skip;"

455 ↗

(On Diff #63102)

replace with "goto skip;"

584 ↗

(On Diff #63102)

add label "skip:"

[julian]

OR do
#ifdef NGM_BRIDGE_TABLE_ABI
if ((msg->header.typecookie == NGM_BRIDGE_GET_CONFIG )||

 (msg->header.typecookie  == NGM_BRIDGE_SET_CONFIG) ||
 (msg->header.typecookie  == NGM_BRIDGE_GET_TABLE) ) {
    ..... new code....
} else

#endif
... old code

[julian]

change review outcome

[donner]

The old ABI is different from the new one.
Fall through to the new ABI was an early (but wrong idea)

[donner]

Allow common calls to the old ABI to fall though to the new one.
Fix generic error handling (go out on error).

[bz]

sys/netgraph/ng_bridge.c

458 ↗

(On Diff #66308)

I think this should be:

if (resp != NULL || error != 0)
  break;

and the goto out and the label should all vanish again.

NG_RESPOND_MSG() still does a NG_FREE_ITEM(item) if resp == NULL and we do want to run through that.

It'd also contain the change to within the #ifdef block which I would consider a feature.

[donner]

Changed back to a self contained handing of the old ABI calls.
Thank you @bz for the idea to check for errors.

Added an additional check to send only valid responses.

[donner]

The diff https://reviews.freebsd.org/D21961?vs=63102&id=66348#toc does only contain two small lines.

[bz]

I'll go ahead and commit it anyway; I think the is no need to refine the review again.