pull the timezone from timedate1 regardless of whether it appears on the bus before or after we startup

4 files changed, 283 insertions, 198 deletions

diff --git a/include/datetime/dbus-shared.h b/include/datetime/dbus-shared.h
index 057ac6b..fd43ae8 100644
--- a/include/datetime/dbus-shared.h
+++ b/include/datetime/dbus-shared.h
@@ -28,5 +28,47 @@
 #define BUS_POWERD_PATH      "/com/canonical/powerd"
 #define BUS_POWERD_INTERFACE "com.canonical.powerd"
 
+namespace ayatana {
+namespace indicator {
+namespace datetime {
+
+namespace Bus
+{
+    namespace Timedate1
+    {
+        static constexpr char const * BUSNAME {"org.freedesktop.timedate1"};
+        static constexpr char const * ADDR {"/org/freedesktop/timedate1"};
+        static constexpr char const * IFACE {"org.freedesktop.timedate1"};
+
+        namespace Properties
+        {
+            static constexpr char const * TIMEZONE {"Timezone"};
+        }
+
+        namespace Methods
+        {
+            static constexpr char const * SET_TIMEZONE {"SetTimezone"};
+        }
+    }
+
+    namespace Properties
+    {
+        static constexpr char const * IFACE {"org.freedesktop.DBus.Properties"};
+
+        namespace Methods
+        {
+            static constexpr char const * GET {"Get"};
+        }
+
+        namespace Signals
+        {
+            static constexpr char const * PROPERTIES_CHANGED {"PropertiesChanged"};
+        }
+    }
+}
+
+} // namespace datetime
+} // namespace indicator
+} // namespace ayatana
 
 #endif /* INDICATOR_DATETIME_DBUS_SHARED_H */
diff --git a/include/datetime/timezone-timedated.h b/include/datetime/timezone-timedated.h
index 336a148..0857706 100644
--- a/include/datetime/timezone-timedated.h
+++ b/include/datetime/timezone-timedated.h
@@ -20,8 +20,6 @@
 #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
