ethernet_interface: Detect nicEnabled from systemd-networkd
Our current method of interface detection can race with
systemd-networkd. If systemd-networkd takes too long to transition from
the initialized state to bringing up the link, phosphor-networkd will
see the link as administratively down and treat it as if it should be
Unmanaged. This is incorrect and causes us to lose BMC network
information about 1% of boots with our current configuration. This has
the consequence of being persistent across powercycles and even across
firmware updates. Without using some method of intervention, this
prevents automated tooling from configuring the management interface.
systemd-networkd can actually inform us to the state of network
interfaces via DBus. We can monitor the AdministrativeState property to
determine whether or not our NIC should be enabled. We now wait until
systemd-networkd has progressed far enough to detect it.
Change-Id: Ic5cb7e6805791e040ab145517e3b1c9e8b146851
Signed-off-by: William A. Kennington III <wak@google.com>
diff --git a/src/ethernet_interface.cpp b/src/ethernet_interface.cpp
index ce424ff..0f8a3c0 100644
--- a/src/ethernet_interface.cpp
+++ b/src/ethernet_interface.cpp
@@ -9,6 +9,7 @@
#include "vlan_interface.hpp"
#include <arpa/inet.h>
+#include <fmt/format.h>
#include <linux/ethtool.h>
#include <linux/rtnetlink.h>
#include <linux/sockios.h>
@@ -23,10 +24,13 @@
#include <fstream>
#include <phosphor-logging/elog-errors.hpp>
#include <phosphor-logging/log.hpp>
+#include <sdbusplus/bus/match.hpp>
#include <sstream>
#include <stdplus/raw.hpp>
#include <string>
#include <string_view>
+#include <unordered_map>
+#include <variant>
#include <xyz/openbmc_project/Common/error.hpp>
namespace phosphor
@@ -76,7 +80,8 @@
EthernetInterface::EthernetInterface(sdbusplus::bus::bus& bus,
const std::string& objPath,
DHCPConf dhcpEnabled, Manager& parent,
- bool emitSignal) :
+ bool emitSignal,
+ std::optional<bool> enabled) :
Ifaces(bus, objPath.c_str(), true),
bus(bus), manager(parent), objPath(objPath)
{
@@ -85,6 +90,7 @@
interfaceName(intfName);
EthernetInterfaceIntf::dhcpEnabled(dhcpEnabled);
EthernetInterfaceIntf::ipv6AcceptRA(getIPv6AcceptRAFromConf());
+ EthernetInterfaceIntf::nicEnabled(enabled ? *enabled : queryNicEnabled());
route::Table routingTable;
auto gatewayList = routingTable.getDefaultGateway();
auto gateway6List = routingTable.getDefaultGateway6();
@@ -120,7 +126,6 @@
EthernetInterfaceIntf::ntpServers(getNTPServersFromConf());
EthernetInterfaceIntf::linkUp(linkUp());
- EthernetInterfaceIntf::nicEnabled(nicEnabled());
#ifdef NIC_SUPPORTS_ETHTOOL
InterfaceInfo ifInfo = EthernetInterface::getInterfaceInfo();
@@ -593,28 +598,89 @@
return value;
}
-bool EthernetInterface::nicEnabled() const
+bool EthernetInterface::queryNicEnabled() const
{
- EthernetIntfSocket eifSocket(PF_INET, SOCK_DGRAM, IPPROTO_IP);
- bool value = EthernetInterfaceIntf::nicEnabled();
+ constexpr auto svc = "org.freedesktop.network1";
+ constexpr auto intf = "org.freedesktop.network1.Link";
+ constexpr auto prop = "AdministrativeState";
+ char* rpath;
+ sd_bus_path_encode("/org/freedesktop/network1/link",
+ std::to_string(ifIndex()).c_str(), &rpath);
+ std::string path(rpath);
+ free(rpath);
- if (eifSocket.sock < 0)
+ // Store / Parser for the AdministrativeState return value
+ std::optional<bool> ret;
+ auto cb = [&](const std::string& state) {
+ if (state != "initialized")
+ {
+ ret = state != "unmanaged";
+ }
+ };
+
+ // Build a matcher before making the property call to ensure we
+ // can eventually get the value.
+ sdbusplus::bus::match::match match(
+ bus,
+ fmt::format("type='signal',sender='{}',path='{}',interface='{}',member="
+ "'PropertiesChanged',arg0='{}',",
+ svc, path, PROPERTY_INTERFACE, intf)
+ .c_str(),
+ [&](sdbusplus::message::message& m) {
+ std::string intf;
+ std::unordered_map<std::string, std::variant<std::string>> values;
+ try
+ {
+ m.read(intf, values);
+ auto it = values.find(prop);
+ // Ignore properties that aren't AdministrativeState
+ if (it != values.end())
+ {
+ cb(std::get<std::string>(it->second));
+ }
+ }
+ catch (const std::exception& e)
+ {
+ log<level::ERR>(
+ fmt::format(
+ "AdministrativeState match parsing failed on {}: {}",
+ interfaceName(), e.what())
+ .c_str(),
+ entry("INTERFACE=%s", interfaceName().c_str()),
+ entry("ERROR=%s", e.what()));
+ }
+ });
+
+ // Actively call for the value in case the interface is already configured
+ auto method =
+ bus.new_method_call(svc, path.c_str(), PROPERTY_INTERFACE, METHOD_GET);
+ method.append(intf, prop);
+ try
{
- return value;
+ auto reply = bus.call(method);
+ std::variant<std::string> state;
+ reply.read(state);
+ cb(std::get<std::string>(state));
+ }
+ catch (const std::exception& e)
+ {
+ log<level::ERR>(
+ fmt::format("Failed to get AdministrativeState on {}: {}",
+ interfaceName(), e.what())
+ .c_str(),
+ entry("INTERFACE=%s", interfaceName().c_str()),
+ entry("ERROR=%s", e.what()));
}
- ifreq ifr = {};
- std::strncpy(ifr.ifr_name, interfaceName().c_str(), IF_NAMESIZE - 1);
- if (ioctl(eifSocket.sock, SIOCGIFFLAGS, &ifr) == 0)
+ // The interface is not yet configured by systemd-networkd, wait until it
+ // signals us a valid state.
+ while (!ret)
{
- value = static_cast<bool>(ifr.ifr_flags & IFF_UP);
+ bus.wait();
+ bus.process_discard();
}
- else
- {
- log<level::ERR>("ioctl failed for SIOCGIFFLAGS:",
- entry("ERROR=%s", strerror(errno)));
- }
- return value;
+
+ return *ret;
}
bool EthernetInterface::nicEnabled(bool value)
diff --git a/src/ethernet_interface.hpp b/src/ethernet_interface.hpp
index 8e28b51..fef1579 100644
--- a/src/ethernet_interface.hpp
+++ b/src/ethernet_interface.hpp
@@ -93,10 +93,12 @@
* @param[in] parent - parent object.
* @param[in] emitSignal - true if the object added signal needs to be
* send.
+ * @param[in] enabled - Override the lookup of nicEnabled
*/
EthernetInterface(sdbusplus::bus::bus& bus, const std::string& objPath,
DHCPConf dhcpEnabled, Manager& parent,
- bool emitSignal = true);
+ bool emitSignal = true,
+ std::optional<bool> enabled = std::nullopt);
/** @brief Function used to load the nameservers.
*/
@@ -176,9 +178,6 @@
/** Retrieve Link State */
bool linkUp() const override;
- /** Retrieve NIC State */
- bool nicEnabled() const override;
-
/** Set value of NICEnabled */
bool nicEnabled(bool value) override;
@@ -366,6 +365,11 @@
* @returns true/false value if the address is static
*/
bool originIsManuallyAssigned(IP::AddressOrigin origin);
+
+ /** @brief Determines if the NIC is enabled in systemd
+ * @returns true/false value if the NIC is enabled
+ */
+ bool queryNicEnabled() const;
};
} // namespace network
diff --git a/src/vlan_interface.cpp b/src/vlan_interface.cpp
index 4920c77..035eb07 100644
--- a/src/vlan_interface.cpp
+++ b/src/vlan_interface.cpp
@@ -27,11 +27,11 @@
EthernetInterface& intf, Manager& parent) :
VlanIface(bus, objPath.c_str()),
DeleteIface(bus, objPath.c_str()),
- EthernetInterface(bus, objPath, dhcpEnabled, parent, false),
+ EthernetInterface(bus, objPath, dhcpEnabled, parent, /*emitSignal=*/false,
+ nicEnabled),
parentInterface(intf)
{
id(vlanID);
- EthernetInterfaceIntf::nicEnabled(nicEnabled);
VlanIface::interfaceName(EthernetInterface::interfaceName());
MacAddressIntf::macAddress(parentInterface.macAddress());
diff --git a/test/mock_ethernet_interface.hpp b/test/mock_ethernet_interface.hpp
index fc1e259..bfe81ed 100644
--- a/test/mock_ethernet_interface.hpp
+++ b/test/mock_ethernet_interface.hpp
@@ -15,7 +15,8 @@
MockEthernetInterface(sdbusplus::bus::bus& bus, const std::string& objPath,
DHCPConf dhcpEnabled, Manager& parent,
bool emitSignal) :
- EthernetInterface(bus, objPath, dhcpEnabled, parent, emitSignal)
+ EthernetInterface(bus, objPath, dhcpEnabled, parent, emitSignal,
+ /*nicEnabled=*/true)
{
}
diff --git a/test/test_vlan_interface.cpp b/test/test_vlan_interface.cpp
index 740b6fd..f7e6f2b 100644
--- a/test/test_vlan_interface.cpp
+++ b/test/test_vlan_interface.cpp
@@ -50,8 +50,12 @@
{
mock_clear();
mock_addIF("test0", 1);
- return {bus, "/xyz/openbmc_test/network/test0",
- EthernetInterface::DHCPConf::none, manager};
+ return {bus,
+ "/xyz/openbmc_test/network/test0",
+ EthernetInterface::DHCPConf::none,
+ manager,
+ false,
+ true};
}
void setConfDir()