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
