[rrd-developers] r2130 comments : rrd_parsetime, create_set_no_overwrite

[kevin brintnall]

Comrades,

I noticed that r2130 adds a call to rrd_parsetime to rrd_daemon.c.  I think
this is problematic since rrd_parsetime is not thread-safe. It would
probably make more sense to do the calculations in the client and pass the
absolute time_t to the daemon.  That's what we've done with UPDATE.  We
already rely on time synchronization between client+server so I don't see
that as a real issue.

A thread-safe re-write of rrd_parsetime() would also be beneficial, but that
looks awfully hairy.  I suppose we could mutex-lock the existing version
until then....

Also, rrd_create_set_no_overwrite() is not thread-safe..  This is a big
problem for the daemon.  It writes a global (un-guarded) variable.  There
are unavoidable race conditions here.  I think a better approach would be
either:

 - add an argument to rrd_create_r(), passing it through to rrd_create_fn()
 -  create a new API function that takes a boolean for whether create should
clobber an existing file....  then, re-implement rrd_create_r() in terms of
that function (with today's default)...  stil passing the arg through to
rrd_create_fn().

Also, in handle_request_create(), maybe it would make sense to do the
validation (i.e. step and last_up) in rrd_create_r() and rely on the return
value from rrd_create_r() to detect whether there was a failure?  That way,
the validation code stays in the same place.

Also, on rrd_daemon.c lines 1830..1831, it looks like it could be re-written
more nicely as:

- status = buffer_get_field(&buffer, &buffer_size, &tok );
- for(;(tok && !status);status = buffer_get_field(&buffer, &buffer_size,
&tok )) {
+ while ((status = buffer_get_field(&buffer, &buffer_size, &tok) == 0 &&
tok) {

In rrd_last.c, we seem to have lost a call to free(opt_daemon).  Is that
intentional?

In handle_request_last(), it seems we could get the last update time without
a flush by using cache_item_t.last_update_stamp and adjusting down to the
nearest RRD step size?  Then, no flush is required and the existing behavior
is preserved.

Thoughts?

-- 
 kevin brintnall =~ /kbrint at rufus.net/
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.oetiker.ch/pipermail/rrd-developers/attachments/20100923/b7a86953/attachment-0001.htm