util: Clean up MAC address formatting
We should be using the built in ether_addr type wherever we want to
reference the MAC address in byte form. This gets rid of the integer MAC
type and makes functions that consistently take the byte form of the MAC
where possible.
Change-Id: Ic9e3eae8b5608ef517416215fb336871911975cc
Signed-off-by: William A. Kennington III <wak@google.com>
diff --git a/ethernet_interface.cpp b/ethernet_interface.cpp
index 053d82f..4acbdc5 100644
--- a/ethernet_interface.cpp
+++ b/ethernet_interface.cpp
@@ -25,6 +25,7 @@
#include <phosphor-logging/log.hpp>
#include <sstream>
#include <string>
+#include <string_view>
#include <xyz/openbmc_project/Common/error.hpp>
namespace phosphor
@@ -188,7 +189,7 @@
elog<InvalidArgument>(Argument::ARGUMENT_NAME("iPAddress"),
Argument::ARGUMENT_VALUE(iPAddress.c_str()));
}
- if (!mac_address::validate(mACAddress))
+ if (!mac_address::isUnicast(mac_address::fromString(mACAddress)))
{
log<level::ERR>("Not a valid MAC address",
entry("MACADDRESS=%s", iPAddress.c_str()));
@@ -262,14 +263,12 @@
EthernetInterface::getMACAddress(const std::string& interfaceName) const
{
ifreq ifr{};
- char macAddress[mac_address::size]{};
-
int sock = socket(AF_INET, SOCK_DGRAM, IPPROTO_IP);
if (sock < 0)
{
log<level::ERR>("socket creation failed:",
entry("ERROR=%s", strerror(errno)));
- return macAddress;
+ return "";
}
std::strcpy(ifr.ifr_name, interfaceName.c_str());
@@ -277,15 +276,13 @@
{
log<level::ERR>("ioctl failed for SIOCGIFHWADDR:",
entry("ERROR=%s", strerror(errno)));
- return macAddress;
+ return "";
}
- std::snprintf(macAddress, mac_address::size, mac_address::format,
- ifr.ifr_hwaddr.sa_data[0], ifr.ifr_hwaddr.sa_data[1],
- ifr.ifr_hwaddr.sa_data[2], ifr.ifr_hwaddr.sa_data[3],
- ifr.ifr_hwaddr.sa_data[4], ifr.ifr_hwaddr.sa_data[5]);
-
- return macAddress;
+ static_assert(sizeof(ifr.ifr_hwaddr.sa_data) >= sizeof(ether_addr));
+ std::string_view hwaddr(reinterpret_cast<char*>(ifr.ifr_hwaddr.sa_data),
+ sizeof(ifr.ifr_hwaddr.sa_data));
+ return mac_address::toString(copyFrom<ether_addr>(hwaddr));
}
std::string EthernetInterface::generateId(const std::string& ipaddress,
@@ -737,7 +734,8 @@
std::string EthernetInterface::mACAddress(std::string value)
{
- if (!mac_address::validate(value))
+ ether_addr newMAC = mac_address::fromString(value);
+ if (!mac_address::isUnicast(newMAC))
{
log<level::ERR>("MACAddress is not valid.",
entry("MAC=%s", value.c_str()));
@@ -745,77 +743,55 @@
Argument::ARGUMENT_VALUE(value.c_str()));
}
- // check whether MAC is broadcast mac.
- auto intMac = mac_address::internal::convertToInt(value);
-
- if (!(intMac ^ mac_address::broadcastMac))
+ // We don't need to update the system if the address is unchanged
+ ether_addr oldMAC = mac_address::fromString(MacAddressIntf::mACAddress());
+ if (!equal(newMAC, oldMAC))
{
- log<level::ERR>("MACAddress is a broadcast mac.",
- entry("MAC=%s", value.c_str()));
- elog<InvalidArgument>(Argument::ARGUMENT_NAME("MACAddress"),
- Argument::ARGUMENT_VALUE(value.c_str()));
- }
-
- // Check if the MAC changed.
- auto pmac = MacAddressIntf::mACAddress();
- if (strcasecmp(pmac.c_str(), value.c_str()) == 0)
- {
- return MacAddressIntf::mACAddress();
- }
-
- // Allow the mac to be set if one of the condition is true.
- // 1) Incoming Mac is of local admin type.
- // or
- // 2) Incoming mac is same as eeprom Mac.
-
- if (!(intMac & mac_address::localAdminMask))
- {
- try
+ if (!mac_address::isLocalAdmin(newMAC))
{
- auto inventoryMac = mac_address::getfromInventory(bus);
- auto intInventoryMac =
- mac_address::internal::convertToInt(inventoryMac);
-
- if (intInventoryMac != intMac)
+ try
{
- log<level::ERR>("Given MAC address is neither a local Admin "
- "type nor is same as in inventory");
- elog<InvalidArgument>(Argument::ARGUMENT_NAME("MACAddress"),
- Argument::ARGUMENT_VALUE(value.c_str()));
+ auto inventoryMAC = mac_address::getfromInventory(bus);
+ if (!equal(newMAC, inventoryMAC))
+ {
+ log<level::ERR>(
+ "Given MAC address is neither a local Admin "
+ "type nor is same as in inventory");
+ elog<InvalidArgument>(
+ Argument::ARGUMENT_NAME("MACAddress"),
+ Argument::ARGUMENT_VALUE(value.c_str()));
+ }
+ }
+ catch (const std::exception& e)
+ {
+ log<level::ERR>("Exception occurred during getting of MAC "
+ "address from Inventory");
+ elog<InternalFailure>();
}
}
- catch (InternalFailure& e)
+
+ // Update everything that depends on the MAC value
+ for (const auto& [name, intf] : vlanInterfaces)
{
- log<level::ERR>("Exception occurred during getting of MAC "
- "address from Inventory");
- elog<InternalFailure>();
+ intf->MacAddressIntf::mACAddress(value);
}
- }
- auto interface = interfaceName();
- execute("/sbin/fw_setenv", "fw_setenv", "ethaddr", value.c_str());
- // TODO: would replace below three calls
- // with restarting of systemd-netwokd
- // through https://github.com/systemd/systemd/issues/6696
- execute("/sbin/ip", "ip", "link", "set", "dev", interface.c_str(), "down");
- execute("/sbin/ip", "ip", "link", "set", "dev", interface.c_str(),
- "address", value.c_str());
+ MacAddressIntf::mACAddress(value);
- execute("/sbin/ip", "ip", "link", "set", "dev", interface.c_str(), "up");
-
- auto mac = MacAddressIntf::mACAddress(std::move(value));
- // update all the vlan interfaces
- for (const auto& intf : vlanInterfaces)
- {
- intf.second->updateMacAddress();
- }
-
- // restart the systemd networkd so that dhcp client gets the
- // ip for the changed mac address.
- if (dHCPEnabled())
- {
+ auto interface = interfaceName();
+ execute("/sbin/fw_setenv", "fw_setenv", "ethaddr", value.c_str());
+ // TODO: would replace below three calls
+ // with restarting of systemd-netwokd
+ // through https://github.com/systemd/systemd/issues/6696
+ execute("/sbin/ip", "ip", "link", "set", "dev", interface.c_str(),
+ "down");
+ execute("/sbin/ip", "ip", "link", "set", "dev", interface.c_str(),
+ "address", value.c_str());
+ execute("/sbin/ip", "ip", "link", "set", "dev", interface.c_str(),
+ "up");
manager.restartSystemdUnit(networkdService);
}
- return mac;
+
+ return value;
}
void EthernetInterface::deleteAll()
diff --git a/test/test_util.cpp b/test/test_util.cpp
index f49e631..37193b8 100644
--- a/test/test_util.cpp
+++ b/test/test_util.cpp
@@ -123,24 +123,6 @@
EXPECT_EQ(false, isValidPrefix(AF_INET, prefixLength));
}
-TEST_F(TestUtil, MacValidation)
-{
- std::string macaddress = "00:00:00:00:00:00";
- EXPECT_EQ(false, phosphor::network::mac_address::validate(macaddress));
-
- macaddress = "F6:C6:E6:6:B0:D3";
- EXPECT_EQ(true, phosphor::network::mac_address::validate(macaddress));
-
- macaddress = "F6:C6:E6:06:B0:D3";
- EXPECT_EQ(true, phosphor::network::mac_address::validate(macaddress));
-
- macaddress = "hh:HH:HH:hh:HH:yy";
- EXPECT_EQ(false, phosphor::network::mac_address::validate(macaddress));
-
- macaddress = "hhh:GGG:iii:jjj:kkk:lll";
- EXPECT_EQ(false, phosphor::network::mac_address::validate(macaddress));
-}
-
TEST_F(TestUtil, ConvertV4MasktoPrefix)
{
std::string mask = "255.255.255.0";
@@ -326,5 +308,81 @@
EXPECT_FALSE(equal(a, b));
}
+namespace mac_address
+{
+
+TEST(MacFromString, Bad)
+{
+ EXPECT_THROW(fromString("0x:00:00:00:00:00"), std::runtime_error);
+ EXPECT_THROW(fromString("00:00:00:00:00"), std::runtime_error);
+ EXPECT_THROW(fromString(""), std::runtime_error);
+}
+
+TEST(MacFromString, Valid)
+{
+ EXPECT_TRUE(equal(ether_addr{}, fromString("00:00:00:00:00:00")));
+ EXPECT_TRUE(equal(ether_addr{0xff, 0xee, 0xdd, 0xcc, 0xbb, 0xaa},
+ fromString("FF:EE:DD:cc:bb:aa")));
+ EXPECT_TRUE(equal(ether_addr{0x00, 0x01, 0x02, 0x03, 0x04, 0x05},
+ fromString("0:1:2:3:4:5")));
+}
+
+TEST(MacToString, Valid)
+{
+ EXPECT_EQ("11:22:33:44:55:66",
+ toString({0x11, 0x22, 0x33, 0x44, 0x55, 0x66}));
+}
+
+TEST(MacIsEmpty, True)
+{
+ EXPECT_TRUE(isEmpty({}));
+}
+
+TEST(MacIsEmpty, False)
+{
+ EXPECT_FALSE(isEmpty(fromString("01:00:00:00:00:00")));
+ EXPECT_FALSE(isEmpty(fromString("00:00:00:10:00:00")));
+ EXPECT_FALSE(isEmpty(fromString("00:00:00:00:00:01")));
+}
+
+TEST(MacIsMulticast, True)
+{
+ EXPECT_TRUE(isMulticast(fromString("ff:ff:ff:ff:ff:ff")));
+ EXPECT_TRUE(isMulticast(fromString("01:00:00:00:00:00")));
+}
+
+TEST(MacIsMulticast, False)
+{
+ EXPECT_FALSE(isMulticast(fromString("00:11:22:33:44:55")));
+ EXPECT_FALSE(isMulticast(fromString("FE:11:22:33:44:55")));
+}
+
+TEST(MacIsUnicast, True)
+{
+ EXPECT_TRUE(isUnicast(fromString("00:11:22:33:44:55")));
+ EXPECT_TRUE(isUnicast(fromString("FE:11:22:33:44:55")));
+}
+
+TEST(MacIsUnicast, False)
+{
+ EXPECT_FALSE(isUnicast(fromString("00:00:00:00:00:00")));
+ EXPECT_FALSE(isUnicast(fromString("01:00:00:00:00:00")));
+ EXPECT_FALSE(isUnicast(fromString("ff:ff:ff:ff:ff:ff")));
+}
+
+TEST(MacIsLocalAdmin, True)
+{
+ EXPECT_TRUE(isLocalAdmin(fromString("02:11:22:33:44:55")));
+ EXPECT_TRUE(isLocalAdmin(fromString("FE:11:22:33:44:55")));
+}
+
+TEST(MacIsLocalAdmin, False)
+{
+ EXPECT_FALSE(isLocalAdmin(fromString("00:00:00:00:00:00")));
+ EXPECT_FALSE(isLocalAdmin(fromString("01:00:00:00:00:00")));
+ EXPECT_FALSE(isLocalAdmin(fromString("fd:ff:ff:ff:ff:ff")));
+}
+
+} // namespace mac_address
} // namespace network
} // namespace phosphor
diff --git a/util.cpp b/util.cpp
index 91030c8..a17bc5d 100644
--- a/util.cpp
+++ b/util.cpp
@@ -16,6 +16,7 @@
#include <phosphor-logging/log.hpp>
#include <stdexcept>
#include <string>
+#include <variant>
#include <xyz/openbmc_project/Common/error.hpp>
namespace phosphor
@@ -549,7 +550,7 @@
"xyz.openbmc_project.Inventory.Item.NetworkInterface";
constexpr auto invRoot = "/xyz/openbmc_project/inventory";
-std::string getfromInventory(sdbusplus::bus::bus& bus)
+ether_addr getfromInventory(sdbusplus::bus::bus& bus)
{
std::vector<DbusInterface> interfaces;
interfaces.emplace_back(invNetworkIntf);
@@ -583,8 +584,6 @@
auto objPath = objectTree.begin()->first;
auto service = objectTree.begin()->second.begin()->first;
- sdbusplus::message::variant<std::string> value;
-
auto method = bus.new_method_call(service.c_str(), objPath.c_str(),
propIntf, methodGet);
@@ -599,8 +598,19 @@
elog<InternalFailure>();
}
+ std::variant<std::string> value;
reply.read(value);
- return sdbusplus::message::variant_ns::get<std::string>(value);
+ return fromString(std::get<std::string>(value));
+}
+
+ether_addr fromString(const char* str)
+{
+ struct ether_addr* mac = ether_aton(str);
+ if (mac == nullptr)
+ {
+ throw std::runtime_error("Invalid mac address string");
+ }
+ return *mac;
}
std::string toString(const ether_addr& mac)
@@ -608,6 +618,26 @@
return ether_ntoa(&mac);
}
+bool isEmpty(const ether_addr& mac)
+{
+ return equal(mac, ether_addr{});
+}
+
+bool isMulticast(const ether_addr& mac)
+{
+ return mac.ether_addr_octet[0] & 0b1;
+}
+
+bool isUnicast(const ether_addr& mac)
+{
+ return !isEmpty(mac) && !isMulticast(mac);
+}
+
+bool isLocalAdmin(const ether_addr& mac)
+{
+ return mac.ether_addr_octet[0] & 0b10;
+}
+
} // namespace mac_address
} // namespace network
} // namespace phosphor
diff --git a/util.hpp b/util.hpp
index cf61e16..79593c5 100644
--- a/util.hpp
+++ b/util.hpp
@@ -26,58 +26,52 @@
namespace mac_address
{
-constexpr auto localAdminMask = 0x020000000000;
-constexpr auto broadcastMac = 0xFFFFFFFFFFFF;
-
-constexpr auto format = "%02hhx:%02hhx:%02hhx:%02hhx:%02hhx:%02hhx";
-constexpr size_t size = 18;
-
-/** @brief validate the mac address
- * @param[in] value - MAC address.
- * @returns true if validate otherwise false.
- */
-inline bool validate(const std::string& value)
-{
- struct ether_addr* addr = ether_aton(value.c_str());
- if (addr == nullptr)
- {
- return false;
- }
- ether_addr zero{};
- return std::memcmp(addr, &zero, sizeof(zero)) != 0;
-}
-
/** @brief gets the MAC address from the Inventory.
* @param[in] bus - DBUS Bus Object.
*/
-std::string getfromInventory(sdbusplus::bus::bus& bus);
+ether_addr getfromInventory(sdbusplus::bus::bus& bus);
+
+/** @brief Converts the given mac address into byte form
+ * @param[in] str - The mac address in human readable form
+ * @returns A mac address in network byte order
+ * @throws std::runtime_error for bad mac
+ */
+ether_addr fromString(const char* str);
+inline ether_addr fromString(const std::string& str)
+{
+ return fromString(str.c_str());
+}
/** @brief Converts the given mac address bytes into a string
- * @param[in] bytes - The mac address
+ * @param[in] mac - The mac address
* @returns A valid mac address string
*/
std::string toString(const ether_addr& mac);
-namespace internal
-{
-/** @brief Converts the given mac address into unsigned 64 bit integer
- * @param[in] value - MAC address.
- * @returns converted unsigned 64 bit number.
+/** @brief Determines if the mac address is empty
+ * @param[in] mac - The mac address
+ * @return True if 00:00:00:00:00:00
*/
-inline uint64_t convertToInt(const std::string& value)
-{
- struct ether_addr* addr = ether_aton(value.c_str());
- if (addr == nullptr)
- {
- return 0;
- }
- uint64_t ret = 0;
- static_assert(sizeof(ret) >= sizeof(*addr));
- std::memcpy(&ret, addr, sizeof(*addr));
- return ret;
-}
+bool isEmpty(const ether_addr& mac);
-} // namespace internal
+/** @brief Determines if the mac address is a multicast address
+ * @param[in] mac - The mac address
+ * @return True if multicast bit is set
+ */
+bool isMulticast(const ether_addr& mac);
+
+/** @brief Determines if the mac address is a unicast address
+ * @param[in] mac - The mac address
+ * @return True if not multicast or empty
+ */
+bool isUnicast(const ether_addr& mac);
+
+/** @brief Determines if the mac address is locally managed
+ * @param[in] mac - The mac address
+ * @return True if local admin bit is set
+ */
+bool isLocalAdmin(const ether_addr& mac);
+
} // namespace mac_address
constexpr auto networkdService = "systemd-networkd.service";
diff --git a/vlan_interface.hpp b/vlan_interface.hpp
index 3ef98eb..48cb14d 100644
--- a/vlan_interface.hpp
+++ b/vlan_interface.hpp
@@ -57,12 +57,6 @@
and creates the vlan interface.*/
void writeDeviceFile();
- /** @brief copy the mac address from the parent interface.*/
- void updateMacAddress()
- {
- MacAddressIntf::mACAddress(parentInterface.mACAddress());
- }
-
private:
/** @brief VLAN Identifier. */
using VlanIface::id;