Repository navigation
interface-lifecycle: fix: interfaces created or deleted during the run leave no stale state - #1390
Merged
Merged
Conversation
…erface tables Interfaces deleted during the run, e.g. by ScenarioManager, and interface tables nested inside a node were not covered by any test, and both left stale or wrong state behind in several modules. These module tests show the defects that the following commits of this group repair. - InterfaceTable_deleted_interface_1 and _2: a switch port deleted by ScenarioManager (BridgingLayer and Ieee8021dRelay switch). The switch must flood the frames for the host behind it to the other ports, without touching the deleted interface. Before the repairs this crashes or fails: the interface stays in its table, and the MAC table and the dispatcher keep its entries. - MessageDispatcher_deleted_interface_1: a packet with a stale InterfaceReq must fail with the dispatcher's "Unknown network interface" error; before the repairs it is sent on a gate that is no longer connected. - InterfaceTable_nested_table_1: an interface created during the run in an interface table nested in a router must get no IPv4/IPv6 protocol data; an interface created in the router's own table must still be configured for IPv4. Before the repairs the nested one is configured too. Change: tests | behavior.add | - | interface-lifecycle
…ble whatever deletes it Before this commit, InterfaceTable::deleteInterface() was the only code that removed an interface from its table (cleared its slot and fired interfaceDeletedSignal). An interface module deleted any other way, e.g. by ScenarioManager's <delete-module>, stayed in the table as a dangling pointer: every later getInterfaceById(), getInterface(i) or iteration handed out a freed object. A switch whose port was deleted that way crashed at the next frame for that port (segmentation fault in the relay's logging), or failed further down. After this commit, NetworkInterface::preDelete() calls the new IInterfaceTable::removeInterface(), which does deleteInterface()'s bookkeeping without deleting: it fires interfaceDeletedSignal while the interface still exists, then clears the slot. OMNeT++ calls preDelete() on every module of a deleted subtree before anything is disconnected, so this covers every way of deleting an interface. When the table is deleted along with the interface (the node is deleted, or the network at the end of the run), nothing is done. deleteInterface() now checks the entry and deletes the module; the removal happens in the same preDelete() path, in the same order as before (signal first, then the slot). removeInterface() is a new pure virtual method of IInterfaceTable, next to addInterface(); InterfaceTable is its only implementation in INET. tests/module/InterfaceTable_deleted_interface_1.test and _2.test delete a switch port with ScenarioManager. They also need the following commits of this group, which make the dispatchers and the MAC table forget the deleted interface. Change: src.networklayer | behavior.change.fix | whatsnew migration | interface-lifecycle
Before this commit, MessageDispatcher learned interfaceId -> gate index mappings from registerInterface() and never forgot them; there was no way to unregister. After an interface was deleted during the run, a packet that still carried its id in an InterfaceReq was sent on the gate that had led to the interface and was no longer connected ("Output gate ... is not connected" or "Gate ... not connected"), instead of failing as an unknown interface.
After this commit, unregisterInterface(), the counterpart of registerInterface(), walks the same path, and IInterfaceRegistrationListener gets handleUnregisterInterface(). MessageDispatcher removes the mapping (when it points to the gate the unregistration arrives on) and propagates it to its other gates, as it propagates a registration; Mpls forwards it upward, as it does a registration. NetworkInterface::preDelete() unregisters the interface, unless the module its upperLayerOut leads to is deleted along with it. A packet for a deleted interface now fails with the dispatcher's "Unknown network interface" error.
handleUnregisterInterface() has an empty default body, so listeners outside INET still compile; they keep their mappings as before.
To reproduce the defect, run tests/module/MessageDispatcher_deleted_interface_1.test on the parent: the packet with the stale InterfaceReq is sent on the gate that is no longer connected.
Change: src | behavior.change.fix | whatsnew | interface-lifecycle
… deleted interface Before this commit, MacForwardingTable subscribed to no interface signal. Entries naming an interface deleted during the run stayed until they aged out (120 s by default), static entries forever, so the relay kept forwarding frames to the deleted interface: "getInterfaceById(): no interface with ID=..." in the relay, or a crash before the interface table forgot the interface. After this commit, the table subscribes to interfaceDeletedSignal on its own interface table module, which emits it for its own interfaces only, so the interfaces of another table, whose ids may collide, are not affected. On the signal it removes the interface's unicast entries (removeForwardingInterface()) and removes the interface from the multicast entries, dropping entries left with no interface. Frames for an address learned on the deleted interface are flooded again, as for an unknown address. tests/module/InterfaceTable_deleted_interface_1.test and _2.test cover this case: a switch port deleted by ScenarioManager while the MAC table holds entries for it. Change: src.linklayer | behavior.change.fix | whatsnew | interface-lifecycle
…erface tables in the node Before this commit, modules that react to interface signals subscribed to them on their node, so they also received the signals of any other interface table nested inside the node, and acted on those interfaces as on the node's own. For an interface created during the run in a nested table, Ipv4NodeConfigurator added Ipv4InterfaceData and joined it to 224.0.0.1 and 224.0.0.2, IGMP configured it, and the routing tables recomputed their routes. Interface ids are per table, so they collide across tables: deleting a nested multicast interface made Igmpv2/Igmpv3/Mldv1/Mldv2 delete the host and router state of the node's own interface with the same id. After this commit, isSignalForInterfaceTable() tells whether the object of a signal is for a given interface table: it is about an interface of that table (the NetworkInterface, or the interface of the NetworkInterfaceChangeDetails), or it is not about an interface at all. The tables are compared as objects (NetworkInterface::getInterfaceTable()), as ids collide. Each node-level listener returns early when a signal is not for its own interface table: Ipv4NodeConfigurator, Ipv6NodeConfigurator, Ipv4RoutingTable, Ipv6RoutingTable, Igmpv2, Igmpv3, Mldv1, Mldv2, Rip, Ospfv2, Ospfv3Process, EigrpIpv4Pdm, EigrpIpv6Pdm, PimDm, PimInterfaceTable, L2NodeConfigurator, Mrp; DhcpClient checks its deletion branch. PimInterfaceTable keeps its interface table in a member for this. The subscriptions stay on the node, so the order in which listeners are called does not change. Not changed: listeners that already compare against their own interfaces (Rstp, DhcpServer), NextHopRoutingTable (its handler is empty), and the network-wide observers (RoutingTableRecorder, visualizers, RadioMedium, GlobalArp, Ipv4NetworkConfigurator). To reproduce the defect, run tests/module/InterfaceTable_nested_table_1.test on the parent: the interface created in the nested interface table gets IPv4 protocol data. Change: src | behavior.change.fix | whatsnew | interface-lifecycle
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Network interfaces created or deleted during the run, e.g. by ScenarioManager, left stale state behind in several modules, and modules subscribed on a node also acted on the interfaces of interface tables nested inside the node.
The first commit adds module tests for each defect; the following commits fix them.
API additions: IInterfaceTable::removeInterface() (pure virtual; InterfaceTable is the only implementation in INET), IInterfaceRegistrationListener::handleUnregisterInterface() (default empty), and the free function isSignalForInterfaceTable().