Virtualize System and Inet interfaces #7715

andy31415 · 2021-06-17T14:15:14Z

Problem

SystemLayer and InetLayer are passed around as pointer already in many places, however they still are not flexible:

DeviceLayer::SystemLayer and similar are used to access a global
The classes are fixed and implementation is #ifdef depending on build

We need to cleanup the #ifdef style of conditionals for readability and maintenance.

We want to be able to perform functional and endtoend tests in our code. We are able to do full simulation (e.g. docker/cirque) however for performance it seems better to be able to have 'mock interfaces' to simulate CHIP nodes within a single process and run tests faster rather than spinning up new applications or docker images.

Proposed Solution

Virtualize Inet and System layer
Separate _Impl.cpp that are chosen by the build script instead of having #ifdef usage
Pass the layers as needed instead of relying on DeviceLayer to access global state

The text was updated successfully, but these errors were encountered:

electrocucaracha · 2021-06-23T21:08:06Z

One alternative for this it is using Vagrant tool, it allows to declare as many as NICs as needed in their Vagrantfile

Vagrant.configure("2") do |config|
  config.vm.network :private_network, ip: "192.168.0.2", type: :static, libvirt__network_name: "mgmt", nic_type: "virtio"
  config.vm.network :private_network, ip: "10.0.0.2", type: :static, libvirt__network_name: "private", nic_type: "virtio"
end

And it's supported on the macOS 10.15 virtual environment so it's possible to consume it in a GitHub Action.

name: Check CI
# yamllint disable-line rule:truthy
on: [push, pull_request]

jobs:
  check-e2e:
    runs-on: macos-10.15
    env:
      VAGRANT_DISABLE_VBOXSYMLINKCREATE: 1
    steps:
      - uses: actions/checkout@v2
      - name: Cache Vagrant boxes
        uses: actions/cache@v2
        with:
          path: ~/.vagrant.d/boxes
          key: ${{ runner.os }}-vagrant-${{ hashFiles('Vagrantfile') }}
          restore-keys: |
            ${{ runner.os }}-vagrant-
      - name: Create Virtual Machine
        run: vagrant up

#### Problem Transitionally, `System::Layer` operations are provided by a configuration-specific `WatchableEventManager`, but they are not yet sufficiently aligned to convert to abstract and implementation classes. Part of project-chip#7715 Virtualize System and Inet interfaces #### Change overview - Move socket event watch APIs into `System::Layer`. - Remove `WatchableSocket` and its include-file hack that allowed implementation-specific state to be contained directly in EndPoints. - Revise the libevent sample implementation to use libevent timers directly instead of the System::Timer pool. The libevent implementation uses STL containers with heap-allocated state for the sake of being different from the select()-based implementation. #### Testing CI; no change to functionality should is expected.

#### Problem Transitionally, `System::Layer` operations are provided by a configuration-specific `WatchableEventManager`, but they are not yet sufficiently aligned to convert to abstract and implementation classes. Part of project-chip#7715 Virtualize System and Inet interfaces #### Change overview - Move socket event watch APIs into `System::Layer`. - Remove `WatchableSocket` and its include-file hack that allowed implementation-specific state to be contained directly in EndPoints. - Revise the select() implementation to use an internal pool for watch state in place of the EndPoint-embedded WatchableSocket. - Revise the libevent sample implementation to use libevent timers directly instead of the System::Timer pool. The libevent implementation uses STL containers with heap-allocated state for the sake of being different from the select()-based implementation. #### Testing CI; no change to functionality should is expected.

* Move socket watch APIs into System::Layer #### Problem Transitionally, `System::Layer` operations are provided by a configuration-specific `WatchableEventManager`, but they are not yet sufficiently aligned to convert to abstract and implementation classes. Part of #7715 Virtualize System and Inet interfaces #### Change overview - Move socket event watch APIs into `System::Layer`. - Remove `WatchableSocket` and its include-file hack that allowed implementation-specific state to be contained directly in EndPoints. - Revise the select() implementation to use an internal pool for watch state in place of the EndPoint-embedded WatchableSocket. - Revise the libevent sample implementation to use libevent timers directly instead of the System::Timer pool. The libevent implementation uses STL containers with heap-allocated state for the sake of being different from the select()-based implementation. #### Testing CI; no change to functionality should is expected. * restyle * fix GetCommandExitStatus race * Enforce use of API via System::Layer * also include dispatch case * add LwIP case * restyle

