commit 74b657e8802d42d6eaf11f9b13018b1561a27a4b
author Gunnar Mills <gmills@us.ibm.com> Fri Jul 13 09:27:31 2018 -0500
committer Adriana Kobylak <anoo@us.ibm.com> Mon Jul 16 11:37:43 2018 -0500
tree 1f9c3383e594b9ff0d467be0ddf74e5667da2796
parent d1d19210e7bbffd5a6d7512539d13f8ec7d14b2a

activation: Improve error handling in Subscribe

Handle exceptions from the sdbusplus method call_noreply API.

The Activation constructor was subscribing to systemd signals, which
would cause an "Already Subscribed" error when creating multiple
Activation instances because the bus is common.

There is no reason to subscribe to systemd signals in the constructor,
since the signals are only triggered during the activation process.
Move the Subscribe call to the activation process which calls
Unsubscribe at the end.

There's a scenario where there could still be an "Already Subscribed"
error if the activation fails, so add error handling there.

This is very similar to:
https://gerrit.openbmc-project.xyz/#/c/11428/

Tested: Code updated an image.
Change-Id: Ia35b7f2fc24c0b605692bc534c54e18742027061
Signed-off-by: Gunnar Mills <gmills@us.ibm.com>

activation.cpp

diff --git a/activation.cpp b/activation.cpp
index 11c7b32..e807d04 100755
--- a/activation.cpp
+++ b/activation.cpp
@@ -4,6 +4,7 @@
 #include "item_updater.hpp"
 #include "serialize.hpp"
 #include <phosphor-logging/log.hpp>
+#include <sdbusplus/exception.hpp>
 
 #ifdef WANT_SIGNATURE_VERIFY
 #include <sdbusplus/server.hpp>
@@ -24,6 +25,7 @@
 namespace softwareServer = sdbusplus::xyz::openbmc_project::Software::server;
 
 using namespace phosphor::logging;
+using sdbusplus::exception::SdBusError;
 
 #ifdef WANT_SIGNATURE_VERIFY
 using InternalFailure =
@@ -41,8 +43,25 @@
 {
     auto method = this->bus.new_method_call(SYSTEMD_SERVICE, SYSTEMD_OBJ_PATH,
                                             SYSTEMD_INTERFACE, "Subscribe");
-    this->bus.call_noreply(method);
-
+    try
+    {
+        this->bus.call_noreply(method);
+    }
+    catch (const SdBusError& e)
+    {
+        if (e.name() != nullptr &&
+            strcmp("org.freedesktop.systemd1.AlreadySubscribed", e.name()) == 0)
+        {
+            // If an Activation attempt fails, the Unsubscribe method is not
+            // called. This may lead to an AlreadySubscribed error if the
+            // Activation is re-attempted.
+        }
+        else
+        {
+            log<level::ERR>("Error subscribing to systemd",
+                            entry("ERROR=%s", e.what()));
+        }
+    }
     return;
 }
 
@@ -122,6 +141,8 @@
 
         if (ubiVolumesCreated == false)
         {
+            // Enable systemd signals
+            Activation::subscribeToSystemdSignals();
 
 #ifdef WANT_SIGNATURE_VERIFY
             // Validate the signed image.

activation.hpp

diff --git a/activation.hpp b/activation.hpp
index 1a600cc..98646e5 100755
--- a/activation.hpp
+++ b/activation.hpp
@@ -193,8 +193,6 @@
             std::bind(std::mem_fn(&Activation::unitStateChange), this,
                       std::placeholders::_1))
     {
-        // Enable systemd signals
-        subscribeToSystemdSignals();
         // Set Properties.
         extendedVersion(extVersion);
         activation(activationStatus);