@@ -31,12 +29,12 @@ namespace indicator {
 namespace datetime {
 
 /**
- * \brief A #Timezone that gets its information from monitoring a file, such as /etc/timezone
+ * \brief A #Timezone that gets its information from org.freedesktop.timedate1
  */
 class TimedatedTimezone: public Timezone
 {
 public:
-    TimedatedTimezone(std::string filename = DEFAULT_FILENAME);
+    TimedatedTimezone();
     ~TimedatedTimezone();
 
 private:
diff --git a/src/timezone-timedated.cpp b/src/timezone-timedated.cpp
index d38557b..9254612 100644
--- a/src/timezone-timedated.cpp
+++ b/src/timezone-timedated.cpp
@@ -17,6 +17,7 @@
  *   Charles Kerr <charles.kerr@canonical.com>
  */
 
+#include <datetime/dbus-shared.h>
 #include <datetime/timezone-timedated.h>
 
 #include <gio/gio.h>
@@ -36,166 +37,186 @@ class TimedatedTimezone::Impl
 {
 public:
 
-    Impl(TimedatedTimezone& owner, std::string filename):
-        m_owner(owner),
-        m_filename(filename)
+    Impl(TimedatedTimezone& owner):
+        m_owner{owner},
+        m_cancellable{g_cancellable_new()}
     {
-        g_debug("Filename is '%s'", filename.c_str());
-        monitor_timezone_property();
+        // set the fallback value
+        m_owner.timezone.set("Etc/Utc");
+
+        // watch for timedate1 on the bus
+        m_watcher_id = g_bus_watch_name(
+            G_BUS_TYPE_SYSTEM,
+            Bus::Timedate1::BUSNAME,
+            G_BUS_NAME_WATCHER_FLAGS_AUTO_START,
+            on_timedate1_appeared,
+            on_timedate1_vanished,
+            this,
+            nullptr);
     }
 
     ~Impl()
     {
-        clear();
-    }
+        g_cancellable_cancel(m_cancellable);
+        g_clear_object(&m_cancellable);
 
-private:
+        g_bus_unwatch_name(m_watcher_id);
 
-    void clear()
-    {
-        if (m_connection && m_signal_subscription_id)
+        if (m_connection != nullptr)
         {
-            g_dbus_connection_signal_unsubscribe (m_connection, m_signal_subscription_id);
-            m_signal_subscription_id = 0;
-        }
+            if (m_signal_subscription_id)
+                g_dbus_connection_signal_unsubscribe(m_connection, m_signal_subscription_id);
 
-        g_clear_object(&m_connection);
+            g_clear_object(&m_connection);
+        }
     }
 
-    static void on_properties_changed (GDBusConnection *connection G_GNUC_UNUSED,
-            const gchar *sender_name G_GNUC_UNUSED,
-            const gchar *object_path G_GNUC_UNUSED,
-            const gchar *interface_name G_GNUC_UNUSED,
-            const gchar *signal_name G_GNUC_UNUSED,
-            GVariant *parameters,
-            gpointer gself)
-    {
-        auto self = static_cast<Impl*>(gself);
-        const char *tz;
-        GVariant *changed_properties;
-        gchar **invalidated_properties;
-
-        g_variant_get (parameters, "(s@a{sv}^as)", NULL, &changed_properties, &invalidated_properties);
+private:
 
-        if (g_variant_lookup(changed_properties, "Timezone", "&s", &tz, NULL))
-            self->notify_timezone(tz);
-        else if (g_strv_contains (invalidated_properties, "Timezone"))
-            self->notify_timezone(self->get_timezone_from_file(self->m_filename));
+    static void on_timedate1_appeared(GDBusConnection * connection,
+                                      const gchar     * /*name*/,
+                                      const gchar     * /*name_owner*/,
+                                      gpointer          gself)
+    {
+        static_cast<Impl*>(gself)->timedate1_appeared(connection);
+    }
+    void timedate1_appeared(GDBusConnection* connection)
+    {
+        // cache m_connection for later...
+        g_clear_object(&m_connection);
+        m_connection = G_DBUS_CONNECTION(g_object_ref(G_OBJECT(connection)));
 
-        g_variant_unref (changed_properties);
-        g_strfreev (invalidated_properties);
+        ensure_propchange_subscription();
+        ask_for_timezone();
     }
 
-    void monitor_timezone_property()
+    void ensure_propchange_subscription()
     {
-        GError *err = nullptr;
-
-        /*
-         * 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.
-         */
-        m_connection = g_bus_get_sync(G_BUS_TYPE_SYSTEM,
-                nullptr,
-                &err);
-
-        if (err)
+        if (m_signal_subscription_id == 0)
         {
-            g_warning("Couldn't get bus connection: '%s'", err->message);
-            g_error_free(err);
-            return;
-        }
-
-        m_signal_subscription_id = g_dbus_connection_signal_subscribe(m_connection,
-                "org.freedesktop.timedate1",
-                "org.freedesktop.DBus.Properties",
-                "PropertiesChanged",
-                "/org/freedesktop/timedate1",
-                NULL, G_DBUS_SIGNAL_FLAGS_NONE,
+            m_signal_subscription_id = g_dbus_connection_signal_subscribe(
+                m_connection,
+                Bus::Timedate1::IFACE,
+                Bus::Properties::IFACE,
+                Bus::Properties::Signals::PROPERTIES_CHANGED,
+                Bus::Timedate1::ADDR,
+                nullptr,
+                G_DBUS_SIGNAL_FLAGS_NONE,
                 on_properties_changed,
-                this, nullptr);
+                this,
+                nullptr);
+        }
     }
 
-    void notify_timezone(std::string new_timezone)
+    static void on_timedate1_vanished(GDBusConnection * /*connection*/,
+                                      const gchar     * name,
+                                      gpointer          /*gself*/)
     {
-        g_debug("notify_timezone '%s'", new_timezone.c_str());
-        if (!new_timezone.empty())
-            m_owner.timezone.set(new_timezone);
+        g_debug("%s not present on system bus", name);
     }
 
-    std::string get_timezone_from_file(const std::string& filename)
+    static void on_properties_changed(GDBusConnection * /*connection*/,
+                                      const gchar     * /*sender_name*/,
+                                      const gchar     * /*object_path*/,
+                                      const gchar     * /*interface_name*/,
+                                      const gchar     * /*signal_name*/,
+                                      GVariant        * parameters,
+                                      gpointer          gself)
     {
-        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);
+        auto self = static_cast<Impl*>(gself);
 
-                if (!line->len) // skip empty lines
-                    continue;
+        GVariant* changed_properties {};
+        gchar** invalidated_properties {};
+        g_variant_get(parameters, "(s@a{sv}^as)", NULL, &changed_properties, &invalidated_properties);
 
-                if (*line->str=='#') // skip comments
-                    continue;
+        const char* tz {};
+        if (g_variant_lookup(changed_properties, Bus::Timedate1::Properties::TIMEZONE, "&s", &tz, NULL))
+        {
+            if (tz != nullptr)
+                self->set_timezone(tz);
+            else
+                g_warning("%s no timezone found", G_STRLOC);
+        }
+        else if (g_strv_contains(invalidated_properties, Bus::Timedate1::Properties::TIMEZONE))
+        {
+            self->ask_for_timezone();
+        }
 
-                ret = line->str;
-            }
+        g_variant_unref(changed_properties);
+        g_strfreev(invalidated_properties);
+    }
 
-            g_string_free(line, true);
-        } else
-            /* Default to UTC */
-            ret = "Etc/Utc";
+    void ask_for_timezone()
+    {
+        g_return_if_fail(m_connection != nullptr);
+
+        g_dbus_connection_call(
+            m_connection,
+            Bus::Timedate1::BUSNAME,
+            Bus::Timedate1::ADDR,
+            Bus::Properties::IFACE,
+            Bus::Properties::Methods::GET,
+            g_variant_new("(ss)", Bus::Timedate1::IFACE, Bus::Timedate1::Properties::TIMEZONE),
+            G_VARIANT_TYPE("(v)"),
+            G_DBUS_CALL_FLAGS_NONE,
+            -1,
+            m_cancellable,
+            on_get_timezone_ready,
+            this);
+    }
 
