Page MenuHomeFreeBSD

netgraph/ng_bridge: Reestablish old ABI
ClosedPublic

Authored by lutz_donnerhacke.de on Oct 9 2019, 9:35 PM.

Details

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.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

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

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

julian accepted this revision.Oct 10 2019, 12:54 AM
This revision is now accepted and ready to land.Oct 10 2019, 12:54 AM

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

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).

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.

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.

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.

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

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

emaste added a subscriber: emaste.Oct 11 2019, 12:46 PM
bz added a reviewer: bz.Jan 3 2020, 11:51 AM
bz added a subscriber: bz.

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

bz requested changes to this revision.Jan 3 2020, 1:46 PM
bz added inline comments.
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?

This revision now requires changes to proceed.Jan 3 2020, 1:46 PM
julian added a comment.Jan 3 2020, 4:51 PM

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 added a comment.Jan 3 2020, 4:55 PM

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 requested changes to this revision.Jan 3 2020, 4:56 PM

change review outcome

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

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

lutz_donnerhacke.de marked 4 inline comments as done.Jan 3 2020, 5:33 PM
lutz_donnerhacke.de marked an inline comment as done.
bz requested changes to this revision.Jan 3 2020, 10:44 PM
bz added inline comments.
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.

This revision now requires changes to proceed.Jan 3 2020, 10:44 PM

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.

lutz_donnerhacke.de marked an inline comment as done.Jan 4 2020, 8:20 PM
julian accepted this revision.Jan 5 2020, 1:11 AM
bz requested changes to this revision.Jan 5 2020, 5:06 PM

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

This revision now requires changes to proceed.Jan 5 2020, 5:06 PM
This revision was not accepted when it landed; it landed in state Needs Revision.Jan 5 2020, 7:14 PM
This revision was automatically updated to reflect the committed changes.