Avoid nested GMainLoops by reading from the file on startup

Restore some tests for this functionality.

4 files changed, 174 insertions, 61 deletions

diff --git a/include/datetime/timezone-timedated.h b/include/datetime/timezone-timedated.h
index 3df9a3e..5978e3e 100644
--- a/include/datetime/timezone-timedated.h
+++ b/include/datetime/timezone-timedated.h
@@ -20,6 +20,8 @@
 #ifndef INDICATOR_DATETIME_TIMEDATED_TIMEZONE_H
 #define INDICATOR_DATETIME_TIMEDATED_TIMEZONE_H
 
+#define DEFAULT_FILENAME "/etc/timezone"
+
 #include <datetime/timezone.h> // base class
 
 #include <string> // std::string
@@ -34,7 +36,7 @@ namespace datetime {
 class TimedatedTimezone: public Timezone
 {
 public:
-    TimedatedTimezone();
+    TimedatedTimezone(std::string filename = DEFAULT_FILENAME);
     ~TimedatedTimezone();
 
 private:
diff --git a/src/timezone-timedated.cpp b/src/timezone-timedated.cpp
index bfe38f4..a2918f0 100644
--- a/src/timezone-timedated.cpp
+++ b/src/timezone-timedated.cpp
@@ -36,10 +36,11 @@ class TimedatedTimezone::Impl
 {
 public:
 
-    Impl(TimedatedTimezone& owner):
+    Impl(TimedatedTimezone& owner, std::string filename):
         m_owner(owner),
-        m_loop(g_main_loop_new(nullptr, FALSE))
+        m_filename(filename)
     {
+        g_debug("Filename is '%s'", filename.c_str());
         monitor_timezone_property();
     }
 
@@ -64,14 +65,7 @@ private:
             m_properties_changed_id = 0;
         }
 
-        if (m_timeout_id)
-        {
-            g_source_remove(m_timeout_id);
-            m_timeout_id = 0;
-        }
-
         g_clear_object(&m_proxy);
-        g_clear_pointer(&m_loop, g_main_loop_unref);
     }
 
     static void on_properties_changed(GDBusProxy *proxy G_GNUC_UNUSED,
@@ -128,14 +122,6 @@ private:
 out:
         g_clear_pointer(&error, g_error_free);
         g_clear_pointer(&prop, g_variant_unref);
-        if (self->m_loop && g_main_loop_is_running(self->m_loop))
-            g_main_loop_quit(self->m_loop);
-
-        if (self->m_timeout_id)
-        {
-            g_source_remove(self->m_timeout_id);
-            self->m_timeout_id = 0;
-        }
     }
 
     static void on_name_appeared(GDBusConnection *connection,
@@ -155,20 +141,6 @@ out:
                 gself);
     }
 
-    static gboolean quit_loop(gpointer gself)
-    {
-        auto self = static_cast<Impl*>(gself);
-
-        g_warning("Timed out when getting initial value of timezone, defaulting to UTC");
-        self->notify_timezone("Etc/Utc");
-
-        g_main_loop_quit(self->m_loop);
-
-        self->m_timeout_id = 0;
-
-        return G_SOURCE_REMOVE;
-    }
-
     static void on_name_vanished(GDBusConnection *connection G_GNUC_UNUSED,
             const gchar *name G_GNUC_UNUSED,
             gpointer gself)
