ipaddress: Refactor object creation
Change-Id: Icae934e3dc88d596812b75a31598636ae7b95823
Signed-off-by: William A. Kennington III <wak@google.com>
diff --git a/src/ethernet_interface.cpp b/src/ethernet_interface.cpp
index 9795a7e..49a6eb6 100644
--- a/src/ethernet_interface.cpp
+++ b/src/ethernet_interface.cpp
@@ -92,7 +92,7 @@
Ifaces(bus, objPath.c_str(),
emitSignal ? Ifaces::action::defer_emit
: Ifaces::action::emit_no_signals),
- bus(bus), manager(manager), objPath(std::move(objPath)), ifIdx(info.idx)
+ manager(manager), bus(bus), objPath(std::move(objPath)), ifIdx(info.idx)
{
interfaceName(*info.name);
auto dhcpVal = getDHCPValue(config);
@@ -160,32 +160,6 @@
}
}
-static IP::Protocol getProtocol(const InAddrAny& addr)
-{
- if (std::holds_alternative<in_addr>(addr))
- {
- return IP::Protocol::IPv4;
- }
- else if (std::holds_alternative<in6_addr>(addr))
- {
- return IP::Protocol::IPv6;
- }
-
- throw std::runtime_error("Invalid addr type");
-}
-
-bool EthernetInterface::dhcpIsEnabled(IP::Protocol family)
-{
- switch (family)
- {
- case IP::Protocol::IPv6:
- return dhcp6();
- case IP::Protocol::IPv4:
- return dhcp4();
- }
- throw std::logic_error("Unreachable");
-}
-
bool EthernetInterface::originIsManuallyAssigned(IP::AddressOrigin origin)
{
return (
@@ -212,10 +186,9 @@
{
continue;
}
- auto address = std::to_string(addr.address);
- IP::Protocol addressType = getProtocol(addr.address);
+ auto ifaddr = IfAddr(addr.address, addr.prefix);
IP::AddressOrigin origin = IP::AddressOrigin::Static;
- if (dhcpIsEnabled(addressType))
+ if (dhcpIsEnabled(addr.address))
{
origin = IP::AddressOrigin::DHCP;
}
@@ -226,13 +199,9 @@
}
#endif
- auto ipAddressObjectPath =
- generateObjectPath(addressType, address, addr.prefix, origin);
-
this->addrs.insert_or_assign(
- address, std::make_unique<IPAddress>(bus, ipAddressObjectPath,
- *this, addressType, address,
- origin, addr.prefix));
+ ifaddr, std::make_unique<IPAddress>(bus, std::string_view(objPath),
+ *this, ifaddr, origin));
}
}
@@ -262,56 +231,54 @@
ObjectPath EthernetInterface::ip(IP::Protocol protType, std::string ipaddress,
uint8_t prefixLength, std::string)
{
- if (dhcpIsEnabled(protType))
+ InAddrAny addr;
+ try
{
- log<level::INFO>("DHCP enabled on the interface, disabling"),
- entry("INTERFACE=%s", interfaceName().c_str());
switch (protType)
{
case IP::Protocol::IPv4:
- dhcp4(false);
+ addr = ToAddr<in_addr>{}(ipaddress);
break;
case IP::Protocol::IPv6:
- dhcp6(false);
+ addr = ToAddr<in6_addr>{}(ipaddress);
break;
+ default:
+ throw std::logic_error("Exhausted protocols");
}
- // Delete the IP address object and that reloads the networkd
- // to allow the same IP address to be set as Static IP
- deleteObject(ipaddress);
}
-
- IP::AddressOrigin origin = IP::AddressOrigin::Static;
-
- int addressFamily = (protType == IP::Protocol::IPv4) ? AF_INET : AF_INET6;
-
- if (!isValidIP(addressFamily, ipaddress))
+ catch (const std::exception& e)
{
- log<level::ERR>("Not a valid IP address"),
- entry("ADDRESS=%s", ipaddress.c_str());
+ auto msg = fmt::format("Invalid IP `{}`: {}\n", ipaddress, e.what());
+ log<level::ERR>(msg.c_str(), entry("ADDRESS=%s", ipaddress.c_str()));
elog<InvalidArgument>(Argument::ARGUMENT_NAME("ipaddress"),
Argument::ARGUMENT_VALUE(ipaddress.c_str()));
}
-
- if (!isValidPrefix(addressFamily, prefixLength))
+ IfAddr ifaddr;
+ try
{
- log<level::ERR>("PrefixLength is not correct "),
- entry("PREFIXLENGTH=%" PRIu8, prefixLength);
+ ifaddr = {addr, prefixLength};
+ }
+ catch (const std::exception& e)
+ {
+ auto msg = fmt::format("Invalid prefix length `{}`: {}\n", prefixLength,
+ e.what());
+ log<level::ERR>(msg.c_str(),
+ entry("PREFIXLENGTH=%" PRIu8, prefixLength));
elog<InvalidArgument>(
Argument::ARGUMENT_NAME("prefixLength"),
Argument::ARGUMENT_VALUE(std::to_string(prefixLength).c_str()));
}
- auto objectPath =
- generateObjectPath(protType, ipaddress, prefixLength, origin);
- this->addrs.insert_or_assign(
- ipaddress,
- std::make_unique<IPAddress>(bus, objectPath, *this, protType, ipaddress,
- origin, prefixLength));
+ auto obj =
+ std::make_unique<IPAddress>(bus, std::string_view(objPath), *this,
+ ifaddr, IP::AddressOrigin::Static);
+ auto path = obj->getObjPath();
+ this->addrs.insert_or_assign(ifaddr, std::move(obj));
writeConfigurationFile();
manager.reloadConfigs();
- return objectPath;
+ return path;
}
ObjectPath EthernetInterface::neighbor(std::string ipAddress,
@@ -344,20 +311,6 @@
return objectPath;
}
-void EthernetInterface::deleteObject(std::string_view ipaddress)
-{
- auto it = addrs.find(ipaddress);
- if (it == addrs.end())
- {
- log<level::ERR>("DeleteObject:Unable to find the object.");
- return;
- }
- this->addrs.erase(it);
-
- writeConfigurationFile();
- manager.reloadConfigs();
-}
-
void EthernetInterface::deleteStaticNeighborObject(std::string_view ipAddress)
{
auto it = staticNeighbors.find(ipAddress);
@@ -851,10 +804,9 @@
}
{
auto& address = network["Address"];
- for (const auto& addr : getAddresses())
+ for (const auto& addr : addrs)
{
- if (originIsManuallyAssigned(addr.second->origin()) &&
- !dhcpIsEnabled(addr.second->type()))
+ if (originIsManuallyAssigned(addr.second->origin()))
{
address.emplace_back(
fmt::format("{}/{}", addr.second->address(),
diff --git a/src/ethernet_interface.hpp b/src/ethernet_interface.hpp
index 462937f..d7342a8 100644
--- a/src/ethernet_interface.hpp
+++ b/src/ethernet_interface.hpp
@@ -91,6 +91,12 @@
bool emitSignal = true,
std::optional<bool> enabled = std::nullopt);
+ /** @brief Network Manager object. */
+ Manager& manager;
+
+ /** @brief Persistent map of IPAddress dbus objects and their names */
+ std::unordered_map<IfAddr, std::unique_ptr<IPAddress>> addrs;
+
/** @brief Updates the interface information based on new InterfaceInfo */
void updateInfo(const system::InterfaceInfo& info);
@@ -120,11 +126,6 @@
/* @brief delete the dbus object of the given ipAddress.
* @param[in] ipAddress - IP address.
*/
- void deleteObject(std::string_view ipAddress);
-
- /* @brief delete the dbus object of the given ipAddress.
- * @param[in] ipAddress - IP address.
- */
void deleteStaticNeighborObject(std::string_view ipAddress);
/* @brief creates the dbus object(IPaddres) given in the address list.
@@ -137,14 +138,6 @@
*/
void createStaticNeighborObjects();
- /* @brief Gets all the ip addresses.
- * @returns the list of ipAddress.
- */
- inline const auto& getAddresses() const
- {
- return addrs;
- }
-
/* @brief Gets all the static neighbor entries.
* @returns Static neighbor map.
*/
@@ -161,6 +154,19 @@
using EthernetInterfaceIntf::dhcp6;
bool dhcp6(bool value) override;
+ inline bool dhcpIsEnabled(in_addr) const
+ {
+ return dhcp4();
+ }
+ inline bool dhcpIsEnabled(in6_addr) const
+ {
+ return dhcp6();
+ }
+ inline bool dhcpIsEnabled(InAddrAny addr) const
+ {
+ return std::visit([&](auto v) { return dhcpIsEnabled(v); }, addr);
+ }
+
/** Retrieve Link State */
bool linkUp() const override;
@@ -264,12 +270,6 @@
/** @brief Persistent sdbusplus DBus bus connection. */
sdbusplus::bus_t& bus;
- /** @brief Network Manager object. */
- Manager& manager;
-
- /** @brief Persistent map of IPAddress dbus objects and their names */
- string_umap<std::unique_ptr<IPAddress>> addrs;
-
/** @brief Persistent map of Neighbor dbus objects and their names */
string_umap<std::unique_ptr<Neighbor>> staticNeighbors;
@@ -299,12 +299,6 @@
const config::Parser& config, bool emitSignal,
std::optional<bool> enabled);
- /** @brief Determines if DHCP is active for the IP::Protocol supplied.
- * @param[in] protocol - Either IPv4 or IPv6
- * @returns true/false value if DHCP is active for the input protocol
- */
- bool dhcpIsEnabled(IP::Protocol protocol);
-
/** @brief Determines if the address is manually assigned
* @param[in] origin - The origin entry of the IP::Address
* @returns true/false value if the address is static
diff --git a/src/ipaddress.cpp b/src/ipaddress.cpp
index 56bec97..647c37e 100644
--- a/src/ipaddress.cpp
+++ b/src/ipaddress.cpp
@@ -2,6 +2,7 @@
#include "ethernet_interface.hpp"
#include "netlink.hpp"
+#include "network_manager.hpp"
#include "util.hpp"
#include <linux/netlink.h>
@@ -38,17 +39,48 @@
using NotAllowed = sdbusplus::xyz::openbmc_project::Common::Error::NotAllowed;
using Reason = xyz::openbmc_project::Common::NotAllowed::REASON;
-IPAddress::IPAddress(sdbusplus::bus_t& bus, stdplus::const_zstring objPath,
- EthernetInterface& parent, IP::Protocol type,
- std::string_view ipaddress, IP::AddressOrigin origin,
- uint8_t prefixLength) :
- IPIfaces(bus, objPath.c_str(), IPIfaces::action::defer_emit),
- parent(parent)
+static auto makeObjPath(std::string_view root, IfAddr addr)
{
+ auto ret = sdbusplus::message::object_path(std::string(root));
+ ret /= std::to_string(addr);
+ return ret;
+}
- IP::address(std::string(ipaddress));
- IP::prefixLength(prefixLength);
- IP::type(type);
+template <typename T>
+struct Proto
+{
+};
+
+template <>
+struct Proto<in_addr>
+{
+ static inline constexpr auto value = IP::Protocol::IPv4;
+};
+
+template <>
+struct Proto<in6_addr>
+{
+ static inline constexpr auto value = IP::Protocol::IPv6;
+};
+
+IPAddress::IPAddress(sdbusplus::bus_t& bus, std::string_view objRoot,
+ EthernetInterface& parent, IfAddr addr,
+ AddressOrigin origin) :
+ IPAddress(bus, makeObjPath(objRoot, addr), parent, addr, origin)
+{
+}
+
+IPAddress::IPAddress(sdbusplus::bus_t& bus,
+ sdbusplus::message::object_path objPath,
+ EthernetInterface& parent, IfAddr addr,
+ AddressOrigin origin) :
+ IPIfaces(bus, objPath.str.c_str(), IPIfaces::action::defer_emit),
+ parent(parent), objPath(std::move(objPath))
+{
+ IP::address(std::to_string(addr.getAddr()));
+ IP::prefixLength(addr.getPfx());
+ IP::type(std::visit([](auto v) { return Proto<decltype(v)>::value; },
+ addr.getAddr()));
IP::origin(origin);
// Emit deferred signal.
@@ -85,7 +117,19 @@
elog<InternalFailure>();
}
- parent.deleteObject(address());
+ std::unique_ptr<IPAddress> ptr;
+ for (auto it = parent.addrs.begin(); it != parent.addrs.end(); ++it)
+ {
+ if (it->second.get() == this)
+ {
+ ptr = std::move(it->second);
+ parent.addrs.erase(it);
+ break;
+ }
+ }
+
+ parent.writeConfigurationFile();
+ parent.manager.reloadConfigs();
}
namespace detail
diff --git a/src/ipaddress.hpp b/src/ipaddress.hpp
index e7bf479..94bdfdb 100644
--- a/src/ipaddress.hpp
+++ b/src/ipaddress.hpp
@@ -6,8 +6,9 @@
#include <cstdint>
#include <optional>
#include <sdbusplus/bus.hpp>
+#include <sdbusplus/message/native_types.hpp>
#include <sdbusplus/server/object.hpp>
-#include <stdplus/zstring.hpp>
+#include <string>
#include <string_view>
#include <vector>
#include <xyz/openbmc_project/Network/IP/server.hpp>
@@ -68,17 +69,13 @@
/** @brief Constructor to put object onto bus at a dbus path.
* @param[in] bus - Bus to attach to.
- * @param[in] objPath - Path to attach at.
+ * @param[in] objRoot - Path to attach at.
* @param[in] parent - Parent object.
- * @param[in] type - ipaddress type(v4/v6).
- * @param[in] ipAddress - ipadress.
+ * @param[in] addr - The ip address and prefix.
* @param[in] origin - origin of ipaddress(dhcp/static/SLAAC/LinkLocal).
- * @param[in] prefixLength - Length of prefix.
*/
- IPAddress(sdbusplus::bus_t& bus, stdplus::const_zstring objPath,
- EthernetInterface& parent, IP::Protocol type,
- std::string_view ipAddress, IP::AddressOrigin origin,
- uint8_t prefixLength);
+ IPAddress(sdbusplus::bus_t& bus, std::string_view objRoot,
+ EthernetInterface& parent, IfAddr addr, IP::AddressOrigin origin);
std::string address(std::string ipAddress) override;
uint8_t prefixLength(uint8_t) override;
@@ -96,9 +93,20 @@
using IP::prefixLength;
using IP::type;
+ inline const auto& getObjPath() const
+ {
+ return objPath;
+ }
+
private:
/** @brief Parent Object. */
EthernetInterface& parent;
+
+ /** @brief Dbus object path */
+ sdbusplus::message::object_path objPath;
+
+ IPAddress(sdbusplus::bus_t& bus, sdbusplus::message::object_path objPath,
+ EthernetInterface& parent, IfAddr addr, IP::AddressOrigin origin);
};
namespace detail
diff --git a/test/test_ethernet_interface.cpp b/test/test_ethernet_interface.cpp
index 654f45d..061e0c9 100644
--- a/test/test_ethernet_interface.cpp
+++ b/test/test_ethernet_interface.cpp
@@ -17,6 +17,7 @@
#include <string_view>
#include <xyz/openbmc_project/Common/error.hpp>
+#include <gmock/gmock.h>
#include <gtest/gtest.h>
namespace phosphor
@@ -25,6 +26,8 @@
{
using std::literals::string_view_literals::operator""sv;
+using testing::Key;
+using testing::UnorderedElementsAre;
class TestEthernetInterface : public stdplus::gtest::TestWithTmp
{
@@ -51,32 +54,6 @@
config::Parser()};
}
- int countIPObjects()
- {
- return interface.getAddresses().size();
- }
-
- bool isIPObjectExist(const std::string& ipaddress)
- {
- auto address = interface.getAddresses().find(ipaddress);
- if (address == interface.getAddresses().end())
- {
- return false;
- }
- return true;
- }
-
- bool deleteIPObject(const std::string& ipaddress)
- {
- auto address = interface.getAddresses().find(ipaddress);
- if (address == interface.getAddresses().end())
- {
- return false;
- }
- address->second->delete_();
- return true;
- }
-
std::string getObjectPath(const std::string& ipaddress, uint8_t subnetMask,
IP::AddressOrigin origin)
{
@@ -86,10 +63,10 @@
origin);
}
- void createIPObject(IP::Protocol addressType, const std::string& ipaddress,
+ auto createIPObject(IP::Protocol addressType, const std::string& ipaddress,
uint8_t subnetMask)
{
- interface.ip(addressType, ipaddress, subnetMask, "");
+ return interface.ip(addressType, ipaddress, subnetMask, "");
}
void setNtpServers()
@@ -129,59 +106,40 @@
TEST_F(TestEthernetInterface, NoIPaddress)
{
- EXPECT_EQ(countIPObjects(), 0);
+ EXPECT_TRUE(interface.addrs.empty());
}
TEST_F(TestEthernetInterface, AddIPAddress)
{
- IP::Protocol addressType = IP::Protocol::IPv4;
- createIPObject(addressType, "10.10.10.10", 16);
- EXPECT_EQ(true, isIPObjectExist("10.10.10.10"));
+ createIPObject(IP::Protocol::IPv4, "10.10.10.10", 16);
+ EXPECT_THAT(interface.addrs, UnorderedElementsAre(Key(
+ IfAddr(in_addr{htonl(0x0a0a0a0a)}, 16))));
}
TEST_F(TestEthernetInterface, AddMultipleAddress)
{
- IP::Protocol addressType = IP::Protocol::IPv4;
- createIPObject(addressType, "10.10.10.10", 16);
- createIPObject(addressType, "20.20.20.20", 16);
- EXPECT_EQ(true, isIPObjectExist("10.10.10.10"));
- EXPECT_EQ(true, isIPObjectExist("20.20.20.20"));
+ createIPObject(IP::Protocol::IPv4, "10.10.10.10", 16);
+ createIPObject(IP::Protocol::IPv4, "20.20.20.20", 16);
+ EXPECT_THAT(
+ interface.addrs,
+ UnorderedElementsAre(Key(IfAddr(in_addr{htonl(0x0a0a0a0a)}, 16)),
+ Key(IfAddr(in_addr{htonl(0x14141414)}, 16))));
}
TEST_F(TestEthernetInterface, DeleteIPAddress)
{
- IP::Protocol addressType = IP::Protocol::IPv4;
- createIPObject(addressType, "10.10.10.10", 16);
- createIPObject(addressType, "20.20.20.20", 16);
- deleteIPObject("10.10.10.10");
- EXPECT_EQ(false, isIPObjectExist("10.10.10.10"));
- EXPECT_EQ(true, isIPObjectExist("20.20.20.20"));
-}
-
-TEST_F(TestEthernetInterface, DeleteInvalidIPAddress)
-{
- EXPECT_EQ(false, deleteIPObject("10.10.10.10"));
+ createIPObject(IP::Protocol::IPv4, "10.10.10.10", 16);
+ createIPObject(IP::Protocol::IPv4, "20.20.20.20", 16);
+ interface.addrs.at(IfAddr(in_addr{htonl(0x0a0a0a0a)}, 16))->delete_();
+ EXPECT_THAT(interface.addrs, UnorderedElementsAre(Key(
+ IfAddr(in_addr{htonl(0x14141414)}, 16))));
}
TEST_F(TestEthernetInterface, CheckObjectPath)
{
- std::string ipaddress = "10.10.10.10";
- uint8_t prefix = 16;
- IP::AddressOrigin origin = IP::AddressOrigin::Static;
-
- auto path = getObjectPath(ipaddress, prefix, origin);
- auto pathsv = std::string_view(path);
- constexpr auto expectedPrefix = "/xyz/openbmc_test/network/test0/ipv4/"sv;
- EXPECT_TRUE(pathsv.starts_with(expectedPrefix));
- pathsv.remove_prefix(expectedPrefix.size());
- uint32_t val;
- auto [ptr, res] = std::from_chars(pathsv.begin(), pathsv.end(), val, 16);
- EXPECT_EQ(res, std::errc());
- EXPECT_EQ(ptr, pathsv.end());
-
- EXPECT_EQ(path, getObjectPath(ipaddress, prefix, origin));
- origin = IP::AddressOrigin::DHCP;
- EXPECT_NE(path, getObjectPath(ipaddress, prefix, origin));
+ auto path = createIPObject(IP::Protocol::IPv4, "10.10.10.10", 16);
+ EXPECT_EQ(path.parent_path(), "/xyz/openbmc_test/network/test0");
+ EXPECT_EQ(path.filename(), "10.10.10.10/16");
}
TEST_F(TestEthernetInterface, addStaticNameServers)