-        if (io_channel != nullptr)
+    static void on_get_timezone_ready(GObject       * connection,
+                                      GAsyncResult  * res,
+                                      gpointer        gself)
+    {
+        GError* error {};
+        GVariant* v = g_dbus_connection_call_finish(G_DBUS_CONNECTION(connection), res, &error);
+        if (v == nullptr)
         {
-            g_io_channel_shutdown(io_channel, false, nullptr);
-            g_io_channel_unref(io_channel);
+            if (!g_error_matches(error, G_IO_ERROR, G_IO_ERROR_CANCELLED))
+                g_warning("%s Couldn't get timezone: %s", G_STRLOC, error->message);
         }
-
-        if (error != nullptr)
+        else
         {
-            g_warning("%s Unable to read timezone file '%s': %s", G_STRLOC, filename.c_str(), error->message);
-            g_error_free(error);
+            GVariant* tzv {};
+            g_variant_get(v, "(v)", &tzv);
+            const char* tz = g_variant_get_string(tzv, nullptr);
+
+            if (tz != nullptr)
+                static_cast<Impl*>(gself)->set_timezone(tz);
+            else
+                g_warning("%s no timezone found", G_STRLOC);
+
+            g_clear_pointer(&tzv, g_variant_unref);
+            g_clear_pointer(&v, g_variant_unref);
         }
+    }
+
+    void set_timezone(const std::string& tz)
+    {
+        g_return_if_fail(!tz.empty());
 
-        return ret;
+        g_debug("set timezone: '%s'", tz.c_str());
+        m_owner.timezone.set(tz);
     }
 
     /***
     ****
     ***/
 
-    TimedatedTimezone & m_owner;
-    GDBusConnection *m_connection = nullptr;
-    unsigned long m_signal_subscription_id = 0;
-    std::string m_filename;
+    TimedatedTimezone& m_owner;
+    GCancellable* m_cancellable {};
+    GDBusConnection* m_connection {};
+    unsigned long m_signal_subscription_id {};
+    unsigned int m_watcher_id {};
 };
 
 /***
 ****
 ***/
 
-TimedatedTimezone::TimedatedTimezone(std::string filename):
-    impl(new Impl{*this, filename})
+TimedatedTimezone::TimedatedTimezone():
+    impl{new Impl{*this}}
 {
 }
 
diff --git a/tests/test-timezone-timedated.cpp b/tests/test-timezone-timedated.cpp
index 2fdec12..b293e59 100644
--- a/tests/test-timezone-timedated.cpp
+++ b/tests/test-timezone-timedated.cpp
@@ -1,9 +1,5 @@
-
 /*
- * Copyright 2013 Canonical Ltd.
- *
- * Authors:
- *   Charles Kerr <charles.kerr@canonical.com>
+ * Copyright © 2014-2016 Canonical Ltd.
  *
  * This program is free software: you can redistribute it and/or modify it
  * under the terms of the GNU General Public License version 3, as published
@@ -16,127 +12,155 @@
  *
  * You should have received a copy of the GNU General Public License along
  * with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authors:
+ *   Charles Kerr <charles.kerr@canonical.com>
+ *   Ted Gould <ted.gould@canonical.com>
  */
 
