<div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;"><div lang="EN-NZ" link="blue" vlink="purple"><div><div class="im"><p class="MsoNormal"><span style="color:#548DD4">>Also, in </span><span style="font-family:"Courier New";color:#548DD4">handle_request_create()</span><span style="color:#548DD4">, maybe it would make sense to do the validation (i.e.
step and last_up) in </span><span style="font-family:"Courier New";color:#548DD4">rrd_create_r()</span><span style="color:#548DD4"> and rely on the return value from </span><span style="font-family:"Courier New";color:#548DD4">rrd_create_r()</span><span style="color:#548DD4"> to detect whether there was a failure? That way,
the validation code stays in the same place.</span></p></div></div></div></blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;"><div lang="EN-NZ" link="blue" vlink="purple">
<div><div class="im"><p class="MsoNormal"><br></p>
</div><p class="MsoNormal">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?</p><div class="im">
<p class="MsoNormal"></p></div></div></div></blockquote><div><br></div><div><div>I think this argues for moving the validation code to rrd_create_r or rrd_create_fn, (depending on how the no-overwrite flag changes the API). Then, it can exist in a single place. I don't see any external effects there.</div>
</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;"><div lang="EN-NZ" link="blue" vlink="purple"><div><div class="im"><p class="MsoNormal"> </p>
<p class="MsoNormal"><span style="color:#548DD4">>Also, on </span><span style="font-family:"Courier New";color:#548DD4">rrd_daemon.c</span><span style="color:#548DD4"> lines 1830..1831, it looks like it could be re-written
more nicely as:</span></p>
<p class="MsoNormal"><span><span style="font-size:8.5pt;font-family:"Courier New";color:#548DD4">- status =
buffer_get_field(&buffer, &buffer_size, &tok );</span></span><span style="color:#548DD4"></span></p>
<p class="MsoNormal"><span><span style="font-size:8.5pt;font-family:"Courier New";color:#548DD4">- for(;(tok && !status);status
= buffer_get_field(&buffer, &buffer_size, &tok )) {</span></span><span style="color:#548DD4"></span></p>
<p class="MsoNormal"><span><span style="font-size:8.5pt;font-family:"Courier New";color:#548DD4">+ while ((status
= buffer_get_field(&buffer, &buffer_size, &tok) == 0
&& tok) {</span></span><span style="color:#548DD4"></span></p>
<p class="MsoNormal"> </p>
</div><p class="MsoNormal">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)</p></div></div></blockquote><div><br></div><div>Yep, need the ). I typed the new line free-hand :)</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<div lang="EN-NZ" link="blue" vlink="purple"><div><div class="im">
<p class="MsoNormal"><span style="color:#548DD4">In rrd_last.c, we seem to have
lost a call to </span><span style="font-family:"Courier New";color:#548DD4">free(opt_daemon)</span><span style="color:#548DD4">. Is that intentional?</span></p>
<p class="MsoNormal"> </p>
</div><p class="MsoNormal">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.</p></div></div></blockquote><div><br></div><div>This line was removed from rrd_info.c and rrd_last.c:</div><div><br></div><div><span class="Apple-style-span" style="font-family: Verdana, Arial, 'Bitstream Vera Sans', Helvetica, sans-serif; font-size: 13px; border-collapse: collapse; -webkit-border-horizontal-spacing: 2px; -webkit-border-vertical-spacing: 2px; "><table class="inline" summary="Differences" cellspacing="0" style="border-right-width: 1px; border-bottom-width: 1px; border-left-width: 1px; border-right-style: solid; border-bottom-style: solid; border-left-style: solid; border-right-color: rgb(221, 221, 221); border-bottom-color: rgb(221, 221, 221); border-left-color: rgb(221, 221, 221); -webkit-border-horizontal-spacing: 0px; -webkit-border-vertical-spacing: 0px; border-top-width: 0px; border-top-style: initial; border-top-color: initial; empty-cells: show; font-size: 12px; line-height: 15px; padding-top: 0px; padding-right: 0px; padding-bottom: 0px; padding-left: 0px; margin-top: 0px; margin-right: auto; margin-bottom: 0px; margin-left: auto; table-layout: fixed; width: 669px; ">
<tbody class="rem"><tr class="last first"><td class="l" style="font: normal normal normal 11px/normal monospace; background-image: initial; background-attachment: initial; background-origin: initial; background-clip: initial; background-color: rgb(255, 221, 221); overflow-x: visible; overflow-y: visible; padding-top: 1px; padding-right: 2px; padding-bottom: 1px; padding-left: 2px; vertical-align: top; border-top-color: rgb(204, 0, 0); border-right-color: rgb(204, 0, 0); border-bottom-color: rgb(204, 0, 0); border-left-color: rgb(204, 0, 0); border-top-style: solid; border-right-style: solid; border-bottom-style: solid; border-left-style: solid; border-top-width: 1px; border-right-width: 1px; border-bottom-width: 1px; border-left-width: 1px; background-position: initial initial; background-repeat: initial initial; ">
<del style="text-decoration: none; "> if (opt_daemon) free (opt_daemon);</del> </td></tr></tbody><tbody class="unmod"><tr></tr></tbody></table><br></span></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<div lang="EN-NZ" link="blue" vlink="purple"><div><div class="im">
<p class="MsoNormal"> </p>
<p class="MsoNormal"><span style="color:#548DD4">In </span><span style="font-family:"Courier New";color:#548DD4">handle_request_last()</span><span><span style="font-size:8.5pt;font-family:"Courier New";color:#548DD4">, </span></span><span style="color:#548DD4">it seems we could
get the last update time without a flush by using </span><span style="font-family:"Courier New";color:#548DD4">cache_item_t.last_update_stamp</span><span style="color:#548DD4"> and adjusting down to the nearest RRD step size?
Then, no flush is required and the existing behavior is preserved.</span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;color:#1F497D"> </span></p>
</div><p class="MsoNormal">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. </p></div></div></blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;"><div lang="EN-NZ" link="blue" vlink="purple"><div><p class="MsoNormal"> </p>
<p class="MsoNormal">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?</p>
<p class="MsoNormal"></p></div></div></blockquote><div><br></div><div>The cache structure maintains this value so that it can deny updates from the past (just like the normal RRD code does). So, to calculate the most recent time stamp, it should be sufficient to do something like this (paraphrased):</div>
<div><br></div><div><font class="Apple-style-span" face="'courier new', monospace">time_t t;</font></div><font class="Apple-style-span" face="'courier new', monospace"><div class="gmail_quote">rrd_open(file); // bail if it doesn't exist</div>
<div class="gmail_quote">time_t from_file = rrd->live_head->last_up;</div><div class="gmail_quote">time_t step = rrd->stat_head->pdp_step;</div><div class="gmail_quote">rrd_close(file);</div><div class="gmail_quote">
pthread_mutex_lock(&cache_lock);</div><div class="gmail_quote">ci = g_tree_lookup(cache_tree, file);</div><div class="gmail_quote">if (ci)</div><div class="gmail_quote"> t = ci->last_update_stamp;</div><div class="gmail_quote">
else</div><div class="gmail_quote"> t = from_file;</div><div class="gmail_quote">pthread_mutex_unlock(&cache_lock);</div>t -= t % step;</font></div><div class="gmail_quote"><font class="Apple-style-span" face="'courier new', monospace">free things;<br>
return t;</font></div><div class="gmail_quote"><font class="Apple-style-span" face="'courier new', monospace"><br></font></div>The reason for this order is that you have to hold the <font class="Apple-style-span" face="'courier new', monospace">cache_lock</font> while processing the tree. We don't want to block out all other clients waiting for the disk read to come back. Note also, that we return the raw info in case the file exists on disk but not in the RRD tree (i.e. hasn't been written recently).<div>
<br></div><div>In the future if we keep step information in the cache structure (i.e. for UPDATEV), we won't have to hit the file for the step.. <br><div><br></div><div>Not sure how to handle the case of a version < 3 RRD file that uses <font class="Apple-style-span" face="'courier new', monospace">legacy_last_up</font>. Apparently <font class="Apple-style-span" face="'courier new', monospace">rrd_last_r</font> doesn't handle that today either.</div>
<div><br><div>-kb<div><br><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;"><div lang="EN-NZ" link="blue" vlink="purple"><div><p class="MsoNormal">
</p>
<p class="MsoNormal">Steve</p>
<p class="MsoNormal"><span style="font-size:11.0pt;color:#1F497D"> </span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;color:#1F497D"> </span></p>
<div class="MsoNormal" align="center" style="text-align:center"><span lang="EN-US" style="font-size:11.0pt;color:#1F497D">
<hr size="2" width="100%" align="center">
</span></div>
<p class="MsoNormal"><b><span style="font-size:11.0pt;color:#1F497D">Steve Shipway</span></b></p>
<p class="MsoNormal"><span style="font-size:10.0pt;color:#1F497D">ITS Unix Services Design Lead</span></p>
<p class="MsoNormal"><span style="font-size:10.0pt;color:#1F497D">University of Auckland, New Zealand</span></p>
<p class="MsoNormal"><span style="font-size:10.0pt;color:#1F497D">Floor 1, 58 Symonds Street, Auckland</span></p>
<p class="MsoNormal"><i><span style="font-size:10.0pt;color:#595959">Phone: +64 (0)9 3737599 ext 86487</span></i></p>
<p class="MsoNormal"><i><span style="font-size:10.0pt;color:#595959">DDI: +64 (0)9 924 6487</span></i></p>
<p class="MsoNormal"><i><span style="font-size:10.0pt;color:#595959">Mobile: +64 (0)21 753 189</span></i></p>
<p class="MsoNormal"><i><span style="font-size:10.0pt;color:#595959">Email: <a href="mailto:s.shipway@auckland.ac.nz" target="_blank"><span style="color:#595959">s.shipway@auckland.ac.nz</span></a></span></i></p>
<p class="MsoNormal"><span lang="EN-GB" style="font-size:18.0pt;font-family:Webdings;color:green">P</span><span lang="EN-GB" style="font-size:11.0pt;color:blue"> </span><span lang="EN-GB" style="font-size:10.0pt;color:green">Please consider the environment before printing this e-mail</span><span lang="EN-GB" style="font-size:11.0pt;color:blue"> </span><span lang="EN-GB" style="font-size:7.5pt;color:navy"></span></p>
<p class="MsoNormal"><i><span style="font-size:10.0pt;color:#1F497D"> </span></i></p>
<div style="border:none;border-left:solid blue 1.5pt;padding:0cm 0cm 0cm 4.0pt">
<div>
<div>
<p class="MsoNormal" style="margin-bottom:12.0pt"> </p>
</div>
</div>
</div>
</div>
</div>
</blockquote></div><br><br clear="all"><br>-- <br> kevin brintnall =~ /<a href="http://kbrint@rufus.net/">kbrint@rufus.net/</a><br><br>
</div></div></div></div>