srvcfg-manager State change issue fix.

srvcfg-manager State change is not working as
it need to act on both .service and .socket files.
Also used MaskUnitFiles and UnmaskUnitFiles to
make changes persistent.

Tested By:
 Disabled/Enabled the state and checked  the service
 is stopping/starting properly. Also rebooted system
 and checked persistent behaviour. Checked changing
 port/channel and validated functionality.

Change-Id: Ib0dce95db3926f5d700fb729aae7bdac1caf64e4
Signed-off-by: AppaRao Puli <apparao.puli@linux.intel.com>

3 files changed, 72 insertions, 37 deletions

diff --git a/srvcfg-manager/inc/utils.hpp b/srvcfg-manager/inc/utils.hpp
index 67ced4c..27e66f3 100644
--- a/srvcfg-manager/inc/utils.hpp
+++ b/srvcfg-manager/inc/utils.hpp
@@ -33,8 +33,8 @@ void systemdUnitAction(const std::shared_ptr<sdbusplus::asio::connection> &conn,
                        const std::string &unitName,

                        const std::string &actionMethod);

 

-void systemdUnitFileStateChange(

+void systemdUnitFilesStateChange(

     const std::shared_ptr<sdbusplus::asio::connection> &conn,

-    const std::string &unitName, const std::string &unitState);

+    const std::vector<std::string> &unitFiles, const std::string &unitState);

 

 bool checkSystemdUnitExist(const std::string &unitName);

diff --git a/srvcfg-manager/src/srvcfg_manager.cpp b/srvcfg-manager/src/srvcfg_manager.cpp
index 04d41ca..333c001 100644
--- a/srvcfg-manager/src/srvcfg_manager.cpp
+++ b/srvcfg-manager/src/srvcfg_manager.cpp
@@ -126,6 +126,7 @@ void ServiceConfig::syncWithSystemD1Properties()
     std::string srvUnitName(sysDUnitName);

     if (srvUnitName == "dropbear")

     {

+        // Dropbear service expects template arguments.

         srvUnitName.append("@");

     }

     srvUnitName.append(".service");

@@ -137,7 +138,15 @@ void ServiceConfig::syncWithSystemD1Properties()
                     "async_method_call error: Failed to get property");

                 return;

             }

-            stateValue = pValue;

+            if ((pValue == "enabled") || (pValue == "static") ||

+                (pValue == "unmasked"))

+            {

+                stateValue = "enabled";

+            }

+            else if ((pValue == "disabled") || (pValue == "masked"))