-#include "timedated-fixture.h"
+#include "glib-fixture.h"
 
+#include <datetime/dbus-shared.h>
 #include <datetime/timezone-timedated.h>
 
-using ayatana::indicator::datetime::TimedatedTimezone;
+#include <gio/gio.h>
 
-/***
-****
-***/
 
-#define TIMEZONE_FILE (SANDBOX"/timezone")
+using namespace ayatana::indicator::datetime;
+
 
-class TimezoneFixture: public TimedateFixture
+struct Timedate1Fixture: public GlibFixture
 {
-  private:
+private:
+
+    typedef GlibFixture super;
 
-    typedef TimedateFixture super;
+protected:
 
-  protected:
+    GDBusConnection* m_bus {};
+    GTestDBus* m_test_bus {};
 
     void SetUp() override
     {
-      super::SetUp();
+        super::SetUp();
+
+        // use a fake bus
+        m_test_bus = g_test_dbus_new(G_TEST_DBUS_NONE);
+        g_test_dbus_up(m_test_bus);
+        const char * address = g_test_dbus_get_bus_address(m_test_bus);
+        g_setenv("DBUS_SYSTEM_BUS_ADDRESS", address, true);
+        g_setenv("DBUS_SESSION_BUS_ADDRESS", address, true);
+        g_debug("test_dbus's address is %s", address);
+
+        // get the bus
+        m_bus = g_bus_get_sync(G_BUS_TYPE_SESSION, nullptr, nullptr);
+        g_dbus_connection_set_exit_on_close(m_bus, FALSE);
+        g_object_add_weak_pointer(G_OBJECT(m_bus), (gpointer*)&m_bus);
     }
 
     void TearDown() override
     {
-      super::TearDown();
+        g_clear_object(&m_bus);
+        g_clear_object(&m_test_bus);
+
+        super::TearDown();
     }
 
-  public:
+    void start_timedate1(const std::string& tzid)
+    {
+        // start the store
+        auto json_parameters = g_strdup_printf("{\"Timezone\": \"%s\"}", tzid.c_str());
+        const gchar* child_argv[] = { "python3", "-m", "dbusmock", "--template", "timedated", "--parameters", json_parameters, nullptr };
+        GError* error = nullptr;
+        g_spawn_async(nullptr, (gchar**)child_argv, nullptr, G_SPAWN_SEARCH_PATH, nullptr, nullptr, nullptr, &error);
+        g_assert_no_error(error);
+        g_free(json_parameters);
+
+        // wait for it to appear on the bus
+        wait_for_name_owned(m_bus, Bus::Timedate1::BUSNAME);
+    }
+
+    bool wait_for_tzid(Timezone& tz, const std::string& tzid)
+    {
+        return wait_for([&tz, &tzid](){return tzid == tz.timezone.get();});
+    }
 
-    /* convenience func to set the timezone file */
-    void set_file(const std::string& text)
+    void set_timedate1_timezone(const std::string& tzid)
     {
-      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();
+        GError* error {};
+        auto v = g_dbus_connection_call_sync(
+            m_bus,
+            Bus::Timedate1::BUSNAME,
+            Bus::Timedate1::ADDR,
+            Bus::Timedate1::IFACE,
+            Bus::Timedate1::Methods::SET_TIMEZONE,
+            g_variant_new("(sb)", tzid.c_str(), FALSE),
+            nullptr,
+            G_DBUS_CALL_FLAGS_NONE,
+            -1,
+            nullptr,
+            &error);
+        g_clear_pointer(&v, g_variant_unref);
+        g_assert_no_error(error);
     }
 };
 
