Nitpicking...
On Friday 03 December 2010 11:47:26 ext Antti Paila, you wrote:
diff --git a/plugins/nettime.c b/plugins/nettime.c
new file mode 100644
index 0000000..0f99bb1
--- /dev/null
+++ b/plugins/nettime.c
@@ -0,0 +1,293 @@
+/*
+ *
+ * oFono - Open Source Telephony
+ *
+ * Copyright (C) 2008-2010 Nokia Corporation and/or its
subsidiary(-ies). + *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301
USA + *
+ */
+
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#include <string.h>
+#include <glib.h>
+
+#define OFONO_API_SUBJECT_TO_CHANGE
+#include <ofono/plugin.h>
+#include <ofono/log.h>
+#include <ofono/nettime.h>
+#include <ofono/types.h>
+#include <gdbus.h>
+#include "ofono.h"
+
+#include "common.h"
+
+#define MAX_TIME_STR_LEN 128
+#define TIME_FORMAT "%FT%TZ"
+
+
+struct nettime_data {
+ struct ofono_network_time nw_time;
+ time_t received;
+};
+
+static void nettime_register(struct ofono_nettime_context *);
+
+static gboolean encode_time_format(struct ofono_network_time *time,
+ struct tm *tm)
+{
+ if (time->year < 0)
+ return FALSE;
+
+ tm->tm_year = time->year - 1900;
+ tm->tm_mon = time->mon - 1;
+ tm->tm_mday = time->mday;
+ tm->tm_hour = time->hour;
+ tm->tm_min = time->min;
+ tm->tm_sec = time->sec;
+ tm->tm_gmtoff = time->utcoff;
+ tm->tm_isdst = time->dst;
+
+ return TRUE;
+}
+
+static time_t get_monotonic_time()
+{
+ struct timespec ts;
+ clock_gettime(CLOCK_MONOTONIC, &ts);
+ return ts.tv_sec;
+}
+
+static struct tm *refresh_nw_time(struct tm *nw_time,
+ time_t received)
+{
+ time_t now, new_nw_time;
+
+ now = get_monotonic_time();
+ new_nw_time = timegm(nw_time) + now - received;
+ return gmtime(&new_nw_time);
+}
Brrr, the result is somewhere in static data. This can cause quite confusing
behaviour if/when the code is changed. I would much rather use gmtime_r().
That being said, wouldn't it be far simpler to return a second timestamp
instead of a broken-down time structure over D-Bus? Eventually, timed/whatever
will want a timestamp anyway to set the system clock or whatever else it wants
to do with the NITZ.
+static int fill_time_changed_signal(DBusMessage *signal,
+ struct nettime_data *ntd)
+{
+ DBusMessageIter iter, iter_array;
+ int time_available;
+ char buf[MAX_TIME_STR_LEN];
+ struct tm time_tm, *new_time_tm;
+ const char *str = buf;
+
+ dbus_message_iter_init_append(signal, &iter);
+ dbus_message_iter_open_container(&iter,
+ DBUS_TYPE_ARRAY,
+ "{sv}",
+ &iter_array);
+
+ time_available = encode_time_format(&ntd->nw_time, &time_tm);
+ if (time_available != 0) {
+ new_time_tm = refresh_nw_time(&time_tm, ntd->received);
+ strftime(buf, MAX_TIME_STR_LEN, TIME_FORMAT, new_time_tm);
+
+ ofono_dbus_dict_append(&iter_array,
+ "DateAndTime",
+ DBUS_TYPE_STRING,
+ &str);
+ }
+
+ ofono_dbus_dict_append(&iter_array,
+ "Timezone",
+ DBUS_TYPE_INT32,
+ &ntd->nw_time.utcoff);
+ ofono_dbus_dict_append(&iter_array,
+ "DST",
+ DBUS_TYPE_UINT32,
+ &ntd->nw_time.dst);
+ dbus_message_iter_close_container(&iter, &iter_array);
+ return 0;
+}
Are the time zone offset and the DST always available, but the current time
not? This seems odd.
Passing the time over D-Bus may cause extra drift/imprecision than we already
have. I wonder if it would make sense to provide not just the current time but
both:
- the current time for trivial programs, and
- the system boot time (i.e. nw_time - received) for smarter programs;
this value cannot drift.
+static DBusMessage *create_time_changed_signal(
+ struct ofono_nettime_context *context)
+{
+ DBusMessage *signal;
+ struct nettime_data *ntd = context->data;
+ const char *path = ofono_modem_get_path(context->modem);
+
+ if (path == NULL) {
+ ofono_error("Fetching path for modem failed");
+ return NULL;
+ }
+
+ signal = dbus_message_new_signal(path, OFONO_NETWORK_TIME_INTERFACE,
+ "NetworkTimeChanged");
+ fill_time_changed_signal(signal, ntd);
+
+ return signal;
+}
+
+static void init_time(struct ofono_nettime_context *context)
+{
+ struct nettime_data *nettime_data;
+ context->data = g_try_new0(struct nettime_data, 1);
Hmm, if you don't check the result, I suspect you should g_new0() instead.
+ nettime_data = context->data;
+ nettime_data->nw_time.year = -1;
+}
+
+static int nettime_probe(struct ofono_nettime_context *context)
+{
+ ofono_debug("Network Time Probe for modem: %p", context->modem);
+
+ init_time(context);
+
+ nettime_register(context);
+ return 0;
+}
+
+static void nettime_remove(struct ofono_nettime_context *context)
+{
+ DBusConnection *conn;
+ const char *path;
+
+ ofono_debug("Network Time Remove for modem: %p", context->modem);
+
+ g_free(context->data);
+
+ conn = ofono_dbus_get_connection();
+ path = ofono_modem_get_path(context->modem);
+
+ if (!g_dbus_unregister_interface(conn,
+ path,
+ OFONO_NETWORK_TIME_INTERFACE))
+ ofono_error("Could not unregister %s interface",
+ OFONO_NETWORK_TIME_INTERFACE);
+
+ ofono_modem_remove_interface(context->modem,
+ OFONO_NETWORK_TIME_INTERFACE);
+}
+
+static void nettime_info_received(struct ofono_nettime_context *context,
+ struct ofono_network_time *info)
+{
+ DBusMessage *nt_signal;
+ struct nettime_data *ntd = context->data;
+
+ if (info == NULL)
+ return;
+
+ ofono_debug("Received a network time notification on modem: %p",
+ context->modem);
+
+ ntd->nw_time = *info;
+ ntd->received = get_monotonic_time();
That works. But I would do the gmtime() conversion here, and store the result
once and for all, instead of copying the raw data and recompute the conversion
every time.
+ nt_signal = create_time_changed_signal(context);
+ if (nt_signal == NULL) {
+ ofono_error("Failed to create NetworkTimeChanged signal");
+ return;
+ }
+
+ g_dbus_send_message(ofono_dbus_get_connection(), nt_signal);
+}
--
Rémi Denis-Courmont
Nokia Devices R&D, Maemo Software, Helsinki