* Move socket watch APIs into System::Layer #### Problem Transitionally, `System::Layer` operations are provided by a configuration-specific `WatchableEventManager`, but they are not yet sufficiently aligned to convert to abstract and implementation classes. Part of project-chip#7715 Virtualize System and Inet interfaces #### Change overview - Move socket event watch APIs into `System::Layer`. - Remove `WatchableSocket` and its include-file hack that allowed implementation-specific state to be contained directly in EndPoints. - Revise the select() implementation to use an internal pool for watch state in place of the EndPoint-embedded WatchableSocket. - Revise the libevent sample implementation to use libevent timers directly instead of the System::Timer pool. The libevent implementation uses STL containers with heap-allocated state for the sake of being different from the select()-based implementation. #### Testing CI; no change to functionality should is expected. * restyle * fix GetCommandExitStatus race * Enforce use of API via System::Layer * also include dispatch case * add LwIP case * restyle

#### Problem Issue project-chip#7715 _Virtualize System and Inet interfaces_ #### Change overview - Establishes an abstract `System::Layer` class hierarchy, described below. - Converts the transitional `WatchableEventManager` classes into corresponding `LayerImpl` classes. - Move `LwIPEventHandlerDelegate` into `LayerLwIP`. - Remove `GetClock()`, which was only used in one file. (`System::Clock` is currently fully static, but might benefit from its own virtualization, e.g. for test timeouts.) The class hierarchy is: - `Layer`: Core timer methods. - `LayerLwIP`: Adds methods specific to builds using `CHIP_SYSTEM_CONFIG_USING_LWIP`. - `LayerImplLwIP`: Concrete implementation of `LayerLwIP`. (This is currently used by all LwIP-based platforms.) - `LayerSockets`: Adds I/O event methods specific to builds using `CHIP_SYSTEM_CONFIG_USING_SOCKETS`. - `LayerSocketsLoop`: Adds methods for event-loop-based implementations. - `LayerImplSelect`: Concrete implementation of `LayerSocketLoop`, using `select()`. (This is currenly used by all sockets-based platforms.) - `LayerImplLibevent`: Concrete implementation of `LayerSocketLoop`, using `libevent`. (This is currenly a demonstration of substitutability, not used by any platform.) #### Testing CI; no changes to functionality. Tests updated where necessary to create concrete `LayerImpl` objects.

#### Problem Issue project-chip#7715 _Virtualize System and Inet interfaces_ #### Change overview - Establishes an abstract `System::Layer` class hierarchy, described below. - Converts the transitional `WatchableEventManager` classes into corresponding `LayerImpl` classes. - Move `LwIPEventHandlerDelegate` into `LayerLwIP`. - Remove `GetClock()`, which was only used in one file. (`System::Clock` is currently fully static, but might benefit from its own virtualization, e.g. for test timeouts.) - For the moment, `CHIP_SYSTEM_CONFIG_USE_DISPATCH` is still included by `#if` on `LayerImplSelect`. With some changes to Inet, Dispatch can probably have its own much simpler implementation of `LayerSockets`. The class hierarchy is: - `Layer`: Core timer methods. - `LayerLwIP`: Adds methods specific to builds using `CHIP_SYSTEM_CONFIG_USING_LWIP`. - `LayerImplLwIP`: Concrete implementation of `LayerLwIP`. (This is currently used by all LwIP-based platforms.) - `LayerSockets`: Adds I/O event methods specific to builds using `CHIP_SYSTEM_CONFIG_USING_SOCKETS`. - `LayerSocketsLoop`: Adds methods for event-loop-based implementations. - `LayerImplSelect`: Concrete implementation of `LayerSocketLoop`, using `select()`. (This is currenly used by all sockets-based platforms.) - `LayerImplLibevent`: Concrete implementation of `LayerSocketLoop`, using `libevent`. (This is currenly a demonstration of substitutability, not used by any platform.) #### Testing CI; no changes to functionality. Tests updated where necessary to create concrete `LayerImpl` objects.

