Merge lp:~ted/ubuntu-app-launch/reset-units into lp:ubuntu-app-launch

[Ted Gould]

Reset failed units so they can be tried again

[lp:~ted/ubuntu-app-launch/reset-units]

406. By Ted Gould on 2017-02-03

Merging future trunk

407. By Ted Gould on 2017-02-04

Add a test that catches the error

408. By Ted Gould on 2017-02-04

Check to ensure we get the reset on a failure

409. By Ted Gould on 2017-02-04

Add comment to describe the function

410. By Ted Gould on 2017-02-04

Attaching bug

Unmerged revisions

=== modified file 'libubuntu-app-launch/jobs-systemd.cpp'
--- libubuntu-app-launch/jobs-systemd.cpp	2017-02-02 22:54:33 +0000
+++ libubuntu-app-launch/jobs-systemd.cpp	2017-02-02 22:54:34 +0000
@@ -306,7 +306,7 @@
     auto unsub = [&](guint& handle) {
         if (handle != 0)
         {
-            g_dbus_connection_signal_unsubscribe(dbus_.get(), handle);
+            g_dbus_connection_signal_unsubscribe(userbus_.get(), handle);
             handle = 0;
         }
     };
@@ -425,12 +425,24 @@
     }
     auto uris = findEnv("APP_URIS", env);
 
+    g_debug("Exec line: %s", exec.c_str());
+    g_debug("App URLS:  %s", uris.c_str());
+
     auto execarray = desktop_exec_parse(exec.c_str(), uris.c_str());
 
-    std::vector<std::string> retval = {execarray->data, execarray->data + execarray->len};
+    std::vector<std::string> retval;
+    for (unsigned int i = 0; i < execarray->len; i++)
+    {
+        auto cstr = g_array_index(execarray, gchar*, i);
+        if (cstr != nullptr)
+        {
+            retval.emplace_back(cstr);
+        }
+    }
 
-    g_array_set_clear_func(execarray, g_free);
-    g_array_free(execarray, TRUE);
+    /* This seems to work better than array_free(), I can't figure out why */
+    auto strv = (gchar**)g_array_free(execarray, FALSE);
+    g_strfreev(strv);
 
     if (retval.empty())
     {
@@ -786,7 +798,7 @@
             auto retval = std::make_shared<instance::SystemD>(appId, job, instance, urls, registry);
             auto chelper = new StartCHelper{};
             chelper->ptr = retval;
-            chelper->bus = manager->userbus_;
+            chelper->bus = registry->impl->_dbus;
 
             tracepoint(ubuntu_app_launch, handshake_wait, appIdStr.c_str());
             starting_handshake_wait(handshake);
@@ -1269,7 +1281,7 @@
             auto data = new FailedData{reg};
 
             handle_appFailed = g_dbus_connection_signal_subscribe(
-                reg->impl->_dbus.get(),            /* bus */
+                userbus_.get(),                    /* bus */
                 SYSTEMD_DBUS_ADDRESS,              /* sender */
                 "org.freedesktop.DBus.Properties", /* interface */
                 "PropertiesChanged",               /* signal */
@@ -1330,6 +1342,9 @@
                     }
                     g_variant_dict_clear(&dict);
 
+                    /* Reset the failure bit on the unit */
+                    manager->resetUnit(unitinfo);
+
                     /* Oh, we might want to do something now */
                     auto reason{Registry::FailureType::CRASH};
                     if (g_strcmp0(value, "exit-code") == 0)
@@ -1356,6 +1371,46 @@
     return sig_appFailed;
 }
 
+void SystemD::resetUnit(const UnitInfo& info) const
+{
+    auto registry = registry_.lock();
+    auto unitname = unitName(info);
+    auto bus = userbus_;
+    auto cancel = registry->impl->thread.getCancellable();
+
+    registry->impl->thread.executeOnThread([bus, unitname, cancel] {
+        g_dbus_connection_call(bus.get(),                       /* user bus */
+                               SYSTEMD_DBUS_ADDRESS,            /* bus name */
+                               SYSTEMD_DBUS_PATH_MANAGER,       /* path */
+                               SYSTEMD_DBUS_IFACE_MANAGER,      /* interface */
+                               "ResetFailedUnit",               /* method */
+                               g_variant_new("(s)",             /* params */
+                                             unitname.c_str()), /* param: specify unit */
+                               nullptr,                         /* ret type */
+                               G_DBUS_CALL_FLAGS_NONE,          /* flags */
+                               -1,                              /* timeout */
+                               cancel.get(),                    /* cancellable */
+                               [](GObject* obj, GAsyncResult* res, gpointer user_data) {
+                                   GError* error{nullptr};
+                                   GVariant* callt = g_dbus_connection_call_finish(G_DBUS_CONNECTION(obj), res, &error);
+
+                                   if (error != nullptr)
+                                   {
+                                       if (!g_error_matches(error, G_IO_ERROR, G_IO_ERROR_CANCELLED))
+                                       {
+                                           g_warning("Unable to reset failed unit: %s", error->message);
+                                       }
+                                       g_error_free(error);
+                                       return;
+                                   }
+
+                                   g_clear_pointer(&callt, g_variant_unref);
+                                   g_debug("Reset Failed Unit");
+                               },
+                               nullptr);
+    });
+}
+
 }  // namespace manager
 }  // namespace jobs
 }  // namespace app_launch

