Hi Daniel, On Thu, Aug 12, 2010 at 04:11:37PM +0200, Daniel Wagner wrote: > From: Daniel Wagner <daniel.wag...@bmw-carit.de> > > Currently connman only has one set of counters for > collecting statistics on online time and > the amount of transfered bytes. > > For 3G connections we should destinguish between > home network and roaming. This patch introduces > two sets of counter values for home network > and roaming network. > > Changing from Home to Roaming counters relies on > connman_network_set_roaming being called. Currently, > this is only done in the ofono plugin. Getting Wifi > 'Hot-Spot Provider' Roaming (e.g. T-Mobile Germany > to T-Mobile USA) is not yet working. Some comments on this patch:
> diff --git a/src/counter.c b/src/counter.c > index 27898c5..747c6bf 100644 > --- a/src/counter.c > +++ b/src/counter.c > @@ -38,6 +38,7 @@ struct connman_counter { > char *path; > unsigned int interval; > guint watch; > + connman_bool_t first_update; Makes sense to me, but can we then remove counter_data->first_update ? In fact, can we then completely remove counter_data ? > @@ -128,20 +130,11 @@ int __connman_counter_unregister(const char *owner, > const char *path) > } > > static void send_usage(struct connman_counter *counter, > - struct connman_service *service) > + struct counter_data *data) What's the reason for changing this prototype ? > @@ -159,42 +152,21 @@ static void send_usage(struct connman_counter *counter, > /* home counter */ > connman_dbus_dict_open(&array, &dict); > > - rx_packets = __connman_service_stats_get_rx_packets(service); > - tx_packets = __connman_service_stats_get_tx_packets(service); > - rx_bytes = __connman_service_stats_get_rx_bytes(service); > - tx_bytes = __connman_service_stats_get_tx_bytes(service); > - rx_errors = __connman_service_stats_get_rx_errors(service); > - tx_errors = __connman_service_stats_get_tx_errors(service); > - rx_dropped = __connman_service_stats_get_rx_dropped(service); > - tx_dropped = __connman_service_stats_get_tx_dropped(service); > - time = __connman_service_stats_get_time(service); > - > - connman_dbus_dict_append_basic(&dict, "RX.Packets", DBUS_TYPE_UINT32, > - &rx_packets); > - connman_dbus_dict_append_basic(&dict, "TX.Packets", DBUS_TYPE_UINT32, > - &tx_packets); > - connman_dbus_dict_append_basic(&dict, "RX.Bytes", DBUS_TYPE_UINT32, > - &rx_bytes); > - connman_dbus_dict_append_basic(&dict, "TX.Bytes", DBUS_TYPE_UINT32, > - &tx_bytes); > - connman_dbus_dict_append_basic(&dict, "RX.Errors", DBUS_TYPE_UINT32, > - &rx_errors); > - connman_dbus_dict_append_basic(&dict, "TX.Errors", DBUS_TYPE_UINT32, > - &tx_errors); > - connman_dbus_dict_append_basic(&dict, "RX.Dropped", DBUS_TYPE_UINT32, > - &rx_dropped); > - connman_dbus_dict_append_basic(&dict, "TX.Dropped", DBUS_TYPE_UINT32, > - &tx_dropped); > - connman_dbus_dict_append_basic(&dict, "Time", DBUS_TYPE_UINT32, > - &time); > + __connman_service_stats_home_append(data->service, &dict, > + counter->first_update); I think it would make more sense to just have one __connman_service_append_stats() routine that would fill the dictionary with the right stat pointer. > +static struct connman_stats *__connman_service_get_stats(struct > connman_service *service) > If this is a static routine, then please do not use the __connman prefix. It is intended for routines accesible to the rest of ConnMan core, not static routines. > +static void append_all_stats(DBusMessageIter *dict, struct connman_stats > *stats) > +{ > + stats->rx_packets_update = stats->rx_packets; > + connman_dbus_dict_append_basic(dict, "RX.Packets", DBUS_TYPE_UINT32, > + &stats->rx_packets); > + > + stats->tx_packets_update = stats->tx_packets; > + connman_dbus_dict_append_basic(dict, "TX.Packets", DBUS_TYPE_UINT32, > + &stats->tx_packets); > + > + stats->rx_bytes_update = stats->rx_bytes; > + connman_dbus_dict_append_basic(dict, "RX.Bytes", DBUS_TYPE_UINT32, > + &stats->rx_bytes); > + > + stats->tx_bytes_update = stats->tx_bytes; > + connman_dbus_dict_append_basic(dict, "TX.Bytes", DBUS_TYPE_UINT32, > + &stats->tx_bytes); > + > + stats->rx_errors_update = stats->rx_errors; > + connman_dbus_dict_append_basic(dict, "RX.Errors", DBUS_TYPE_UINT32, > + &stats->rx_errors); > + > + stats->tx_errors_update = stats->tx_errors; > + connman_dbus_dict_append_basic(dict, "TX.Errors", DBUS_TYPE_UINT32, > + &stats->tx_errors); > + > + stats->rx_dropped_update = stats->rx_dropped; > + connman_dbus_dict_append_basic(dict, "RX.Dropped", DBUS_TYPE_UINT32, > + &stats->rx_dropped); > + > + stats->tx_dropped_update = stats->tx_dropped; > + connman_dbus_dict_append_basic(dict, "TX.Dropped", DBUS_TYPE_UINT32, > + &stats->tx_dropped); > + > + stats->time_update = stats->time; > + connman_dbus_dict_append_basic(dict, "Time", DBUS_TYPE_UINT32, > + &stats->time); > +} > + > +static void append_stats(DBusMessageIter *dict, struct connman_stats *stats) > +{ So here I'd like to have only one append_stats() routine that would take an additional boolean argument and that would do e.g.: if (stats->rx_packets_update != stats->rx_packets || all) { Now, I have a concern about notifying counters about the changed stats only. With the new counter registering API, I should be able to register 2 counters e.g. counter A with a 500KB threshold and counter B with a 2MB threshold. Every 500KB, counter A will be notified and the service stats->*_update will be updated if the field changed. Thus when counter B will get notifed later on, it won't have tracked those changes and will still believe that for example stats->rx_error has not changed while it had and only counter A knows about it. I know at the moment all registered counters get notified at the same time, but I hope that will change. So, my question is: Is it really worth saving a few D-Bus bytes at the cost of adding significant complecxity if we ever want to have a proper counter framework (i.e. one that wouldn't notify _all_ counters at the same time) ? Thanks a lot for your patch. Cheers, Samuel. -- Intel Open Source Technology Centre http://oss.intel.com/ _______________________________________________ connman mailing list connman@connman.net http://lists.connman.net/listinfo/connman