* Virtualize System::Layer interfaces. #### Problem Issue #7715 _Virtualize System and Inet interfaces_ #### Change overview - Establishes an abstract `System::Layer` class hierarchy, described below. - Converts the transitional `WatchableEventManager` classes into corresponding `LayerImpl` classes. - Move `LwIPEventHandlerDelegate` into `LayerLwIP`. - Remove `GetClock()`, which was only used in one file. (`System::Clock` is currently fully static, but might benefit from its own virtualization, e.g. for test timeouts.) - For the moment, `CHIP_SYSTEM_CONFIG_USE_DISPATCH` is still included by `#if` on `LayerImplSelect`. With some changes to Inet, Dispatch can probably have its own much simpler implementation of `LayerSockets`. The class hierarchy is: - `Layer`: Core timer methods. - `LayerLwIP`: Adds methods specific to builds using `CHIP_SYSTEM_CONFIG_USING_LWIP`. - `LayerImplLwIP`: Concrete implementation of `LayerLwIP`. (This is currently used by all LwIP-based platforms.) - `LayerSockets`: Adds I/O event methods specific to builds using `CHIP_SYSTEM_CONFIG_USING_SOCKETS`. - `LayerSocketsLoop`: Adds methods for event-loop-based implementations. - `LayerImplSelect`: Concrete implementation of `LayerSocketLoop`, using `select()`. (This is currenly used by all sockets-based platforms.) - `LayerImplLibevent`: Concrete implementation of `LayerSocketLoop`, using `libevent`. (This is currenly a demonstration of substitutability, not used by any platform.) #### Testing CI; no changes to functionality. Tests updated where necessary to create concrete `LayerImpl` objects. * remove debug code from SystemPacketBuffer.h

* Virtualize System::Layer interfaces. #### Problem Issue project-chip#7715 _Virtualize System and Inet interfaces_ #### Change overview - Establishes an abstract `System::Layer` class hierarchy, described below. - Converts the transitional `WatchableEventManager` classes into corresponding `LayerImpl` classes. - Move `LwIPEventHandlerDelegate` into `LayerLwIP`. - Remove `GetClock()`, which was only used in one file. (`System::Clock` is currently fully static, but might benefit from its own virtualization, e.g. for test timeouts.) - For the moment, `CHIP_SYSTEM_CONFIG_USE_DISPATCH` is still included by `#if` on `LayerImplSelect`. With some changes to Inet, Dispatch can probably have its own much simpler implementation of `LayerSockets`. The class hierarchy is: - `Layer`: Core timer methods. - `LayerLwIP`: Adds methods specific to builds using `CHIP_SYSTEM_CONFIG_USING_LWIP`. - `LayerImplLwIP`: Concrete implementation of `LayerLwIP`. (This is currently used by all LwIP-based platforms.) - `LayerSockets`: Adds I/O event methods specific to builds using `CHIP_SYSTEM_CONFIG_USING_SOCKETS`. - `LayerSocketsLoop`: Adds methods for event-loop-based implementations. - `LayerImplSelect`: Concrete implementation of `LayerSocketLoop`, using `select()`. (This is currenly used by all sockets-based platforms.) - `LayerImplLibevent`: Concrete implementation of `LayerSocketLoop`, using `libevent`. (This is currenly a demonstration of substitutability, not used by any platform.) #### Testing CI; no changes to functionality. Tests updated where necessary to create concrete `LayerImpl` objects. * remove debug code from SystemPacketBuffer.h

#### Problem `src/inet` has some unused code inherited from Weave, which complicates maintenance. (Specifically, this is on the path to project-chip#7715 _Virtualize System and Inet interfaces_.) #### Change overview - Remove some unused code from `Inet::EndPointBasis` - Remove some unused `#include`s - Remove unused `INET_CONFIG_WILL_OVERRIDE_PLATFORM_XTOR_FUNCS` - Remove `src/inet/InetUtils.cpp` - Remove `src/system/SystemLayerPrivate.h` - Convert LwIPEndPointType to `enum class` #### Testing CI; no functional changes.

#### Problem `src/inet` has some unused code inherited from Weave, which complicates maintenance. (Specifically, this is on the path to #7715 _Virtualize System and Inet interfaces_.) #### Change overview - Remove some unused code from `Inet::EndPointBasis` - Remove some unused `#include`s - Remove unused `INET_CONFIG_WILL_OVERRIDE_PLATFORM_XTOR_FUNCS` - Remove `src/inet/InetUtils.cpp` - Remove `src/system/SystemLayerPrivate.h` - Convert LwIPEndPointType to `enum class` #### Testing CI; no functional changes.

