<div>Comrades,</div><div><br></div><div>I noticed that r2130 adds a call to <font class="Apple-style-span" face="'courier new', monospace">rrd_parsetime</font> to <font class="Apple-style-span" face="'courier new', monospace">rrd_daemon.c</font>. I think this is problematic since <font class="Apple-style-span" face="'courier new', monospace">rrd_parsetime</font> is not thread-safe. It would probably make more sense to do the calculations in the client and pass the absolute <font class="Apple-style-span" face="'courier new', monospace">time_t</font> 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.<div>
<br></div><div>A thread-safe re-write of <font class="Apple-style-span" face="'courier new', monospace">rrd_parsetime()</font> would also be beneficial, but that looks awfully hairy. I suppose we could mutex-lock the existing version until then....</div>
<div><br></div><div>Also, <font class="Apple-style-span" face="'courier new', monospace">rrd_create_set_no_overwrite()</font> 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:</div>
<div><br></div><div> - add an argument to <font class="Apple-style-span" face="'courier new', monospace">rrd_create_r()</font>, passing it through to <font class="Apple-style-span" face="'courier new', monospace">rrd_create_fn()</font></div>
<div> - create a new API function that takes a boolean for whether create should clobber an existing file.... then, re-implement <font class="Apple-style-span" face="'courier new', monospace">rrd_create_r()</font> in terms of that function (with today's default)... stil passing the arg through to <font class="Apple-style-span" face="'courier new', monospace">rrd_create_fn()</font>.</div>
<div><br></div><div>Also, in <font class="Apple-style-span" face="'courier new', monospace">handle_request_create()</font>, maybe it would make sense to do the validation (i.e. step and last_up) in <font class="Apple-style-span" face="'courier new', monospace">rrd_create_r()</font> and rely on the return value from <font class="Apple-style-span" face="'courier new', monospace">rrd_create_r()</font> to detect whether there was a failure? That way, the validation code stays in the same place.</div>
<div><br></div><div>Also, on <font class="Apple-style-span" face="'courier new', monospace">rrd_daemon.c</font> lines 1830..1831, it looks like it could be re-written more nicely as:</div><div><br></div><div><span class="Apple-style-span" style="font-size: 11px; "><font class="Apple-style-span" face="'courier new', monospace">- status = buffer_get_field(&buffer, &buffer_size, &tok );</font></span></div>
<div><span class="Apple-style-span" style="font-size: 11px; "><font class="Apple-style-span" face="'courier new', monospace">- for(;(tok && !status);status = buffer_get_field(&buffer, &buffer_size, &tok )) {</font></span></div>
<div><span class="Apple-style-span" style="font-size: 11px; "><font class="Apple-style-span" face="'courier new', monospace">+ while ((status = </font></span><span class="Apple-style-span" style="font-size: 11px; "><font class="Apple-style-span" face="'courier new', monospace">buffer_get_field(&buffer, &buffer_size, &tok) == 0 && tok) {</font></span></div>
<div><span class="Apple-style-span" style="font-family: monospace; font-size: 11px; "><br></span></div>In rrd_last.c, we seem to have lost a call to <font class="Apple-style-span" face="'courier new', monospace">free(opt_daemon)</font>. Is that intentional?</div>
<div><br></div><div>In <font class="Apple-style-span" face="'courier new', monospace">handle_request_last()</font><span class="Apple-style-span" style="font-family: monospace; font-size: 11px; ">, </span>it seems we could get the last update time without a flush by using <font class="Apple-style-span" face="'courier new', monospace">cache_item_t.last_update_stamp</font> and adjusting down to the nearest RRD step size? Then, no flush is required and the existing behavior is preserved.</div>
<meta charset="utf-8"><div><br><div>Thoughts?</div><div><br></div><div>-- <br> kevin brintnall =~ /<a href="http://kbrint@rufus.net/">kbrint@rufus.net/</a><br><br>
</div></div>