@@ -185,27 +157,100 @@ out:
 
     void monitor_timezone_property()
     {
-        m_bus_watch_id = g_bus_watch_name (G_BUS_TYPE_SYSTEM,
+        GError *err = nullptr;
+        GDBusConnection *conn;
+
+        /*
+         * There is an unlikely race which happens if there is an activation
+         * and timezone change before our match rule is added.
+         */
+        notify_timezone(get_timezone_from_file(m_filename));
+
+        /*
+         * Make sure the bus is around at least until we add the match rules,
+         * otherwise things (tests) are sad.
+         */
+        conn = g_bus_get_sync(G_BUS_TYPE_SYSTEM,
+                nullptr,
+                &err);
+
+        if (err)
+        {
+            g_warning("Couldn't get bus connection: '%s'", err->message);
+            g_error_free(err);
+            return;
+        }
+
+        m_bus_watch_id = g_bus_watch_name_on_connection(conn,
                 "org.freedesktop.timedate1",
-                G_BUS_NAME_WATCHER_FLAGS_AUTO_START,
+                G_BUS_NAME_WATCHER_FLAGS_NONE,
                 on_name_appeared,
                 on_name_vanished,
                 this,
                 nullptr);
 
-        /* Incase something breaks, we don't want to hang */
-        m_timeout_id = g_timeout_add(500, quit_loop, this);
-
-        /* We need to block until we've read the tz once */
-        g_main_loop_run(m_loop);
+        g_object_unref (conn);
     }
 
     void notify_timezone(std::string new_timezone)
     {
+        g_debug("notify_timezone '%s'", new_timezone.c_str());
         if (!new_timezone.empty())
             m_owner.timezone.set(new_timezone);
     }
 
+    std::string get_timezone_from_file(const std::string& filename)
+    {
+        GError * error;
+        GIOChannel * io_channel;
+        std::string ret;
+
+        // read through filename line-by-line until we fine a nonempty non-comment line
+        error = nullptr;
+        io_channel = g_io_channel_new_file(filename.c_str(), "r", &error);
+        if (error == nullptr)
+        {
+            auto line = g_string_new(nullptr);
+
+            while(ret.empty())
+            {
+                const auto io_status = g_io_channel_read_line_string(io_channel, line, nullptr, &error);
+                if ((io_status == G_IO_STATUS_EOF) || (io_status == G_IO_STATUS_ERROR))
+                    break;
+                if (error != nullptr)
+                    break;
+
+                g_strstrip(line->str);
+
+                if (!line->len) // skip empty lines
+                    continue;
+
+                if (*line->str=='#') // skip comments
+                    continue;
+
+                ret = line->str;
+            }
+
+            g_string_free(line, true);
+        } else
+            /* Default to UTC */
+            ret = "Etc/Utc";
+
+        if (io_channel != nullptr)
+        {
+            g_io_channel_shutdown(io_channel, false, nullptr);
+            g_io_channel_unref(io_channel);
+        }
+
+        if (error != nullptr)
+        {
+            g_warning("%s Unable to read timezone file '%s': %s", G_STRLOC, filename.c_str(), error->message);
+            g_error_free(error);
+        }
+
+        return ret;
+    }
+
     /***
     ****
     ***/
@@ -213,17 +258,16 @@ out:
     TimedatedTimezone & m_owner;
     unsigned long m_properties_changed_id = 0;
     unsigned long m_bus_watch_id = 0;
-    unsigned long m_timeout_id = 0;
     GDBusProxy *m_proxy = nullptr;
-    GMainLoop *m_loop = nullptr;
+    std::string m_filename;
 };
 
 /***
 ****
 ***/
 
-TimedatedTimezone::TimedatedTimezone():
-    impl(new Impl{*this})
+TimedatedTimezone::TimedatedTimezone(std::string filename):
+    impl(new Impl{*this, filename})
 {
 }
 
diff --git a/tests/test-timezone-timedated.cpp b/tests/test-timezone-timedated.cpp
index 7086e96..5cfc311 100644
--- a/tests/test-timezone-timedated.cpp
+++ b/tests/test-timezone-timedated.cpp
@@ -22,43 +22,99 @@
 
 #include <datetime/timezone-timedated.h>
 
-#include <cstdio> // fopen()
-//#include <sys/stat.h> // chmod()
-#include <unistd.h> // sync()
-
 using unity::indicator::datetime::TimedatedTimezone;
 
+/***
+****
+***/
+
+#define TIMEZONE_FILE (SANDBOX"/timezone")
+
+class TimezoneFixture: public TimedateFixture
+{
+  private:
+
+    typedef TimedateFixture super;
+
+  protected:
+
+    void SetUp() override
+    {
+      super::SetUp();
+    }
+
+    void TearDown() override
+    {
+      super::TearDown();
+    }
+
+  public:
+
+    /* convenience func to set the timezone file */
+    void set_file(const std::string& text)
+    {
+      g_debug("set_file %s %s", TIMEZONE_FILE, text.c_str());
+      auto fp = fopen(TIMEZONE_FILE, "w+");
+      fprintf(fp, "%s\n", text.c_str());
+      fclose(fp);
+      sync();
+    }
+};
+
 /**
- * Test that timezone-file picks up the initial value
+ * Test that timezone-timedated warns, but doesn't crash, if the timezone file doesn't exist
  */