#### Problem Code for different Inet implementations is scattered across many `#if`s. #### Change overview This is a step toward project-chip#7715 _Virtualize System and Inet interfaces_. - For the affected `.cpp` files, code is moved to a single `#if` section for each of the three implementations (LwIP, sockets, and Network Framework.) Some methods have internals split off into separate `…Impl` methods. - Method implementations are reordered to match their declarations. - Doxygen comments move to headers. Since comparing moved code in diff format is difficult, this change explicitly does NOT make any changes to function bodies, beyond any necessary `return` when splitting. #### Testing CI; no change to functionality.

kpschoedel · 2021-10-12T15:32:29Z

My current impression is that for Inet, full abstract interchangeability may be impractically expensive, but we can get cleaner separation and mockability within each major build group (LwIP vs sockets vs …).

#### Problem This is a step toward project-chip#7715 _Virtualize System and Inet interfaces_, building on recent virtualization of Inet endpoints and changes to object allocation pools. #### Change overview - XXX #### Testing CI; no changes to functionality.

#### Problem This is a step toward project-chip#7715 _Virtualize System and Inet interfaces_ for mockability, building on recent virtualization of Inet endpoints and changes to object allocation pools. The `InetLayer` class acts as a factory for `UDPEndPoint` and `TCPEndPoint`. Merely making its methods virtual would result in an unacceptable code size increase for applications that do not use TCP on platforms that offer it, since the vtable would introduce a dependency that normal linking doesn't remove. #### Change overview - Split the `EndPoint` factory/iteration code into its own class, `EndPointManager`. `InetLayer` remains as a stub wrapper for the pair of Managers. - Move the code for TCP idle timeout from `InetLayer` to `TCPEndPoint`. - Move `IPPacketInfo` from `InetLayer.h` to its own files. **NOTE** that applications that use CHIP TCP must now explicitly initialize it via `InetLayer::InitTCP()`. #### Testing CI; no changes to external functionality. Tests revised where necessary to initialize TCP.

#### Problem This is a step toward #7715 _Virtualize System and Inet interfaces_ for mockability, building on recent virtualization of Inet endpoints and changes to object allocation pools. The `InetLayer` class acts as a factory for `UDPEndPoint` and `TCPEndPoint`. Merely making its methods virtual would result in an unacceptable code size increase for applications that do not use TCP on platforms that offer it, since the vtable would introduce a dependency that normal linking doesn't remove. #### Change overview - Split the `EndPoint` factory/iteration code into its own class, `EndPointManager`. `InetLayer` remains as a stub wrapper for the pair of Managers. - Move the code for TCP idle timeout from `InetLayer` to `TCPEndPoint`. - Move `IPPacketInfo` from `InetLayer.h` to its own files. **NOTE** that applications that use CHIP TCP must now explicitly initialize it via `InetLayer::InitTCP()`. #### Testing CI; no changes to external functionality. Tests revised where necessary to initialize TCP.

* Convert Inet::InterfaceId to a class #### Problem The current `#if`+`typedef` requires the actual definition to be visible at every appearance, rather than merely an opaque forward class declaration, which is an obstace to project-chip#7715 _Virtualize System and Inet interfaces_. #### Change overview Convert `InterfaceId` to a class. Some free functions become class methods, and `INET_NULL_INTERFACEID` is replaced by a default-constructed `InterfaceId`. #### Testing CI; no changes to functionality intended. * add InterfaceId::Null() * replace strcpy()

* Collapse IPEndPointBasis into UDPEndPoint #### Problem For historical reasons, Inet EndPoints have an unnecessarily deep class hierarchy: InetLayerBasis EndPointBasis TCPEndPoint IPEndPointBasis UDPEndPoint #### Change overview This change moves the contents of IPEndPointBasis into UDPEndPoint. InetLayerBasis EndPointBasis TCPEndPoint UDPEndPoint Since comparing moved code in diff format is difficult, this change explicitly does NOT make any changes to moved code; a future change will remove redundancies. Transitionally, methods that would have duplicate names have are prefixed with `Ip` here. This is a step toward project-chip#7715 _Virtualize System and Inet interfaces_. #### Testing CI; no changes to functionality. * Restyled by clang-format Co-authored-by: Restyled.io <commits@restyled.io>