=== modified file 'libubuntu-app-launch/jobs-systemd.h'
--- libubuntu-app-launch/jobs-systemd.h	2017-02-02 22:54:33 +0000
+++ libubuntu-app-launch/jobs-systemd.h	2017-02-02 22:54:34 +0000
@@ -128,6 +128,8 @@
 
     static std::vector<std::string> parseExec(std::list<std::pair<std::string, std::string>>& env);
     static void application_start_cb(GObject* obj, GAsyncResult* res, gpointer user_data);
+
+    void resetUnit(const UnitInfo& info) const;
 };
 
 }  // namespace manager

=== modified file 'tests/jobs-systemd.cpp'
--- tests/jobs-systemd.cpp	2017-02-02 22:54:33 +0000
+++ tests/jobs-systemd.cpp	2017-02-02 22:54:34 +0000
@@ -238,7 +238,7 @@
     std::function<std::list<std::pair<std::string, std::string>>()> getenvfunc =
         [&]() -> std::list<std::pair<std::string, std::string>> {
         gotenv = true;
-        return {};
+        return {{"APP_EXEC", "sh"}};
     };
 
     auto inst = manager->launch(multipleAppID(), defaultJobName(), "123", {},
@@ -265,6 +265,9 @@
         units.begin()->environment.end(),
         units.begin()->environment.find(std::string{"DBUS_SESSION_BUS_ADDRESS="} + getenv("DBUS_SESSION_BUS_ADDRESS")));
 
+    /* Ensure the exec is correct */
+    EXPECT_EQ("/bin/sh", units.begin()->execpath);
+
     /* Try an entirely custom variable */
     systemd->managerClear();
     units.clear();

=== modified file 'tests/systemd-mock.h'
--- tests/systemd-mock.h	2017-02-02 22:54:33 +0000
+++ tests/systemd-mock.h	2017-02-02 22:54:34 +0000
@@ -326,6 +326,8 @@
     {
         std::string name;
         std::set<std::string> environment;
+        std::string execpath;
+        std::list<std::string> execline;
     };
 
     std::list<TransientUnit> unitCalls()
@@ -366,15 +368,16 @@
             unit.name = name;
 
             auto paramarray = g_variant_get_child_value(call.params, 2);
-            gchar* key;
+            gchar* ckey;
             GVariant* var;
             GVariantIter iter;
             g_variant_iter_init(&iter, paramarray);
-            while (g_variant_iter_loop(&iter, "(sv)", &key, &var))
+            while (g_variant_iter_loop(&iter, "(sv)", &ckey, &var))
             {
-                g_debug("Looking at parameter: %s", key);
+                g_debug("Looking at parameter: %s", ckey);
+                std::string key{ckey};
 
-                if (std::string{key} == "Environment")
+                if (key == "Environment")
                 {
                     GVariantIter array;
                     gchar* envvar;
@@ -385,6 +388,41 @@
                         unit.environment.emplace(envvar);
                     }
                 }
+                else if (key == "ExecStart")
+                {
+                    /* a(sasb) */
+                    if (g_variant_n_children(var) > 1)
+                    {
+                        g_warning("'ExecStart' has more than one entry, only processing the first");
+                    }
+
+                    auto tuple = g_variant_get_child_value(var, 0);
+
+                    const gchar* cpath = nullptr;
+                    g_variant_get_child(tuple, 0, "&s", &cpath);
+
+                    if (cpath != nullptr)
+                    {
+                        unit.execpath = cpath;
+                    }
+                    else
+                    {
+                        g_warning("'ExecStart[0][0]' isn't a string?");
+                    }
+
+                    auto vexecarray = g_variant_get_child_value(tuple, 1);
+                    GVariantIter execarray;
+                    g_variant_iter_init(&execarray, vexecarray);
+                    const gchar* execentry;
+
+                    while (g_variant_iter_loop(&execarray, "&s", &execentry))
+                    {
+                        unit.execline.emplace_back(execentry);
+                    }
+
+                    g_clear_pointer(&vexecarray, g_variant_unref);
+                    g_clear_pointer(&tuple, g_variant_unref);
+                }
             }
             g_variant_unref(paramarray);