-TEST_F(TimedateFixture, InitialValue)
+TEST_F(TimezoneFixture, NoFile)
 {
-  const std::string expected_timezone = "America/Chicago";
-  set_timezone(expected_timezone);
-  TimedatedTimezone tz;
+  remove(TIMEZONE_FILE);
+  ASSERT_FALSE(g_file_test(TIMEZONE_FILE, G_FILE_TEST_EXISTS));
+
+  TimedatedTimezone tz(TIMEZONE_FILE);
+  testLogCount(G_LOG_LEVEL_WARNING, 1);
+}
+
+/**
+ * Test that timezone-timedated gives a default of UTC if the file doesn't exist
+ */
+TEST_F(TimezoneFixture, DefaultValueNoFile)
+{
+  const std::string expected_timezone = "Etc/Utc";
+  remove(TIMEZONE_FILE);
+  ASSERT_FALSE(g_file_test(TIMEZONE_FILE, G_FILE_TEST_EXISTS));
+
+  TimedatedTimezone tz(TIMEZONE_FILE);
   ASSERT_EQ(expected_timezone, tz.timezone.get());
 }
 
 /**
+ * Test that timezone-timedated picks up the initial value
+ */
+TEST_F(TimezoneFixture, InitialValue)
+{
+  const std::string expected_timezone = "America/Chicago";
+  set_file(expected_timezone);
+  TimedatedTimezone tz(TIMEZONE_FILE);
+}
+
+/**
  * Test that changing the tz after we are running works.
  */
-TEST_F(TimedateFixture, ChangedValue)
+TEST_F(TimezoneFixture, ChangedValue)
 {
   const std::string initial_timezone = "America/Chicago";
   const std::string changed_timezone = "America/New_York";
-  GMainLoop *l = g_main_loop_new(nullptr, FALSE);
 
-  set_timezone(initial_timezone);
+  set_file(initial_timezone);
 
-  TimedatedTimezone tz;
+  TimedatedTimezone tz(TIMEZONE_FILE);
   ASSERT_EQ(initial_timezone, tz.timezone.get());
 
   bool changed = false;
   tz.timezone.changed().connect(
-        [&changed, this, l](const std::string& s){
+        [&changed, this](const std::string& s){
           g_message("timezone changed to %s", s.c_str());
           changed = true;
-          g_main_loop_quit(l);
+          g_main_loop_quit(loop);
         });
 
   g_idle_add([](gpointer gself){
@@ -66,8 +122,20 @@ TEST_F(TimedateFixture, ChangedValue)
     return G_SOURCE_REMOVE;
   }, this);
 
-  g_main_loop_run(l);
+  g_main_loop_run(loop);
 
   ASSERT_TRUE(changed);
   ASSERT_EQ(changed_timezone, tz.timezone.get());
 }
+
+/**
+ * Test that timezone-timedated picks up the initial value
+ */
+TEST_F(TimezoneFixture, IgnoreComments)
+{
+  const std::string comment = "# Created by cloud-init v. 0.7.5 on Thu, 24 Apr 2014 14:03:29 +0000";
+  const std::string expected_timezone = "Europe/Berlin";
+  set_file(comment + "\n" + expected_timezone);
+  TimedatedTimezone tz(TIMEZONE_FILE);
+  ASSERT_EQ(expected_timezone, tz.timezone.get());
+}
diff --git a/tests/timedated-fixture.h b/tests/timedated-fixture.h
index 0e89ea1..5ec425d 100644
--- a/tests/timedated-fixture.h
+++ b/tests/timedated-fixture.h
@@ -183,8 +183,6 @@ private:
                                        &local_error);
         g_assert_no_error (local_error);
         g_variant_unref(child);
-
-        g_main_loop_quit(self->loop);
     }
 
 protected:
@@ -215,6 +213,7 @@ protected:
         node_info = nullptr;
         object_register_id = 0;
         own_name = 0;
+        proxy = nullptr;
 
         // bring up the test bus
         bus = g_test_dbus_new(G_TEST_DBUS_NONE);