#### Problem PR project-chip#11135 moved code from IPEndPointBasis into UDPEndPoint without otherwise touching it, leaving various redundancies. #### Change overview This change merges corresponding functions, with a small amount of refactoring of resulting bodies. This is a step toward project-chip#7715 _Virtualize System and Inet interfaces_. #### Testing CI; no changes to functionality.

#### Problem This is a step toward project-chip#7715 _Virtualize System and Inet interfaces_. #### Change overview Move each `Inet::EndPointBasis` implementation into its own file. Each inherits from `Inet::EndPointBase`, but transitionally each implementation is statically named `EndPointBasis`, and initialization continues to be done in `InitEndPointBasis()` rather than a constructor. #### Testing CI; no changes to functionality.

* Inet: Use constructors for Inet EndPoints #### Problem `Inet::TCPEndPoint` and `Inet::UDPEndPoint` historically could not use constructors because of `System::ObjectPool` limitations. Incidentally renamed `mAppState` for consistency (it had been in `System::Object` prior to project-chip#11428). This is a step toward project-chip#7715 _Virtualize System and Inet interfaces_, split off to reduce the complexity of an upcoming PR. #### Change overview Convert from `Init()` to constructors. Transitionally, the constructors still call a per-implementation function `InitImpl()`. #### Testing CI; no changes to functionality. * explicitly initialize all members

…11673) * Inet: Move some InterfaceId functions out of InetLayer #### Problem `InetLayer` contains some operations involving `InterfaceId` that don't use any `InetLayer` state. This is a step toward project-chip#7715 _Virtualize System and Inet interfaces_. #### Change overview Move these methods from `InetLayer` to `InterfaceId`: - `GetLinkLocalAddr()` - `MatchLocalIPv6Subnet()` - `GetInterfaceFromAddr()`, renamed `InterfaceId::FromIPAddress()` #### Testing CI; no changes to functionality. * restyle

#### Problem This is a step toward project-chip#7715 _Virtualize System and Inet interfaces_. #### Change overview - The per-implementation subclasses (formerly `#if` sections) of `EndPointBase` didn't actually depend on `EndPointBase` at all, so they are split off into `EndPointState${IMPL}` classes, separately inherited by the leaf classes. - `UDPEndPoint` and `TCPEndPoint` are split into base classes and per-implementation subclasses. Transitionally, the implementation classes remain the main files, and are instantiated using a single concrete name by `#if`. #### Testing CI; no changes to functionality.

#### Problem Part of project-chip#7715 _Virtualize System and Inet interfaces_ #### Change overview Move each `UDPEndPoint` and `TCPEndPoint` implementation into a separate file. (This follows the pattern used by `System::Layer`.) #### Testing CI; no change to functionality.

#### Problem After PR project-chip#12291, `InetLayer` merely holds pointers to TCP and UDP `EndPointManager`. Almost all uses are of `UDPEndPointManager` only, so `InetLayer` is an unnecessary indirection. Part of project-chip#7715 _Virtualize System and Inet interfaces_ #### Change overview Remove the `InetLayer` class and pass or store `UDPEndPointManager` and/or `TCPEndPointManager` directly. (This was not included in project-chip#12291 because it touches a large number of files with trivial changes.) #### Testing CI; no change to external functionality.

#### Problem Part of #7715 _Virtualize System and Inet interfaces_ #### Change overview Move each `UDPEndPoint` and `TCPEndPoint` implementation into a separate file. (This follows the pattern used by `System::Layer`.) #### Testing CI; no change to functionality.

#### Problem After PR project-chip#12291, `InetLayer` merely holds pointers to TCP and UDP `EndPointManager`. Almost all uses are of `UDPEndPointManager` only, so `InetLayer` is an unnecessary indirection. Part of project-chip#7715 _Virtualize System and Inet interfaces_ #### Change overview Remove the `InetLayer` class and pass or store `UDPEndPointManager` and/or `TCPEndPointManager` directly. (This was not included in project-chip#12291 because it touches a large number of files with trivial changes.) #### Testing CI; no change to external functionality.

