Hi Daniel, As I said, I think this is the right approach, and I only have a few minor comments so far:
On Fri, Sep 03, 2010 at 06:02:56PM +0200, Daniel Wagner wrote: > From: Daniel Wagner <[email protected]> > > Currently the statistic information is > stored into the profile file. This > results in rewriting the whole file > if connann_stats_save is called. This > results in too many disk I/Os for low > power devices. > > Furthermore, only the current value is > stored. There is no way to find out > how many bytes have been transfered > in the last month. > > First, each service statistic is > stored into a separate files under > /var/lib/connman having a *.data > extension. This file contains fixed sized > records of stats counters and will be mmap > into memory. It is used like a ring buffer. > > If the buffer is full or after a certain > period (e.g. a month), the raw data > will be summerized into the *.info file. This is not implemented yet, right ? > --- > > Still messy but it gets better. I've learned a lot about mmap and > ftruncate. Funny stuff. > > collect.c is just a debug program but I think I'm going to clean it > up and add it later to the test dir. Seems to be useful. Indeed. > > have a nice weekdend > > > > Makefile.am | 2 +- > bootstrap-configure | 6 +- > configure.ac | 11 ++ > src/collect.c | 202 ++++++++++++++++++++++++++ > src/connman.h | 24 +++ > src/main.c | 2 + > src/service.c | 131 +++--------------- > src/stats.c | 389 > +++++++++++++++++++++++++++++++++++++++++++++++++++ > 8 files changed, 649 insertions(+), 118 deletions(-) > create mode 100644 src/collect.c > create mode 100644 src/stats.c > > diff --git a/Makefile.am b/Makefile.am > index 1bf7207..6a6fe6d 100644 > --- a/Makefile.am > +++ b/Makefile.am > @@ -63,7 +63,7 @@ src_connmand_SOURCES = $(gdbus_sources) $(gdhcp_sources) > $(gresolv_sources) \ > src/wifi.c src/storage.c src/dbus.c src/config.c \ > src/technology.c src/counter.c src/location.c \ > src/session.c src/tethering.c src/ondemand.c \ > - src/wpad.c > + src/wpad.c src/stats.c > > if UDEV > src_connmand_SOURCES += src/udev.c > diff --git a/bootstrap-configure b/bootstrap-configure > index 1045688..9b5237e 100755 > --- a/bootstrap-configure > +++ b/bootstrap-configure I guess you can get rid of this chunk ;) > diff --git a/src/connman.h b/src/connman.h > index 7c723ba..af0e73e 100644 > --- a/src/connman.h > +++ b/src/connman.h > @@ -455,6 +455,7 @@ void __connman_service_create_ipconfig(struct > connman_service *service, > int index); > struct connman_ipconfig *__connman_service_get_ipconfig( > struct connman_service *service); > +const char *__connman_service_get_ident(struct connman_service *service); > const char *__connman_service_get_path(struct connman_service *service); > unsigned int __connman_service_get_order(struct connman_service *service); > struct connman_network *__connman_service_get_network(struct connman_service > *service); > @@ -561,3 +562,26 @@ void __connman_session_cleanup(void); > > int __connman_ondemand_init(void); > void __connman_ondemand_cleanup(void); > + > +struct connman_stats_data { > + unsigned int rx_packets; > + unsigned int tx_packets; > + unsigned int rx_bytes; > + unsigned int tx_bytes; > + unsigned int rx_errors; > + unsigned int tx_errors; > + unsigned int rx_dropped; > + unsigned int tx_dropped; > + unsigned int time; > +}; I don't particularily enjoy exporting the whole structure through connman.h. > +int __connman_stats_init(void); > +void __connman_stats_cleanup(void); > +int __connman_stats_service_register(struct connman_service *service); > +void __connman_stats_service_unregister(struct connman_service *service); > +int __connman_stats_update(struct connman_service *service, > + connman_bool_t roaming, > + struct connman_stats_data *data); > +int __connman_stats_get(struct connman_service *service, > + connman_bool_t roaming, > + struct connman_stats_data *data); > diff --git a/src/stats.c b/src/stats.c > new file mode 100644 > index 0000000..a9c349e > --- /dev/null > +++ b/src/stats.c > @@ -0,0 +1,389 @@ > +/* > + * > + * Connection Manager > + * > + * Copyright (C) 2010 BMW Car IT GmbH. All rights reserved. > + * > + * 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 > + > +#define _GNU_SOURCE > +#include <sys/mman.h> > +#include <sys/types.h> > +#include <sys/stat.h> > +#include <sys/time.h> > +#include <fcntl.h> > +#include <unistd.h> > +#include <string.h> > + > +#include "connman.h" I know we're really bad at commenting the ConnMan code currently, but I'd appreciate a small comment here explaining that you're trying to have a fixed size stat file per service, with some header and a stats_record based ring buffer. > +#define MAGIC 0xFA00B915 > + > +struct stats_file_header { > + unsigned int magic; > + unsigned int first; > + unsigned int last; > +}; > + > +struct stats_record { > + struct timeval tv; > + struct connman_stats_data data; > +}; > + > +struct stats_file { > + int fd; > + char *addr; > + size_t len; > + size_t max_len; > +}; > + > +struct stats_handle { > + struct stats_file home; > + struct stats_file roaming; > +}; > + > +GHashTable *stats_hash = NULL; > + > +static int stats_file_remap(struct stats_file *file, size_t size) > +{ > + size_t ps, ns; > + void *np; Please use more verbose variable names here. Cheers, Samuel. -- Intel Open Source Technology Centre http://oss.intel.com/ _______________________________________________ connman mailing list [email protected] http://lists.connman.net/listinfo/connman
