Hi Daniel,
> The static values are maintained in the
> Service object and exposed through simple
> accessors.
>
> When a Service object enters the ready state it
> registers itself at Counter.
>
> If the Service object is leaving the ready
> state it will de-register itself from Counter
> and consequently it will not be updated
> anymore.
>
> The user can shorten the update interval
> when he registers a Counter object with
> a shorter interval value.
>
> The statistic is stored in the profile file.
> Only the current value is stored, no history.
>
> If there is not Counter object the stats
> wont be upated. This short coming will
> be addressed by the 'data threshold
> netfilter module' patches.
> ---
> src/connman.h | 10 +++
> src/service.c | 174
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 184 insertions(+), 0 deletions(-)
>
> diff --git a/src/connman.h b/src/connman.h
> index 8bf5e97..c30dccd 100644
> --- a/src/connman.h
> +++ b/src/connman.h
> @@ -435,6 +435,16 @@ void __connman_service_append_nameserver(struct
> connman_service *service,
> void __connman_service_remove_nameserver(struct connman_service *service,
> const char *nameserver);
>
> +unsigned long __connman_service_stats_get_tx_bytes(
> + struct connman_service *service);
> +unsigned long __connman_service_stats_get_rx_bytes(
> + struct connman_service *service);
> +unsigned long __connman_service_stats_get_time(
> + struct connman_service *service);
So in these cases you can break the 78 chars rule. Just put them in one
line. We are just out of luck with keeping it in shape ;)
> struct connman_service {
> gint refcount;
> char *identifier;
> @@ -79,6 +90,7 @@ struct connman_service {
> DBusMessage *pending;
> guint timeout;
> struct connman_location *location;
> + struct connman_service_statistics stats;
> };
Not important right now, but just calling it connman_stats might be
simpler and shorter. In theory they are not bound to a service anyway
and we also don't care what you are counting either.
> + g_timer_stop(service->stats.timer);
> +
> + seconds = (uint32_t)g_timer_elapsed(service->stats.timer, NULL);
> + service->stats.time = service->stats.time_start + seconds;
Do we have to really cast here? And if so, please lets use (uint32_t) g_
with the extra space in between.
> +static int __connman_service_stats_load(struct connman_service *service,
> + GKeyFile *keyfile, const char *identifier, const char *prefix)
> +{
> + char *key;
> +
> + key = g_strdup_printf("%srx_bytes", prefix);
> + service->stats.rx_bytes = g_key_file_get_integer(keyfile,
> + identifier, key, NULL);
> + g_free(key);
> +
> + key = g_strdup_printf("%stx_bytes", prefix);
> + service->stats.tx_bytes = g_key_file_get_integer(keyfile,
> + identifier, key, NULL);
> + g_free(key);
> +
> + key = g_strdup_printf("%stime", prefix);
> + service->stats.time = g_key_file_get_integer(keyfile,
> + identifier, key, NULL);
> + g_free(key);
> +
> + return 0;
> +}
What is the prefix parameter good for. What else are we storing except
the stats counters?
> +static void __connman_service_reset_stats(struct connman_service *service)
> +{
> + service->stats.valid = FALSE;
> + service->stats.rx_bytes = 0;
> + service->stats.tx_bytes = 0;
> + service->stats.time = 0;
> + service->stats.time_start = 0;
> + g_timer_reset(service->stats.timer);
> +
> +}
> +unsigned long __connman_service_stats_get_tx_bytes(
> + struct connman_service *service)
> +{
> + return service->stats.tx_bytes;
> +}
> +unsigned long __connman_service_stats_get_rx_bytes(
> + struct connman_service *service)
> +{
> + return service->stats.rx_bytes;
> +}
> +
> +unsigned long __connman_service_stats_get_time(
> + struct connman_service *service)
> +{
> + return service->stats.time;
> +}
You are missing some empty lines here before and after each function,
but then also feel free to break the 78 chars rule.
> static GDBusMethodTable service_methods[] = {
> { "GetProperties", "", "a{sv}", get_properties },
> { "SetProperty", "sv", "", set_property },
> @@ -1546,6 +1670,7 @@ static GDBusMethodTable service_methods[] = {
> { "Remove", "", "", remove_service },
> { "MoveBefore", "o", "", move_before },
> { "MoveAfter", "o", "", move_after },
> + { "ResetStatistics", "", "", reset_stats },
I am really leaning towards calling this ResetCounters().
> +
> + seconds = (uint32_t)g_timer_elapsed(stats->timer, NULL);
> + stats->time = stats->time_start + seconds;
> +}
Cast? Extra space!
Regards
Marcel
_______________________________________________
connman mailing list
connman@connman.net
http://lists.connman.net/listinfo/connman