-/**
- * Test that timezone-timedated warns, but doesn't crash, if the timezone file doesn't exist
- */
-TEST_F(TimezoneFixture, NoFile)
-{
-  remove(TIMEZONE_FILE);
-  ASSERT_FALSE(g_file_test(TIMEZONE_FILE, G_FILE_TEST_EXISTS));
+/***
+****
+***/
 
-  expectLogMessage(G_LOG_DOMAIN, G_LOG_LEVEL_WARNING, "*No such file or directory*");
-  TimedatedTimezone tz(TIMEZONE_FILE);
+TEST_F(Timedate1Fixture, HelloWorld)
+{
 }
 
 /**
- * Test that timezone-timedated gives a default of UTC if the file doesn't exist
+ * Test that we get a default timezone of UTC if timedate1 isn't available on the bus
  */
-TEST_F(TimezoneFixture, DefaultValueNoFile)
+TEST_F(Timedate1Fixture, DefaultTimezone)
 {
-  const std::string expected_timezone = "Etc/Utc";
-  remove(TIMEZONE_FILE);
-  ASSERT_FALSE(g_file_test(TIMEZONE_FILE, G_FILE_TEST_EXISTS));
+    const std::string expected_tzid{"Etc/Utc"};
 
-  expectLogMessage(G_LOG_DOMAIN, G_LOG_LEVEL_WARNING, "*No such file or directory*");
-  TimedatedTimezone tz(TIMEZONE_FILE);
-  ASSERT_EQ(expected_timezone, tz.timezone.get());
+    TimedatedTimezone tmp;
+    EXPECT_TRUE(wait_for_tzid(tmp, expected_tzid)) << "expected " << expected_tzid << " got " << tmp.timezone.get();
 }
 
 /**
- * Test that timezone-timedated picks up the initial value
+ * Test that we get the right tzid if timedated shows up on the bus BEFORE we start
  */
-TEST_F(TimezoneFixture, InitialValue)
+TEST_F(Timedate1Fixture, Timedate1First)
 {
-  const std::string expected_timezone = "America/Chicago";
-  set_file(expected_timezone);
-  TimedatedTimezone tz(TIMEZONE_FILE);
+    const std::string expected_tzid{"America/Chicago"};
+
+    start_timedate1(expected_tzid);
+    TimedatedTimezone tmp;
+    EXPECT_TRUE(wait_for_tzid(tmp, expected_tzid)) << "expected " << expected_tzid << " got " << tmp.timezone.get();
 }
 
 /**
- * Test that changing the tz after we are running works.
+ * Test that we get the right tzid if timedated shows up on the bus AFTER we start
  */
-TEST_F(TimezoneFixture, ChangedValue)
+TEST_F(Timedate1Fixture, Timedate1Last)
 {
-  const std::string initial_timezone = "America/Chicago";
-  const std::string changed_timezone = "America/New_York";
-
-  set_file(initial_timezone);
+    const std::string expected_tzid("America/Los_Angeles");
 
-  TimedatedTimezone tz(TIMEZONE_FILE);
-  ASSERT_EQ(initial_timezone, tz.timezone.get());
-
-  bool changed = false;
-  tz.timezone.changed().connect(
-        [&changed, this](const std::string& s){
-          g_message("timezone changed to %s", s.c_str());
-          changed = true;
-          g_main_loop_quit(loop);
-        });
-
-  g_idle_add([](gpointer gself){
-    static_cast<TimedateFixture*>(gself)->set_timezone("America/New_York");
-    return G_SOURCE_REMOVE;
-  }, this);
-
-  g_main_loop_run(loop);
-
-  ASSERT_TRUE(changed);
-  ASSERT_EQ(changed_timezone, tz.timezone.get());
+    TimedatedTimezone tmp;
+    start_timedate1(expected_tzid);
+    EXPECT_TRUE(wait_for_tzid(tmp, expected_tzid)) << "expected " << expected_tzid << " got " << tmp.timezone.get();
 }
 
 /**
- * Test that timezone-timedated picks up the initial value
+ * Test that the timezone core::Property changes when timedate1's property changes
  */
-TEST_F(TimezoneFixture, IgnoreComments)
+TEST_F(Timedate1Fixture, TimezoneChange)
 {
-  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());
+    const std::vector<std::string> expected_tzids{"America/Los_Angeles", "America/Chicago", "Etc/Utc"};
+
+    TimedatedTimezone tmp;
+    start_timedate1("America/New_York");
+
+    for(const auto& expected_tzid : expected_tzids)
+    {
+        set_timedate1_timezone(expected_tzid);
+        EXPECT_TRUE(wait_for_tzid(tmp, expected_tzid)) << "expected " << expected_tzid << " got " << tmp.timezone.get();
+    }
 }