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
_______________________________________________
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
