From: Andrei Pavel <andrei@isc.org>
Date: Sat, 13 May 2023 12:06:35 +0000 (+0300)
Subject: [#2817] use unsigned int for ifindex throughout
X-Git-Tag: Kea-2.3.8~145
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=19f1217521b085dbf8e7fd7092a85db1b26ddf17;p=thirdparty%2Fkea.git

[#2817] use unsigned int for ifindex throughout

which is what if_nametoindex returns
---

diff --git a/src/bin/dhcp4/tests/direct_client_unittest.cc b/src/bin/dhcp4/tests/direct_client_unittest.cc
index 3d98e4ec7e..b6145974a1 100644
--- a/src/bin/dhcp4/tests/direct_client_unittest.cc
+++ b/src/bin/dhcp4/tests/direct_client_unittest.cc
@@ -83,7 +83,7 @@ public:
     /// @return Generated message.
     Pkt4Ptr createClientMessage(const uint16_t msg_type,
                                 const std::string& iface,
-                                const uint32_t ifindex);
+                                const unsigned int ifindex);
 
     /// @brief Creates simple message from a client.
     ///
@@ -102,7 +102,7 @@ public:
     /// @return Configured and parsed message.
     Pkt4Ptr createClientMessage(const Pkt4Ptr &msg,
                                 const std::string& iface,
-                                const uint32_t ifindex);
+                                const unsigned int ifindex);
 
     /// @brief This test checks that the message from directly connected client
     /// is processed and that client is offered IPv4 address from the subnet