+            {

+                stateValue = "disabled";

+            }

             conn->async_method_call(

                 [](boost::system::error_code ec) {

                     if (ec)

@@ -212,6 +221,12 @@ ServiceConfig::ServiceConfig(
 

 void ServiceConfig::applySystemDServiceConfig()

 {

+    if (updatedFlag)

+    {

+        // No updates. Just return.

+        return;

+    }

+

     phosphor::logging::log<phosphor::logging::level::INFO>(

         "Applying new settings.",

         phosphor::logging::entry("OBJPATH=%s", objPath.c_str()));

@@ -261,28 +276,46 @@ void ServiceConfig::applySystemDServiceConfig()
             phosphor::logging::elog<sdbusplus::xyz::openbmc_project::Common::

                                         Error::InternalFailure>();

         }

+    }

 

-        // Systemd forcing explicit socket stop before reload...!

-        std::string socketUnitName(sysDUnitName + ".socket");

-        systemdUnitAction(conn, socketUnitName, sysdActionStopUnit);

-

-        std::string srvUnitName(sysDUnitName + ".service");

-        systemdUnitAction(conn, srvUnitName, sysdActionStopUnit);

-

-        // Perform daemon reload to read new settings

-        systemdDaemonReload(conn);

-

-        // Restart the unit

-        systemdUnitAction(conn, socketUnitName, sysdActionStartUnit);

-        systemdUnitAction(conn, srvUnitName, sysdActionStartUnit);

+    std::string socketUnitName(sysDUnitName + ".socket");

+    std::string srvUnitName(sysDUnitName);

+    if (srvUnitName == "dropbear")

+    {

+        // Dropbear service expects template arguments.

+        // Todo: Unit action for service, fails with error

+        // "missing the instance name". Needs to implement

+        // getting all running instances and use it. This

+        // impact runtime but works fine during reboot.

+        srvUnitName.append("@");

+    }

+    srvUnitName.append(".service");

+    // Stop the running service in below scenarios.

+    // 1. State changed from "enabled" to "disabled"

+    // 2. No change in state and existing stateValue is

+    //    "enabled" and there is change in other properties.

+    if (((stateValue == "disabled") &&

+         (updatedFlag & (1 << static_cast<uint8_t>(UpdatedProp::state)))) ||

+        (!(updatedFlag & (1 << static_cast<uint8_t>(UpdatedProp::state))) &&

+         (stateValue == "enabled") && (updatedFlag)))

+    {

+        systemdUnitAction(conn, socketUnitName, "StopUnit");

+        systemdUnitAction(conn, srvUnitName, "StopUnit");

     }

 

     if (updatedFlag & (1 << static_cast<uint8_t>(UpdatedProp::state)))

     {

-        if ((stateValue == "enabled") || (stateValue == "disabled"))

-        {

-            systemdUnitFileStateChange(conn, sysDUnitName, stateValue);

-        }

+        std::vector<std::string> unitFiles = {socketUnitName, srvUnitName};

+        systemdUnitFilesStateChange(conn, unitFiles, stateValue);

+    }

+

+    // Perform daemon reload to read new settings

+    systemdDaemonReload(conn);

+

+    if (stateValue == "enabled")

+    {

+        // Restart the socket

+        systemdUnitAction(conn, socketUnitName, "StartUnit");

     }

 

     // Reset the flag

@@ -290,8 +323,8 @@ void ServiceConfig::applySystemDServiceConfig()
 

     // All done. Lets reload the properties which are applied on systemd1.

     // TODO: We need to capture the service restart signal and reload data

-    // inside the signal handler. So that we can update the service properties

-    // modified, outside of this service as well.

+    // inside the signal handler. So that we can update the service

+    // properties modified, outside of this service as well.

     syncWithSystemD1Properties();

 

     return;

@@ -330,8 +363,6 @@ void ServiceConfig::registerProperties()
 

     iface->register_property(

         "Port", portNum, [this](const uint16_t &req, uint16_t &res) {

-            phosphor::logging::log<phosphor::logging::level::ERR>(

-                " Inside register_property");

             if (req == res)

             {

                 return 1;

@@ -366,7 +397,7 @@ void ServiceConfig::registerProperties()
             {

                 return 1;

             }

-            if ((req != "enabled") && (req != "disabled") && (req != "static"))

+            if ((req != "enabled") && (req != "disabled"))

             {

                 phosphor::logging::log<phosphor::logging::level::ERR>(

                     "Invalid value specified");

diff --git a/srvcfg-manager/src/utils.cpp b/srvcfg-manager/src/utils.cpp
index 07239e2..32ac69d 100644
--- a/srvcfg-manager/src/utils.cpp
+++ b/srvcfg-manager/src/utils.cpp
@@ -28,6 +28,7 @@ void systemdDaemonReload(
                         "async error: Failed to do systemd reload.");

                     return;

                 }

+                return;

             },

             "org.freedesktop.systemd1", "/org/freedesktop/systemd1",

             "org.freedesktop.systemd1.Manager", "Reload");

@@ -51,13 +52,17 @@ void systemdUnitAction(const std::shared_ptr<sdbusplus::asio::connection> &conn,
     {

         conn->async_method_call(

             [](boost::system::error_code ec,

-               const sdbusplus::message::object_path &res) {

+               const sdbusplus::message::object_path &objPath) {

                 if (ec)

                 {

                     phosphor::logging::log<phosphor::logging::level::ERR>(

                         "async error: Failed to do systemd action");

                     return;

                 }

+                phosphor::logging::log<phosphor::logging::level::ERR>(

+                    "Created unit action job.",

+                    phosphor::logging::entry("JobID=%s", objPath.str.c_str()));

+                return;

             },

             "org.freedesktop.systemd1", "/org/freedesktop/systemd1",

             "org.freedesktop.systemd1.Manager", actionMethod, unitName,

@@ -75,15 +80,12 @@ void systemdUnitAction(const std::shared_ptr<sdbusplus::asio::connection> &conn,
     return;

 }

 

-void systemdUnitFileStateChange(

+void systemdUnitFilesStateChange(

     const std::shared_ptr<sdbusplus::asio::connection> &conn,

-    const std::string &unitName, const std::string &unitState)

+    const std::vector<std::string> &unitFiles, const std::string &unitState)

 {

     try

     {

-        // For now, we support two states(enabled & disabled). we can extend

-        // to other states (enable-runtime, mask, link etc..) if needed.

-        std::vector<std::string> unitFiles = {unitName};

         if (unitState == "enabled")

         {

             conn->async_method_call(

@@ -91,13 +93,14 @@ void systemdUnitFileStateChange(
                     if (ec)

                     {

                         phosphor::logging::log<phosphor::logging::level::ERR>(

-                            "async error: Failed to do systemd reload.");

+                            "async error: Failed to perform UnmaskUnitFiles.");

                         return;

                     }

+                    return;

                 },

                 "org.freedesktop.systemd1", "/org/freedesktop/systemd1",

-                "org.freedesktop.systemd1.Manager", "EnableUnitFiles",

-                unitFiles, false, false);

+                "org.freedesktop.systemd1.Manager", "UnmaskUnitFiles",

+                unitFiles, false);

         }

         else if (unitState == "disabled")

         {

@@ -106,13 +109,14 @@ void systemdUnitFileStateChange(
                     if (ec)

                     {

                         phosphor::logging::log<phosphor::logging::level::ERR>(

-                            "async error: Failed to do systemd reload.");

+                            "async error: Failed to perform MaskUnitFiles.");

                         return;

                     }

+                    return;

                 },

                 "org.freedesktop.systemd1", "/org/freedesktop/systemd1",

-                "org.freedesktop.systemd1.Manager", "DisableUnitFiles",

-                unitFiles, false);

+                "org.freedesktop.systemd1.Manager", "MaskUnitFiles", unitFiles,

+                false, false);

         }

         else

         {