* [Inet] Remove InetLayer class #### Problem After PR #12291, `InetLayer` merely holds pointers to TCP and UDP `EndPointManager`. Almost all uses are of `UDPEndPointManager` only, so `InetLayer` is an unnecessary indirection. Part of #7715 _Virtualize System and Inet interfaces_ #### Change overview Remove the `InetLayer` class and pass or store `UDPEndPointManager` and/or `TCPEndPointManager` directly. (This was not included in #12291 because it touches a large number of files with trivial changes.) #### Testing CI; no change to external functionality. * restyle * remove src/inet/InetLayer.cpp * fix merge

#### Problem Part of project-chip#7715 _Virtualize System and Inet interfaces_ #### Change overview - Convert `InterfaceId`, which is relatively heavily used and stored, to be platform-independent. Platform-specific parts move to a namespace `PlaformNetworkInterface` (since this includes a type and compile-time constant, it can't practically be made an abstract class). - Convert `InterfaceIterator` and `InterfaceAddressIterator` to use abstract base classes, for mockability. At present, the concrete implementation is fixed at compile time under the original name. - Split the three implementations into separate files. #### Testing CI; no change to outside functionality.

#### Problem Part of project-chip#7715 _Virtualize System and Inet interfaces_ #### Change overview - Convert `InterfaceId`, which is relatively heavily used and stored, to be platform-independent. Platform-specific parts move to a namespace `PlaformNetworkInterface` (since this includes a type and compile-time constant, it can't practically be made an abstract class). - Convert `InterfaceIterator` and `InterfaceAddressIterator` to use abstract base classes, for mockability. At present, the concrete implementation is fixed at compile time under the original name. - Split the three implementations into separate files. - Remove unused functions: - `InterfaceId::MatchLocalIPv6Subnet()` #### Testing CI; no change to outside functionality.

stale · 2022-09-05T17:47:36Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

stale · 2022-09-18T04:01:43Z

This stale issue has been automatically closed. Thank you for your contributions.

kpschoedel mentioned this issue Aug 5, 2021

Separate System::Layer and Timer into per-implementation files. #8800

Merged

This was referenced Aug 13, 2021

[Darwin] Fix Watchable Event Manager Select crash on shutdown #8985

Merged

[CI]: Fix random segmentation fault during event handling #9010

Merged

kpschoedel mentioned this issue Aug 17, 2021

Move socket watch APIs into System::Layer #9086

Merged

kpschoedel mentioned this issue Aug 17, 2021

MdnsAvahi destructor segfault; require separate shutdown #8001

Closed

kpschoedel mentioned this issue Aug 31, 2021

Virtualize System::Layer interfaces #9366

Merged

kpschoedel mentioned this issue Oct 4, 2021

Remove some dead Inet code #10211

Merged

kpschoedel mentioned this issue Oct 8, 2021

Move some code within src/inet files #10361

Merged

kpschoedel mentioned this issue Nov 26, 2021

Inet: Split InetLayer class #12291

Merged

kpschoedel mentioned this issue Dec 7, 2021

Inet: Split EndPoint into per-implementation files #12688

Merged

kpschoedel mentioned this issue Dec 8, 2021

[Inet] Remove InetLayer class #12727

Merged

kpschoedel mentioned this issue Dec 22, 2021

[Inet] Split off per-implementation parts of InetInterface #13228

Closed

woody-apple added the V1.X label Jan 26, 2022

stale bot added the stale Stale issue or PR label Sep 5, 2022

stale bot closed this as completed Sep 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Virtualize System and Inet interfaces #7715

Virtualize System and Inet interfaces #7715

andy31415 commented Jun 17, 2021

electrocucaracha commented Jun 23, 2021

kpschoedel commented Oct 12, 2021

stale bot commented Sep 5, 2022

stale bot commented Sep 18, 2022

Virtualize System and Inet interfaces #7715

Virtualize System and Inet interfaces #7715

Comments

andy31415 commented Jun 17, 2021

Problem

Proposed Solution

electrocucaracha commented Jun 23, 2021

kpschoedel commented Oct 12, 2021

stale bot commented Sep 5, 2022

stale bot commented Sep 18, 2022