Hi Marcel,
On Thu, Sep 23, 2010 at 09:41:46PM +0900, Marcel Holtmann wrote:
> > --- a/src/stats.c
> > +++ b/src/stats.c
> > @@ -250,11 +250,12 @@ static int stats_open_file(struct connman_service
> > *service,
> > struct stats_file *file,
> > connman_bool_t roaming)
> > {
> > - struct stat stat;
> > + struct stat buf;
>
> please don't. The common declaration here is this:
>
> struct stat st;
>
> And buf is the worst variable name you could have picked ;)
I completely agree. I was pondering what cool name I could use and
found in the man pages:
int stat(const char *path, struct stat *buf);
>
> > char *name;
> > int err;
> > size_t size;
> > struct stats_file_header *hdr;
> > + connman_bool_t new_file = FALSE;
> >
> > if (roaming == FALSE) {
> > name = g_strdup_printf("%s/%s.data", STORAGEDIR,
> > @@ -265,6 +266,13 @@ static int stats_open_file(struct connman_service
> > *service,
> > }
> >
> > file->name = name;
> > +
> > + err = stat(file->name, &buf);
> > + if (err < 0) {
> > + /* according documentation the only possible error is ENOENT */
> > + new_file = TRUE;
> > + }
> > +
> > file->fd = open(name, O_RDWR | O_CREAT, 0644);
> >
> > if (file->fd == -1) {
>
> I prefer these checks like this btw.:
>
> if (fd < 0)
> error...
yep, I'm changing this
> > @@ -275,14 +283,11 @@ static int stats_open_file(struct connman_service
> > *service,
> >
> > file->max_len = STATS_MAX_FILE_SIZE;
> >
> > - err = fstat(file->fd, &stat);
> > - if (err < 0)
> > - return -errno;
> > -
> > - if (stat.st_size < sysconf(_SC_PAGESIZE))
> > + if (buf.st_size < sysconf(_SC_PAGESIZE))
> > size = sysconf(_SC_PAGESIZE);
> > else
> > - size = (size_t)stat.st_size;
> > + size = (size_t)buf.st_size;
> > +
>
> What is this cast for. I don't really like casts like this.
left overs...
>
> > err = stats_file_remap(file, size);
> > if (err < 0)
> > @@ -290,12 +295,13 @@ static int stats_open_file(struct connman_service
> > *service,
> >
> > hdr = get_hdr(file);
> >
> > - if (hdr->magic != MAGIC ||
> > + if (new_file == TRUE || hdr->magic != MAGIC ||
> > hdr->begin < sizeof(struct stats_file_header) ||
> > hdr->end < sizeof(struct stats_file_header) ||
> > hdr->begin > file->len ||
> > hdr->end > file->len) {
> > - connman_warn("invalid file header for %s", file->name);
> > + if (new_file == FALSE)
> > + connman_warn("invalid file header for %s", file->name);
>
> First check for new_file == TRUE and now for new_file == FALSE. That
> logic seems a bit too complicated for me. Makes my brain hurt ;)
my brain hurts too. maybe the reason for this crap. sorry about this.
an update is on the way.
thanks for the review,
daniel
_______________________________________________
connman mailing list
connman@connman.net
http://lists.connman.net/listinfo/connman
