[rrd-developers] r2130 comments : rrd_parsetime, create_set_no_overwrite

[Steve Shipway]

Kevin Brintnall addressed the proletariat thusly:
>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.

I hadn’t realised that rrd_parsetime wasn’t threadsafe.  The easiest thing here is probably to change it to just take a time_t as the parameter; the rrd_client already passes a pre-parsed time_t anyway, so there’s no need to reparse it here.

>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().

I created rrd_create_set_no_overwrite in order to get around having to modify the rrd_create function; the opt_no_overwrite variable is a pain in the neck and shouldn’t really exist.  I agree that we should add an argument to rrd_create_r, however this would change the API (the rrd_create_r is a documented function in the library?).  Maybe rename rrd_create_r as rrd_create_r2, add the parameter, and create an rrd_create_r that defaults the opt_no_overwrite… This is really a separate issue to the changes, but was not really a problem until rrd_create_r became added to the daemon.  So, rrd_create_r was not actually thread-safe yet due to this opt_no_overwrite variable.  This also means adding the parameter to rrd_create_fn although I think this is not a big issue.

>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.

The validation is normally done in rrd_create and so this is bypassed by a call to rrd_create_r; since it was two small checks I didn’t think it worth the effort of moving the tests from rrd_create to rrd_create_r (and again, I was trying to avoid modifying the rrd_create_r function).  I think this can probably stay?

>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) {

This is just a legacy of how this code evolved.  I’m happy to change this or not (you want another close bracket before the == though I think)

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

Can you be more specific?  I’m not sure where you’re referring to.  The free() is still called in the argument processing; none of the other functions free the opt_daemon after handling either since it is potentially used later.

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.

Probably…  However I’m not familiar with the cache data structures and thought it best to avoid this, particularly since the previous behaviour did a flush anyway.  Also, by making the flush an optional parameter (in the client) it means you can retrieve both the last cache update time and also the last flush time.  So, the call from rrd_client will preserve the existing behaviour (return last data update) as it forces a flush.

If we can get the last time accurately via the cache data structures then this would be preferable (although the rrd_info would still need a flush as it gets a whole stack more data as well) – can you let me know the code required to obtain it?

Steve


________________________________
Steve Shipway
ITS Unix Services Design Lead
University of Auckland, New Zealand
Floor 1, 58 Symonds Street, Auckland
Phone: +64 (0)9 3737599 ext 86487
DDI: +64 (0)9 924 6487
Mobile: +64 (0)21 753 189
Email: s.shipway at auckland.ac.nz<mailto:s.shipway at auckland.ac.nz>
P Please consider the environment before printing this e-mail


-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.oetiker.ch/pipermail/rrd-developers/attachments/20100924/a8f74e03/attachment.htm