[netif] refactor Netif and NcpSpinel (#2479)

`Netif::Ip6Send` depends on the implementation of `NcpSpinel`. And there are more methods to come. (For example, `SubcribeMulticastAddress`). Currently we pass the Ip6Send implementation to `Netif` in `Netif::Init` through a std::function object. But then, each time we add a new method, we need to update the signature of `Netif::Init`(making it very long) and the calling of `Netif::Init`, which is not preferred. This commit adds a dependencies class `NetifDependencies` to encapsulate all the implementation functions for Netif and let `NcpSpinel` implement this interface.
openthread · Sep 10, 2024 · 7f13bf6 · 7f13bf6
1 parent 697cb48
commit 7f13bf6
Show file tree

Hide file tree

Showing 5 changed files with 74 additions and 57 deletions.
diff --git a/src/ncp/ncp_host.cpp b/src/ncp/ncp_host.cpp
@@ -65,7 +65,7 @@ void NcpNetworkProperties::SetDeviceRole(otDeviceRole aRole)
 
 NcpHost::NcpHost(const char *aInterfaceName, bool aDryRun)
     : mSpinelDriver(*static_cast<ot::Spinel::SpinelDriver *>(otSysGetSpinelDriver()))
-    , mNetif()
+    , mNetif(mNcpSpinel)
 {
     memset(&mConfig, 0, sizeof(mConfig));
     mConfig.mInterfaceName = aInterfaceName;
@@ -82,8 +82,7 @@ void NcpHost::Init(void)
 {
     otSysInit(&mConfig);
     mNcpSpinel.Init(mSpinelDriver, *this);
-    mNetif.Init(mConfig.mInterfaceName,
-                [this](const uint8_t *aData, uint16_t aLength) { return mNcpSpinel.Ip6Send(aData, aLength); });
+    mNetif.Init(mConfig.mInterfaceName);
 
     mNcpSpinel.Ip6SetAddressCallback(
         [this](const std::vector<Ip6AddressInfo> &aAddrInfos) { mNetif.UpdateIp6UnicastAddresses(aAddrInfos); });

diff --git a/src/ncp/ncp_spinel.hpp b/src/ncp/ncp_spinel.hpp
@@ -50,6 +50,7 @@
 #include "common/task_runner.hpp"
 #include "common/types.hpp"
 #include "ncp/async_task.hpp"
+#include "ncp/posix/netif.hpp"
 
 namespace otbr {
 namespace Ncp {
@@ -80,7 +81,7 @@ class PropsObserver
  * The class provides methods for controlling the Thread stack on the network co-processor (NCP).
  *
  */
-class NcpSpinel
+class NcpSpinel : public Netif::Dependencies
 {
 public:
     using Ip6AddressTableCallback          = std::function<void(const std::vector<Ip6AddressInfo> &)>;
@@ -197,7 +198,7 @@ class NcpSpinel
      * @retval OTBR_ERROR_BUSY  NcpSpinel is busy with other requests.
      *
      */
-    otbrError Ip6Send(const uint8_t *aData, uint16_t aLength);
+    otbrError Ip6Send(const uint8_t *aData, uint16_t aLength) override;
 
     /**
      * This method enableds/disables the Thread network on the NCP.

diff --git a/src/ncp/posix/netif.cpp b/src/ncp/posix/netif.cpp
@@ -50,22 +50,27 @@
 
 namespace otbr {
 
-Netif::Netif(void)
+otbrError Netif::Dependencies::Ip6Send(const uint8_t *aData, uint16_t aLength)
+{
+    OTBR_UNUSED_VARIABLE(aData);
+    OTBR_UNUSED_VARIABLE(aLength);
+    return OTBR_ERROR_NONE;
+}
+
+Netif::Netif(Dependencies &aDependencies)
     : mTunFd(-1)
     , mIpFd(-1)
     , mNetlinkFd(-1)
     , mNetlinkSequence(0)
     , mNetifIndex(0)
+    , mDeps(aDependencies)
 {
 }
 
-otbrError Netif::Init(const std::string &aInterfaceName, const Ip6SendFunc &aIp6SendFunc)
+otbrError Netif::Init(const std::string &aInterfaceName)
 {
     otbrError error = OTBR_ERROR_NONE;
 
-    VerifyOrExit(aIp6SendFunc, error = OTBR_ERROR_INVALID_ARGS);
-    mIp6SendFunc = aIp6SendFunc;
-
     mIpFd = SocketWithCloseExec(AF_INET6, SOCK_DGRAM, IPPROTO_IP, kSocketNonBlock);
     VerifyOrExit(mIpFd >= 0, error = OTBR_ERROR_ERRNO);
 
@@ -256,10 +261,7 @@ void Netif::ProcessIp6Send(void)
 
     otbrLogInfo("Send packet (%hu bytes)", static_cast<uint16_t>(rval));
 
-    if (mIp6SendFunc != nullptr)
-    {
-        error = mIp6SendFunc(packet, rval);
-    }
+    error = mDeps.Ip6Send(packet, rval);
 exit:
     if (error == OTBR_ERROR_ERRNO)
     {
@@ -290,7 +292,6 @@ void Netif::Clear(void)
     mNetifIndex = 0;
     mIp6UnicastAddresses.clear();
     mIp6MulticastAddresses.clear();
-    mIp6SendFunc = nullptr;
 }
 
 } // namespace otbr
diff --git a/src/ncp/posix/netif.hpp b/src/ncp/posix/netif.hpp
@@ -49,11 +49,17 @@ namespace otbr {
 class Netif
 {
 public:
-    using Ip6SendFunc = std::function<otbrError(const uint8_t *, uint16_t)>;
+    class Dependencies
+    {
+    public:
+        virtual ~Dependencies(void) = default;
 
-    Netif(void);
+        virtual otbrError Ip6Send(const uint8_t *aData, uint16_t aLength);
+    };
 
-    otbrError Init(const std::string &aInterfaceName, const Ip6SendFunc &aIp6SendFunc);
+    Netif(Dependencies &aDependencies);
+
+    otbrError Init(const std::string &aInterfaceName);
     void      Deinit(void);
 
     void      Process(const MainloopContext *aContext);
@@ -89,7 +95,7 @@ class Netif
 
     std::vector<Ip6AddressInfo> mIp6UnicastAddresses;
     std::vector<Ip6Address>     mIp6MulticastAddresses;
-    Ip6SendFunc                 mIp6SendFunc;
+    Dependencies               &mDeps;
 };
 
 } // namespace otbr

diff --git a/tests/gtest/test_netif.cpp b/tests/gtest/test_netif.cpp
@@ -167,21 +167,16 @@ std::vector<std::string> GetAllIp6MulAddrs(const char *aInterfaceName)
     return ip6MulAddrs;
 }
 
-otbrError Ip6SendEmptyImpl(const uint8_t *aData, uint16_t aLength)
-{
-    OTBR_UNUSED_VARIABLE(aData);
-    OTBR_UNUSED_VARIABLE(aLength);
-    return OTBR_ERROR_NONE;
-}
+static otbr::Netif::Dependencies sDefaultNetifDependencies;
 
 TEST(Netif, WpanInitWithFullInterfaceName)
 {
     const char  *wpan = "wpan0";
     int          sockfd;
     struct ifreq ifr;
 
-    otbr::Netif netif;
-    EXPECT_EQ(netif.Init(wpan, Ip6SendEmptyImpl), OT_ERROR_NONE);
+    otbr::Netif netif(sDefaultNetifDependencies);
+    EXPECT_EQ(netif.Init(wpan), OT_ERROR_NONE);
 
     sockfd = socket(AF_INET, SOCK_DGRAM, 0);
     if (sockfd < 0)
@@ -204,8 +199,8 @@ TEST(Netif, WpanInitWithFormatInterfaceName)
     int          sockfd;
     struct ifreq ifr;
 
-    otbr::Netif netif;
-    EXPECT_EQ(netif.Init(wpan, Ip6SendEmptyImpl), OT_ERROR_NONE);
+    otbr::Netif netif(sDefaultNetifDependencies);
+    EXPECT_EQ(netif.Init(wpan), OT_ERROR_NONE);
 
     sockfd = socket(AF_INET, SOCK_DGRAM, 0);
     if (sockfd < 0)
@@ -227,8 +222,8 @@ TEST(Netif, WpanInitWithEmptyInterfaceName)
     int          sockfd;
     struct ifreq ifr;
 
-    otbr::Netif netif;
-    EXPECT_EQ(netif.Init("", Ip6SendEmptyImpl), OT_ERROR_NONE);
+    otbr::Netif netif(sDefaultNetifDependencies);
+    EXPECT_EQ(netif.Init(""), OT_ERROR_NONE);
 
     sockfd = socket(AF_INET, SOCK_DGRAM, 0);
     if (sockfd < 0)
@@ -248,8 +243,8 @@ TEST(Netif, WpanInitWithInvalidInterfaceName)
 {
     const char *invalid_netif_name = "invalid_netif_name";
 
-    otbr::Netif netif;
-    EXPECT_EQ(netif.Init(invalid_netif_name, Ip6SendEmptyImpl), OTBR_ERROR_INVALID_ARGS);
+    otbr::Netif netif(sDefaultNetifDependencies);
+    EXPECT_EQ(netif.Init(invalid_netif_name), OTBR_ERROR_INVALID_ARGS);
 }
 
 TEST(Netif, WpanMtuSize)
@@ -258,8 +253,8 @@ TEST(Netif, WpanMtuSize)
     int          sockfd;
     struct ifreq ifr;
 
-    otbr::Netif netif;
-    EXPECT_EQ(netif.Init(wpan, Ip6SendEmptyImpl), OT_ERROR_NONE);
+    otbr::Netif netif(sDefaultNetifDependencies);
+    EXPECT_EQ(netif.Init(wpan), OT_ERROR_NONE);
 
     sockfd = socket(AF_INET, SOCK_DGRAM, 0);
     if (sockfd < 0)
@@ -281,8 +276,8 @@ TEST(Netif, WpanDeinit)
     int          sockfd;
     struct ifreq ifr;
 
-    otbr::Netif netif;
-    EXPECT_EQ(netif.Init(wpan, Ip6SendEmptyImpl), OT_ERROR_NONE);
+    otbr::Netif netif(sDefaultNetifDependencies);
+    EXPECT_EQ(netif.Init(wpan), OT_ERROR_NONE);
 
     sockfd = socket(AF_INET, SOCK_DGRAM, 0);
     if (sockfd < 0)
@@ -300,8 +295,8 @@ TEST(Netif, WpanDeinit)
 
 TEST(Netif, WpanAddrGenMode)
 {
-    otbr::Netif netif;
-    EXPECT_EQ(netif.Init("wpan0", Ip6SendEmptyImpl), OT_ERROR_NONE);
+    otbr::Netif netif(sDefaultNetifDependencies);
+    EXPECT_EQ(netif.Init("wpan0"), OT_ERROR_NONE);
 
     std::fstream file("/proc/sys/net/ipv6/conf/wpan0/addr_gen_mode", std::ios::in);
     if (!file.is_open())
@@ -333,8 +328,8 @@ TEST(Netif, WpanIfHasCorrectUnicastAddresses_AfterUpdatingUnicastAddresses)
     const char *kMlRlocStr = "fd0d:7fc:a1b9:f050:0:ff:fe00:b800";
     const char *kMlAlocStr = "fd0d:7fc:a1b9:f050:0:ff:fe00:fc00";
 
-    otbr::Netif netif;
-    EXPECT_EQ(netif.Init(wpan, Ip6SendEmptyImpl), OT_ERROR_NONE);
+    otbr::Netif netif(sDefaultNetifDependencies);
+    EXPECT_EQ(netif.Init(wpan), OT_ERROR_NONE);
 
     otbr::Ip6AddressInfo testArray1[] = {
         {kLl, 64, 0, 1, 0},
@@ -377,8 +372,8 @@ TEST(Netif, WpanIfHasCorrectUnicastAddresses_AfterUpdatingUnicastAddresses)
 TEST(Netif, WpanIfHasCorrectMulticastAddresses_AfterUpdatingMulticastAddresses)
 {
     const char *wpan = "wpan0";
-    otbr::Netif netif;
-    EXPECT_EQ(netif.Init(wpan, Ip6SendEmptyImpl), OT_ERROR_NONE);
+    otbr::Netif netif(sDefaultNetifDependencies);
+    EXPECT_EQ(netif.Init(wpan), OT_ERROR_NONE);
 
     otbr::Ip6Address kDefaultMulAddr1 = {
         {0xff, 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01}};
@@ -438,9 +433,9 @@ TEST(Netif, WpanIfHasCorrectMulticastAddresses_AfterUpdatingMulticastAddresses)
 
 TEST(Netif, WpanIfStateChangesCorrectly_AfterSettingNetifState)
 {
-    otbr::Netif netif;
+    otbr::Netif netif(sDefaultNetifDependencies);
     const char *wpan = "wpan0";
-    EXPECT_EQ(netif.Init(wpan, Ip6SendEmptyImpl), OTBR_ERROR_NONE);
+    EXPECT_EQ(netif.Init(wpan), OTBR_ERROR_NONE);
 
     int fd = SocketWithCloseExec(AF_INET6, SOCK_DGRAM, IPPROTO_IP, kSocketNonBlock);
     if (fd < 0)
@@ -466,8 +461,8 @@ TEST(Netif, WpanIfStateChangesCorrectly_AfterSettingNetifState)
 
 TEST(Netif, WpanIfRecvIp6PacketCorrectly_AfterReceivingFromNetif)
 {
-    otbr::Netif netif;
-    EXPECT_EQ(netif.Init("wpan0", Ip6SendEmptyImpl), OTBR_ERROR_NONE);
+    otbr::Netif netif(sDefaultNetifDependencies);
+    EXPECT_EQ(netif.Init("wpan0"), OTBR_ERROR_NONE);
 
     const otIp6Address kOmr = {
         {0xfd, 0x2a, 0xc3, 0x0c, 0x87, 0xd3, 0x00, 0x01, 0xed, 0x1c, 0x0c, 0x91, 0xcc, 0xb6, 0x57, 0x8b}};
@@ -522,28 +517,43 @@ TEST(Netif, WpanIfRecvIp6PacketCorrectly_AfterReceivingFromNetif)
     netif.Deinit();
 }
 
-TEST(Netif, WpanIfSendIp6PacketCorrectly_AfterReceivingOnIf)
+class NetifDependencyTestIp6Send : public otbr::Netif::Dependencies
 {
-    bool        received = false;
-    std::string receivedPayload;
-    const char *hello = "Hello Otbr Netif!";
+public:
+    NetifDependencyTestIp6Send(bool &aReceived, std::string &aReceivedPayload)
+        : mReceived(aReceived)
+        , mReceivedPayload(aReceivedPayload)
+    {
+    }
 
-    auto Ip6SendTestImpl = [&received, &receivedPayload](const uint8_t *aData, uint16_t aLength) {
+    otbrError Ip6Send(const uint8_t *aData, uint16_t aLength) override
+    {
         const ip6_hdr *ipv6_header = reinterpret_cast<const ip6_hdr *>(aData);
         if (ipv6_header->ip6_nxt == IPPROTO_UDP)
         {
             const uint8_t *udpPayload    = aData + aLength - ntohs(ipv6_header->ip6_plen) + sizeof(udphdr);
             uint16_t       udpPayloadLen = ntohs(ipv6_header->ip6_plen) - sizeof(udphdr);
-            receivedPayload              = std::string(reinterpret_cast<const char *>(udpPayload), udpPayloadLen);
+            mReceivedPayload             = std::string(reinterpret_cast<const char *>(udpPayload), udpPayloadLen);
 
-            received = true;
+            mReceived = true;
         }
 
         return OTBR_ERROR_NONE;
-    };
+    }
+
+    bool        &mReceived;
+    std::string &mReceivedPayload;
+};
+
+TEST(Netif, WpanIfSendIp6PacketCorrectly_AfterReceivingOnIf)
+{
+    bool                       received = false;
+    std::string                receivedPayload;
+    NetifDependencyTestIp6Send netifDependency(received, receivedPayload);
+    const char                *hello = "Hello Otbr Netif!";
 
-    otbr::Netif netif;
-    EXPECT_EQ(netif.Init("wpan0", Ip6SendTestImpl), OT_ERROR_NONE);
+    otbr::Netif netif(netifDependency);
+    EXPECT_EQ(netif.Init("wpan0"), OT_ERROR_NONE);
 
     // OMR Prefix: fd76:a5d1:fcb0:1707::/64
     const otIp6Address kOmr = {