From 8e157735e0655f216bd901e299e6b98329cbee26 Mon Sep 17 00:00:00 2001
From: Ed Tanous <ed@tanous.net>
Date: Thu, 7 Mar 2024 13:07:00 -0800
Subject: Fix NTP set race condition

There's currently a problem with phosphor-timesyncd, where enabling NTP
doesn't immediately reflect in the system status on return[1].  To say
it another way, NTP is not enabled/disabled atomically, which leads to
the following problem.

// Disable NTP
PATCH /redfish/v1/Managers/bmc/NetworkProtocol
{"NTP":{"ProtocolEnabled": false}}
// Set the time manually
PATCH /redfish/v1/Managers/bmc {"DateTime": "<timestring"}

Doing this in rapid succession leads to a 500 error, which is obviously
a bug.  In the prior commit, this error was changed to a
PropertyValueConflict error, which is still incorrect, but at least
informative of what's going on.  REST APIs are intended to have CRUD
compliance.  The response should not be returned until the value has
been accepted, and not doing that can lead to problems.

This commit changes the backend to use systemd directly, rather than
routing through phosphor-settings, to avoid this race.

Quite possibly resolves #264 but haven't tested that.

Tested: The above procedure succeeds.

[1] https://github.com/systemd/systemd/pull/11424

Change-Id: I19241e7677d9b6415aff79ac65c474ae71984417
Signed-off-by: Ed Tanous <ed@tanous.net>
---
 redfish-core/lib/network_protocol.hpp | 60 ++++++++++++++++-------------------
 1 file changed, 28 insertions(+), 32 deletions(-)

diff --git a/redfish-core/lib/network_protocol.hpp b/redfish-core/lib/network_protocol.hpp
index 747abbf97d..adafcbb0be 100644
--- a/redfish-core/lib/network_protocol.hpp
+++ b/redfish-core/lib/network_protocol.hpp
@@ -238,25 +238,30 @@ inline void getNetworkData(const std::shared_ptr<bmcweb::AsyncResp>& asyncResp,
                          std::bind_front(afterNetworkPortRequest, asyncResp));
 } // namespace redfish
 
-inline void handleNTPProtocolEnabled(
-    const bool& ntpEnabled, const std::shared_ptr<bmcweb::AsyncResp>& asyncResp)
+inline void afterSetNTP(const std::shared_ptr<bmcweb::AsyncResp>& asyncResp,
+                        const boost::system::error_code& ec)
 {
-    std::string timeSyncMethod;
-    if (ntpEnabled)
-    {
-        timeSyncMethod = "xyz.openbmc_project.Time.Synchronization.Method.NTP";
-    }
-    else
+    if (ec)
     {
-        timeSyncMethod =
-            "xyz.openbmc_project.Time.Synchronization.Method.Manual";
+        BMCWEB_LOG_DEBUG("Failed to set elapsed time. DBUS response error {}",
+                         ec);
+        messages::internalError(asyncResp->res);
+        return;
     }
+    asyncResp->res.result(boost::beast::http::status::no_content);
+}
 
-    setDbusProperty(asyncResp, "xyz.openbmc_project.Settings",
-                    sdbusplus::message::object_path(
-                        "/xyz/openbmc_project/time/sync_method"),
-                    "xyz.openbmc_project.Time.Synchronization",
-                    "TimeSyncMethod", "NTP/ProtocolEnabled", timeSyncMethod);
+inline void handleNTPProtocolEnabled(
+    const std::shared_ptr<bmcweb::AsyncResp>& asyncResp, bool ntpEnabled)
+{
+    bool interactive = false;
+    auto callback = [asyncResp](const boost::system::error_code& ec) {
+        afterSetNTP(asyncResp, ec);
+    };
+    crow::connections::systemBus->async_method_call(
+        std::move(callback), "org.freedesktop.timedate1",
+        "/org/freedesktop/timedate1", "org.freedesktop.timedate1", "SetNTP",
+        ntpEnabled, interactive);
 }
 
 // Redfish states that ip addresses can be
@@ -420,27 +425,18 @@ inline std::string getHostName()
 inline void
     getNTPProtocolEnabled(const std::shared_ptr<bmcweb::AsyncResp>& asyncResp)
 {
-    sdbusplus::asio::getProperty<std::string>(
-        *crow::connections::systemBus, "xyz.openbmc_project.Settings",
-        "/xyz/openbmc_project/time/sync_method",
-        "xyz.openbmc_project.Time.Synchronization", "TimeSyncMethod",
-        [asyncResp](const boost::system::error_code& ec,
-                    const std::string& timeSyncMethod) {
+    sdbusplus::asio::getProperty<bool>(
+        *crow::connections::systemBus, "org.freedesktop.timedate1",
+        "/org/freedesktop/timedate1", "org.freedesktop.timedate1", "NTP",
+        [asyncResp](const boost::system::error_code& ec, bool enabled) {
         if (ec)
         {
+            BMCWEB_LOG_WARNING(
+                "Failed to get NTP status, assuming not supported");
             return;
         }
 
-        if (timeSyncMethod ==
-            "xyz.openbmc_project.Time.Synchronization.Method.NTP")
-        {
-            asyncResp->res.jsonValue["NTP"]["ProtocolEnabled"] = true;
-        }
-        else if (timeSyncMethod == "xyz.openbmc_project.Time.Synchronization."
-                                   "Method.Manual")
-        {
-            asyncResp->res.jsonValue["NTP"]["ProtocolEnabled"] = false;
-        }
+        asyncResp->res.jsonValue["NTP"]["ProtocolEnabled"] = enabled;
     });
 }
 
@@ -502,7 +498,7 @@ inline void handleManagersNetworkProtocolPatch(
 
     if (ntpEnabled)
     {
-        handleNTPProtocolEnabled(*ntpEnabled, asyncResp);
+        handleNTPProtocolEnabled(asyncResp, *ntpEnabled);
     }
     if (ntpServerObjects)
     {
-- 
cgit v1.2.3