@@ -196,7 +196,7 @@ DirectClientTest::configureTwoSubnets(const std::string& prefix1,
 Pkt4Ptr
 DirectClientTest::createClientMessage(const uint16_t msg_type,
                                       const std::string& iface,
-                                      const uint32_t ifindex) {
+                                      const unsigned int ifindex) {
     // Create a source packet.
     Pkt4Ptr msg = Pkt4Ptr(new Pkt4(msg_type, 1234));
     return (createClientMessage(msg, iface, ifindex));
@@ -206,7 +206,7 @@ DirectClientTest::createClientMessage(const uint16_t msg_type,
 Pkt4Ptr
 DirectClientTest::createClientMessage(const Pkt4Ptr& msg,
                                       const std::string& iface,
-                                      const uint32_t ifindex) {
+                                      const unsigned int ifindex) {
     msg->setRemoteAddr(IOAddress("255.255.255.255"));
     msg->addOption(generateClientId());
     msg->setIface(iface);
diff --git a/src/bin/dhcp6/tests/dhcp6_test_utils.h b/src/bin/dhcp6/tests/dhcp6_test_utils.h
index 93aadb968d..35a11280bd 100644
--- a/src/bin/dhcp6/tests/dhcp6_test_utils.h
+++ b/src/bin/dhcp6/tests/dhcp6_test_utils.h
@@ -4,7 +4,7 @@
 // License, v. 2.0. If a copy of the MPL was not distributed with this
 // file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
-/// @file   dhcp6_test_utils.h
+/// @file dhcp6_test_utils.h
 ///
 /// @brief This file contains utility classes used for DHCPv6 server testing
 
@@ -519,7 +519,7 @@ public:
     std::string valid_iface_;
 
     // Index of a valid network interface
-    uint32_t valid_ifindex_;
+    unsigned int valid_ifindex_;
 };
 
 // We need to pass one reference to the Dhcp6Client, which is defined in
diff --git a/src/bin/perfdhcp/perf_socket.h b/src/bin/perfdhcp/perf_socket.h
index 56a8551a27..e0c45734b8 100644
--- a/src/bin/perfdhcp/perf_socket.h
+++ b/src/bin/perfdhcp/perf_socket.h
@@ -26,7 +26,7 @@ namespace perfdhcp {
 class BasePerfSocket : public dhcp::SocketInfo {
 public:
     /// Interface index.
-    uint16_t ifindex_;
+    unsigned int ifindex_;
 
     /// \brief Default constructor of BasePerfSocket.
     BasePerfSocket() :
diff --git a/src/lib/dhcp/iface_mgr.cc b/src/lib/dhcp/iface_mgr.cc
index ebb5bf0a9b..6bf1a0853f 100644
--- a/src/lib/dhcp/iface_mgr.cc
+++ b/src/lib/dhcp/iface_mgr.cc
@@ -1,4 +1,4 @@
-// Copyright (C) 2011-2022 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2011-2023 Internet Systems Consortium, Inc. ("ISC")
 //
 // This Source Code Form is subject to the terms of the Mozilla Public
 // License, v. 2.0. If a copy of the MPL was not distributed with this
@@ -832,7 +832,7 @@ IfaceMgr::printIfaces(std::ostream& out /*= std::cout*/) {
 }
 
 IfacePtr
-IfaceCollection::getIface(uint32_t ifindex) {
+IfaceCollection::getIface(const unsigned int ifindex) {
     return (getIfaceInternal(ifindex, MultiThreadingMgr::instance().getMode()));
 }
 
@@ -843,7 +843,10 @@ IfaceCollection::getIface(const std::string& ifname) {
 }
 
 IfacePtr
-IfaceCollection::getIfaceInternal(uint32_t ifindex, bool need_lock) {
+IfaceCollection::getIfaceInternal(const unsigned int ifindex, const bool need_lock) {
+    if (ifindex == UNSET_IFINDEX) {
+        isc_throw(BadValue, "interface index was not set");
+    }
     if (need_lock) {
         lock_guard<mutex> lock(mutex_);
         if (cache_ && (cache_->getIndex() == ifindex)) {
@@ -864,14 +867,13 @@ IfaceCollection::getIfaceInternal(uint32_t ifindex, bool need_lock) {
         cache_ = *it;
         return (cache_);
     } else {
-        lock_guard<mutex> lock(mutex_);
         cache_ = *it;
         return (cache_);
     }
 }
 
 IfacePtr
-IfaceCollection::getIfaceInternal(const std::string& ifname, bool need_lock) {
+IfaceCollection::getIfaceInternal(const std::string& ifname, const bool need_lock) {
     if (need_lock) {
         lock_guard<mutex> lock(mutex_);
         if (cache_ && (cache_->getName() == ifname)) {
@@ -892,17 +894,13 @@ IfaceCollection::getIfaceInternal(const std::string& ifname, bool need_lock) {
         cache_ = *it;
         return (cache_);
     } else {
-        lock_guard<mutex> lock(mutex_);
         cache_ = *it;
         return (cache_);
     }
 }
 
 IfacePtr
-IfaceMgr::getIface(int ifindex) {
-    if ((ifindex < 0) || (ifindex > std::numeric_limits<int32_t>::max())) {
-        return (IfacePtr()); // out of range
-    }
+IfaceMgr::getIface(const unsigned int ifindex) {
     return (ifaces_.getIface(ifindex));
 }
 
@@ -1210,7 +1208,7 @@ Pkt4Ptr IfaceMgr::receive4Indirect(uint32_t timeout_sec, uint32_t timeout_usec /
         return (Pkt4Ptr());
     } else if (result < 0) {
         // In most cases we would like to know whether select() returned
-        // an error because of a signal being received  or for some other
+        // an error because of a signal being received or for some other
         // reason. This is because DHCP servers use signals to trigger
         // certain actions, like reconfiguration or graceful shutdown.
         // By catching a dedicated exception the caller will know if the
@@ -1329,7 +1327,7 @@ Pkt4Ptr IfaceMgr::receive4Direct(uint32_t timeout_sec, uint32_t timeout_usec /*
 
     } else if (result < 0) {
         // In most cases we would like to know whether select() returned
-        // an error because of a signal being received  or for some other
+        // an error because of a signal being received or for some other
         // reason. This is because DHCP servers use signals to trigger
         // certain actions, like reconfiguration or graceful shutdown.
         // By catching a dedicated exception the caller will know if the
@@ -1476,7 +1474,7 @@ IfaceMgr::receive6Direct(uint32_t timeout_sec, uint32_t timeout_usec /* = 0 */ )
 
     } else if (result < 0) {
         // In most cases we would like to know whether select() returned
-        // an error because of a signal being received  or for some other
+        // an error because of a signal being received or for some other
         // reason. This is because DHCP servers use signals to trigger
         // certain actions, like reconfiguration or graceful shutdown.
         // By catching a dedicated exception the caller will know if the
@@ -1600,7 +1598,7 @@ IfaceMgr::receive6Indirect(uint32_t timeout_sec, uint32_t timeout_usec /* = 0 */
         return (Pkt6Ptr());
     } else if (result < 0) {
         // In most cases we would like to know whether select() returned
-        // an error because of a signal being received  or for some other
+        // an error because of a signal being received or for some other
         // reason. This is because DHCP servers use signals to trigger
         // certain actions, like reconfiguration or graceful shutdown.
         // By catching a dedicated exception the caller will know if the
diff --git a/src/lib/dhcp/iface_mgr.h b/src/lib/dhcp/iface_mgr.h
index c0ee4f0308..bccfeee01c 100644
--- a/src/lib/dhcp/iface_mgr.h
+++ b/src/lib/dhcp/iface_mgr.h
@@ -1,4 +1,4 @@
-// Copyright (C) 2011-2022 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2011-2023 Internet Systems Consortium, Inc. ("ISC")
 //
 // This Source Code Form is subject to the terms of the Mozilla Public
 // License, v. 2.0. If a copy of the MPL was not distributed with this
@@ -216,7 +216,7 @@ public:
     /// @brief Returns interface index.
     ///
     /// @return interface index
-    int getIndex() const { return ifindex_; }
+    unsigned int getIndex() const { return ifindex_; }
 
     /// @brief Returns interface name.
     ///
@@ -416,7 +416,7 @@ protected:
     std::string name_;
 
     /// Interface index (a value that uniquely identifies an interface).
-    int ifindex_;
+    unsigned int ifindex_;
 
     /// List of assigned addresses.
     AddressCollection addrs_;
@@ -511,7 +511,7 @@ public:
             boost::multi_index::hashed_unique<
                 // Use the interface index as the key.
                 boost::multi_index::const_mem_fun<
-                    Iface, int, &Iface::getIndex
+                    Iface, unsigned int, &Iface::getIndex
                 >
             >,
             // Start definition of index #2.
@@ -571,7 +571,7 @@ public:
     ///
     /// @param ifindex The index of the interface to find.
     /// @return The interface with the index or null.
-    IfacePtr getIface(uint32_t ifindex);
+    IfacePtr getIface(const unsigned int ifindex);
 
     /// @brief Lookup by interface name.
     ///
@@ -585,7 +585,7 @@ private:
     /// @param ifindex The index of the interface to find.
     /// @param need_lock True when the cache operation needs to hold the mutex.
     /// @return The interface with the index or null.
-    IfacePtr getIfaceInternal(uint32_t ifindex, bool need_lock);
+    IfacePtr getIfaceInternal(const unsigned int ifindex, const bool need_lock);
 
     /// @brief Lookup by interface name.
     ///
@@ -594,7 +594,7 @@ private:
     /// @param ifname The name of the interface to find.
     /// @param need_lock True when the cache operation needs to hold the mutex.
     /// @return The interface with the name or null.
-    IfacePtr getIfaceInternal(const std::string& ifname, bool need_lock);
+    IfacePtr getIfaceInternal(const std::string& ifname, const bool need_lock);
 
     /// @brief The mutex for protecting the cache from concurrent
     /// access from packet processing threads.
@@ -749,7 +749,7 @@ public:
     /// @return interface with requested index (or null if no such
     ///         interface is present)
     ///
-    IfacePtr getIface(int ifindex);
+    IfacePtr getIface(const unsigned int ifindex);
 
     /// @brief Returns interface with specified interface name
     ///
diff --git a/src/lib/dhcp/pkt.cc b/src/lib/dhcp/pkt.cc
index fdd1e8977e..6f2555f352 100644
--- a/src/lib/dhcp/pkt.cc
+++ b/src/lib/dhcp/pkt.cc
@@ -1,4 +1,4 @@
-// Copyright (C) 2014-2022 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2014-2023 Internet Systems Consortium, Inc. ("ISC")
 //
 // This Source Code Form is subject to the terms of the Mozilla Public
 // License, v. 2.0. If a copy of the MPL was not distributed with this
@@ -17,7 +17,7 @@ namespace dhcp {
 Pkt::Pkt(uint32_t transid, const isc::asiolink::IOAddress& local_addr,
          const isc::asiolink::IOAddress& remote_addr, uint16_t local_port,
          uint16_t remote_port)
-    : transid_(transid), iface_(""), ifindex_(-1), local_addr_(local_addr),
+    : transid_(transid), iface_(""), ifindex_(UNSET_IFINDEX), local_addr_(local_addr),
       remote_addr_(remote_addr), local_port_(local_port),
       remote_port_(remote_port), buffer_out_(0), copy_retrieved_options_(false) {
 }
@@ -25,7 +25,7 @@ Pkt::Pkt(uint32_t transid, const isc::asiolink::IOAddress& local_addr,
 Pkt::Pkt(const uint8_t* buf, uint32_t len, const isc::asiolink::IOAddress& local_addr,
          const isc::asiolink::IOAddress& remote_addr, uint16_t local_port,
          uint16_t remote_port)
-    : transid_(0), iface_(""), ifindex_(-1), local_addr_(local_addr),
+    : transid_(0), iface_(""), ifindex_(UNSET_IFINDEX), local_addr_(local_addr),
       remote_addr_(remote_addr), local_port_(local_port),
       remote_port_(remote_port), buffer_out_(0), copy_retrieved_options_(false) {
     if (len != 0) {
diff --git a/src/lib/dhcp/pkt.h b/src/lib/dhcp/pkt.h
index 441fd39598..3a2a35a301 100644
--- a/src/lib/dhcp/pkt.h
+++ b/src/lib/dhcp/pkt.h
@@ -1,4 +1,4 @@
-// Copyright (C) 2014-2022 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2014-2023 Internet Systems Consortium, Inc. ("ISC")
 //
 // This Source Code Form is subject to the terms of the Mozilla Public
 // License, v. 2.0. If a copy of the MPL was not distributed with this
@@ -17,12 +17,18 @@
 #include <boost/date_time/posix_time/posix_time.hpp>
 #include <boost/shared_ptr.hpp>
 
+#include <limits>
 #include <utility>
 
 namespace isc {
 
 namespace dhcp {
 
+/// @brief A value used to signal that the interface index was not set.
+/// That means that more than UNSET_IFINDEX interfaces are not supported.
+/// That's fine, since it would have overflowed with UNSET_IFINDEX + 1 anyway.
+constexpr unsigned int UNSET_IFINDEX = std::numeric_limits<unsigned int>::max();
+
 /// @brief RAII object enabling copying options retrieved from the
 /// packet.
 ///
@@ -524,13 +530,13 @@ public:
     /// @brief Sets interface index.
     ///
     /// @param ifindex specifies interface index.
-    void setIndex(int ifindex) {
+    void setIndex(const unsigned int ifindex) {
         ifindex_ = ifindex;
     }
 
     /// @brief Resets interface index to negative value.
     void resetIndex() {
-        ifindex_ = -1;
+        ifindex_ = UNSET_IFINDEX;
     }
 
     /// @brief Returns interface index.
@@ -544,7 +550,7 @@ public:
     ///
     /// @return true if interface index set, false otherwise.
     bool indexSet() const {
-        return (ifindex_ >= 0);
+        return (ifindex_ != UNSET_IFINDEX);
     }
 
     /// @brief Returns interface name.
@@ -789,7 +795,7 @@ protected:
     /// Each network interface has assigned an unique ifindex.
     /// It is a functional equivalent of a name, but sometimes more useful, e.g.
     /// when using odd systems that allow spaces in interface names.
-    int64_t ifindex_;
+    unsigned int ifindex_;
 
     /// @brief Local IP (v4 or v6) address.
     ///
diff --git a/src/lib/dhcp/pkt_filter_inet6.cc b/src/lib/dhcp/pkt_filter_inet6.cc
index 20d682cfbe..e8e7ed0950 100644
--- a/src/lib/dhcp/pkt_filter_inet6.cc
+++ b/src/lib/dhcp/pkt_filter_inet6.cc
@@ -182,7 +182,7 @@ PktFilterInet6::receive(const SocketInfo& socket_info) {
     struct in6_addr to_addr;
     memset(&to_addr, 0, sizeof(to_addr));
 
-    int ifindex = -1;
+    unsigned int ifindex = UNSET_IFINDEX;
     if (result >= 0) {
         struct in6_pktinfo* pktinfo = NULL;
 
diff --git a/src/lib/dhcp/tests/iface_mgr_test_config.cc b/src/lib/dhcp/tests/iface_mgr_test_config.cc
index bd5d20778f..a36786734c 100644
--- a/src/lib/dhcp/tests/iface_mgr_test_config.cc
+++ b/src/lib/dhcp/tests/iface_mgr_test_config.cc
@@ -1,4 +1,4 @@
-// Copyright (C) 2014-2022 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2014-2023 Internet Systems Consortium, Inc. ("ISC")
 //
 // This Source Code Form is subject to the terms of the Mozilla Public
 // License, v. 2.0. If a copy of the MPL was not distributed with this
@@ -67,12 +67,12 @@ IfaceMgrTestConfig::addIface(const IfacePtr& iface) {
 }
 
 void
-IfaceMgrTestConfig::addIface(const std::string& name, const int ifindex) {
+IfaceMgrTestConfig::addIface(const std::string& name, const unsigned int ifindex) {
     IfaceMgr::instance().addInterface(createIface(name, ifindex));
 }
 
 IfacePtr
-IfaceMgrTestConfig::createIface(const std::string &name, const int ifindex) {
+IfaceMgrTestConfig::createIface(const std::string &name, const unsigned int ifindex) {
     IfacePtr iface(new Iface(name, ifindex));
     if (name == "lo") {
         iface->flag_loopback_ = true;
diff --git a/src/lib/dhcp/tests/iface_mgr_test_config.h b/src/lib/dhcp/tests/iface_mgr_test_config.h
index 26167e8f2d..2b13a996e8 100644
--- a/src/lib/dhcp/tests/iface_mgr_test_config.h
+++ b/src/lib/dhcp/tests/iface_mgr_test_config.h
@@ -177,7 +177,7 @@ public:
     ///
     /// @param name Name of the new interface.
     /// @param ifindex Index for a new interface.
-    void addIface(const std::string& name, const int ifindex);
+    void addIface(const std::string& name, const unsigned int ifindex);
 
     /// @brief Create an object representing interface.
     ///
@@ -198,7 +198,7 @@ public:
     /// @param ifindex An index of the interface to be created.
     ///
     /// @return An object representing interface.
-    static IfacePtr createIface(const std::string& name, const int ifindex);
+    static IfacePtr createIface(const std::string& name, const unsigned int ifindex);
 
     /// @brief Creates a default (example) set of fake interfaces.
     void createIfaces();
diff --git a/src/lib/dhcp/tests/iface_mgr_unittest.cc b/src/lib/dhcp/tests/iface_mgr_unittest.cc
index 41611fa9b1..2761319b30 100644
--- a/src/lib/dhcp/tests/iface_mgr_unittest.cc
+++ b/src/lib/dhcp/tests/iface_mgr_unittest.cc
@@ -1,4 +1,4 @@
-// Copyright (C) 2011-2022 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2011-2023 Internet Systems Consortium, Inc. ("ISC")
 //
 // This Source Code Form is subject to the terms of the Mozilla Public
 // License, v. 2.0. If a copy of the MPL was not distributed with this
@@ -271,7 +271,7 @@ public:
     /// @param ifindex An index of the interface to be created.
     ///
     /// @return An object representing interface.
-    static IfacePtr createIface(const std::string& name, const int ifindex) {
+    static IfacePtr createIface(const std::string& name, const unsigned int ifindex) {
         IfacePtr iface(new Iface(name, ifindex));
         if (name == "lo") {
             iface->flag_loopback_ = true;
@@ -941,6 +941,46 @@ TEST_F(IfaceMgrTest, ifaceClass) {
     EXPECT_EQ(66666, iface->getIndex());
 }
 
+// This test checks the getIface by index method.
+TEST_F(IfaceMgrTest, getIfaceByIndex) {
+    NakedIfaceMgr ifacemgr;
+
+    // Create a set of fake interfaces. At the same time, remove the actual
+    // interfaces that have been detected by the IfaceMgr.
+    ifacemgr.createIfaces();
+
+    // Getting an unset index should throw.
+    EXPECT_THROW_MSG(ifacemgr.getIface(UNSET_IFINDEX), BadValue, "interface index was not set");
+
+    // Historically -1 was used as an unset value. Let's also check that it throws in case we didn't
+    // migrate all code to UNSET_IFINDEX and in case the values diverge.
+    EXPECT_THROW_MSG(ifacemgr.getIface(-1), BadValue, "interface index was not set");
+
+    // Get the first interface defined.
+    IfacePtr iface(ifacemgr.getIface(0));
+    ASSERT_TRUE(iface);
+    EXPECT_EQ("lo", iface->getName());
+
+    // Attemt to get an undefined interface.
+    iface = ifacemgr.getIface(3);
+    EXPECT_FALSE(iface);
+
+    // Check that we can go past INT_MAX.
+    unsigned int int_max(numeric_limits<int>::max());
+    iface = ifacemgr.getIface(int_max);
+    EXPECT_FALSE(iface);
+    iface = ifacemgr.createIface("wlan0", int_max);
+    ifacemgr.addInterface(iface);
+    iface = ifacemgr.getIface(int_max);
+    EXPECT_TRUE(iface);
+    iface = ifacemgr.getIface(int_max + 1);
+    EXPECT_FALSE(iface);
+    iface = ifacemgr.createIface("wlan1", int_max + 1);
+    ifacemgr.addInterface(iface);
+    iface = ifacemgr.getIface(int_max + 1);
+    EXPECT_TRUE(iface);
+}
+
 // This test checks the getIface by packet method.
 TEST_F(IfaceMgrTest, getIfaceByPkt) {
     NakedIfaceMgr ifacemgr;
@@ -991,9 +1031,9 @@ TEST_F(IfaceMgrTest, getIfaceByPkt) {
     EXPECT_FALSE(pkt6->indexSet());
 
     // Test that you can also reset the index via setIndex().
-    pkt4->setIndex(-1);
+    pkt4->setIndex(UNSET_IFINDEX);
     EXPECT_FALSE(pkt4->indexSet());
-    pkt6->setIndex(-1);
+    pkt6->setIndex(UNSET_IFINDEX);
     EXPECT_FALSE(pkt6->indexSet());
 }
 
diff --git a/src/lib/dhcp/tests/pkt_filter6_test_utils.h b/src/lib/dhcp/tests/pkt_filter6_test_utils.h
index 398b3a1ac1..dc683cf6ce 100644
--- a/src/lib/dhcp/tests/pkt_filter6_test_utils.h
+++ b/src/lib/dhcp/tests/pkt_filter6_test_utils.h
@@ -74,7 +74,7 @@ public:
     void testRcvdMessage(const Pkt6Ptr& rcvd_msg) const;
 
     std::string ifname_;   ///< Loopback interface name.
-    uint16_t ifindex_;     ///< Loopback interface index.
+    unsigned int ifindex_; ///< Loopback interface index.
     uint16_t port_;        ///< A port number used for the test.
     isc::dhcp::SocketInfo sock_info_; ///< A structure holding socket info.
     int send_msg_sock_;    ///< Holds a descriptor of the socket used by
diff --git a/src/lib/dhcp/tests/pkt_filter_test_utils.h b/src/lib/dhcp/tests/pkt_filter_test_utils.h
index 253df184d2..c7ebb263fc 100644
--- a/src/lib/dhcp/tests/pkt_filter_test_utils.h
+++ b/src/lib/dhcp/tests/pkt_filter_test_utils.h
@@ -83,7 +83,7 @@ public:
     void testRcvdMessageAddressPort(const Pkt4Ptr& rcvd_msg) const;
 
     std::string ifname_;   ///< Loopback interface name
-    uint16_t ifindex_;     ///< Loopback interface index.
+    unsigned int ifindex_; ///< Loopback interface index.
     uint16_t port_;        ///< A port number used for the test.
     isc::dhcp::SocketInfo sock_info_; ///< A structure holding socket info.
     int send_msg_sock_;    ///< Holds a descriptor of the socket used by