<html dir="ltr"><head>
<meta http-equiv="Content-Type" content="text/html; charset=Windows-1252"><style>@font-face {
font-family: Calibri;
}
@font-face {
font-family: Verdana;
}
@font-face {
font-family: Webdings;
}
@font-face {
font-family: Arial Narrow;
}
@page WordSection1 {margin: 72.0pt 72.0pt 72.0pt 72.0pt; }
P.MsoNormal {
MARGIN: 0cm 0cm 0pt; FONT-FAMILY: "Times New
Roman","serif"; FONT-SIZE: 12pt
}
LI.MsoNormal {
MARGIN: 0cm 0cm 0pt; FONT-FAMILY: "Times New
Roman","serif"; FONT-SIZE: 12pt
}
DIV.MsoNormal {
MARGIN: 0cm 0cm 0pt; FONT-FAMILY: "Times New
Roman","serif"; FONT-SIZE: 12pt
}
A:link {
COLOR: blue; TEXT-DECORATION: underline
}
SPAN.MsoHyperlink {
COLOR: blue; TEXT-DECORATION: underline
}
A:visited {
COLOR: purple; TEXT-DECORATION: underline
}
SPAN.MsoHyperlinkFollowed {
COLOR: purple; TEXT-DECORATION: underline
}
SPAN.apple-style-span {
}
SPAN.EmailStyle18 {
FONT-FAMILY: "Calibri","sans-serif"; COLOR: #1f497d
}
.MsoChpDefault {
}
DIV.WordSection1 {
}
</style>
<style id="owaParaStyle">P {
MARGIN-TOP: 0px; MARGIN-BOTTOM: 0px
}
</style>
</head>
<body lang="EN-NZ" link="blue" vlink="purple" ocsi="0" fPStyle="1">
<div style="FONT-FAMILY: Tahoma; DIRECTION: ltr; COLOR: #000000; FONT-SIZE: 13px">
<div><span style="COLOR: #548dd4">Kevin Brintnall addressed the proletariat thusly:</span></div>
<div>
<div class="WordSection1">
<p class="MsoNormal"><span style="COLOR: #548dd4">>I noticed that r2130 adds a call to </span><span style="FONT-FAMILY: 'Courier New'; COLOR: #548dd4">rrd_parsetime</span><span style="COLOR: #548dd4"> to </span><span style="FONT-FAMILY: 'Courier New'; COLOR: #548dd4">rrd_daemon.c</span><span style="COLOR: #548dd4">. I think this is problematic since </span><span style="FONT-FAMILY: 'Courier New'; COLOR: #548dd4">rrd_parsetime</span><span style="COLOR: #548dd4"> is not thread-safe. It would probably make more sense to do the calculations in the client and pass the absolute </span><span style="FONT-FAMILY: 'Courier New'; COLOR: #548dd4">time_t</span><span style="COLOR: #548dd4"> 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.</span></p>
<p class="MsoNormal"> </p>
<p class="MsoNormal">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.</p>
<p class="MsoNormal"> </p>
<p class="MsoNormal"><span style="COLOR: #548dd4">>Also, </span><span style="FONT-FAMILY: 'Courier New'; COLOR: #548dd4">rrd_create_set_no_overwrite()</span><span style="COLOR: #548dd4"> 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:</span></p>
<p class="MsoNormal"><span style="COLOR: #548dd4"> - add an argument to </span><span style="FONT-FAMILY: 'Courier New'; COLOR: #548dd4">rrd_create_r()</span><span style="COLOR: #548dd4">, passing it through to </span><span style="FONT-FAMILY: 'Courier New'; COLOR: #548dd4">rrd_create_fn()</span><span style="COLOR: #548dd4"></span></p>
<p class="MsoNormal"><span style="COLOR: #548dd4"> - create a new API function that takes a boolean for whether create should clobber an existing file.... then, re-implement </span><span style="FONT-FAMILY: 'Courier New'; COLOR: #548dd4">rrd_create_r()</span><span style="COLOR: #548dd4"> in terms of that function (with today's default)... stil passing the arg through to </span><span style="FONT-FAMILY: 'Courier New'; COLOR: #548dd4">rrd_create_fn()</span><span style="COLOR: #548dd4">.</span></p>
<p class="MsoNormal"> </p>
<p class="MsoNormal">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.</p>
<p class="MsoNormal"> </p>
<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>
<p class="MsoNormal"> </p>
<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>
<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 class="apple-style-span"><span style="FONT-FAMILY: 'Courier New'; COLOR: #548dd4; FONT-SIZE: 8.5pt">- status = buffer_get_field(&buffer, &buffer_size, &tok );</span></span><span style="COLOR: #548dd4"></span></p>
<p class="MsoNormal"><span class="apple-style-span"><span style="FONT-FAMILY: 'Courier New'; COLOR: #548dd4; FONT-SIZE: 8.5pt">- for(;(tok && !status);status = buffer_get_field(&buffer, &buffer_size, &tok )) {</span></span><span style="COLOR: #548dd4"></span></p>
<p class="MsoNormal"><span class="apple-style-span"><span style="FONT-FAMILY: 'Courier New'; COLOR: #548dd4; FONT-SIZE: 8.5pt">+ while ((status = buffer_get_field(&buffer, &buffer_size, &tok) == 0 && tok) {</span></span><span style="COLOR: #548dd4"></span></p>
<p class="MsoNormal"> </p>
<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>
<p class="MsoNormal"> </p>
<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>
<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>
<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 class="apple-style-span"><span style="FONT-FAMILY: 'Courier New'; COLOR: #548dd4; FONT-SIZE: 8.5pt">, </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-FAMILY: 'Calibri','sans-serif'; COLOR: #1f497d; FONT-SIZE: 11pt"></span> </p>
<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>
<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>
<p class="MsoNormal">Steve</p>
<p class="MsoNormal"><span style="FONT-FAMILY: 'Calibri','sans-serif'; COLOR: #1f497d; FONT-SIZE: 11pt"></span> </p>
<p class="MsoNormal"><span style="FONT-FAMILY: 'Calibri','sans-serif'; COLOR: #1f497d; FONT-SIZE: 11pt"></span> </p>
<div style="TEXT-ALIGN: center" class="MsoNormal" align="center"><span style="FONT-FAMILY: 'Calibri','sans-serif'; COLOR: #1f497d; FONT-SIZE: 11pt" lang="EN-US">
<hr align="center" size="2" width="100%">
</span></div>
<p class="MsoNormal"><b><span style="FONT-FAMILY: 'Calibri','sans-serif'; COLOR: #1f497d; FONT-SIZE: 11pt">Steve Shipway</span></b></p>
<p class="MsoNormal"><span style="FONT-FAMILY: 'Calibri','sans-serif'; COLOR: #1f497d; FONT-SIZE: 10pt">ITS Unix Services Design Lead</span></p>
<p class="MsoNormal"><span style="FONT-FAMILY: 'Calibri','sans-serif'; COLOR: #1f497d; FONT-SIZE: 10pt">University of Auckland, New Zealand</span></p>
<p class="MsoNormal"><span style="FONT-FAMILY: 'Calibri','sans-serif'; COLOR: #1f497d; FONT-SIZE: 10pt">Floor 1, 58 Symonds Street, Auckland</span></p>
<p class="MsoNormal"><i><span style="FONT-FAMILY: 'Calibri','sans-serif'; COLOR: #595959; FONT-SIZE: 10pt">Phone: +64 (0)9 3737599 ext 86487</span></i></p>
<p class="MsoNormal"><i><span style="FONT-FAMILY: 'Calibri','sans-serif'; COLOR: #595959; FONT-SIZE: 10pt">DDI: +64 (0)9 924 6487</span></i></p>
<p class="MsoNormal"><i><span style="FONT-FAMILY: 'Calibri','sans-serif'; COLOR: #595959; FONT-SIZE: 10pt">Mobile: +64 (0)21 753 189</span></i></p>
<p class="MsoNormal"><i><span style="FONT-FAMILY: 'Calibri','sans-serif'; COLOR: #595959; FONT-SIZE: 10pt">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 style="FONT-FAMILY: Webdings; COLOR: green; FONT-SIZE: 18pt" lang="EN-GB">P</span><span style="FONT-FAMILY: 'Calibri','sans-serif'; COLOR: blue; FONT-SIZE: 11pt" lang="EN-GB"> </span><span style="FONT-FAMILY: 'Arial Narrow','sans-serif'; COLOR: green; FONT-SIZE: 10pt" lang="EN-GB">Please consider the environment before printing this e-mail</span><span style="FONT-FAMILY: 'Calibri','sans-serif'; COLOR: blue; FONT-SIZE: 11pt" lang="EN-GB"> </span><span style="FONT-FAMILY: 'Verdana','sans-serif'; COLOR: navy; FONT-SIZE: 7.5pt" lang="EN-GB"></span></p>
<p class="MsoNormal"><i><span style="FONT-FAMILY: 'Calibri','sans-serif'; COLOR: #1f497d; FONT-SIZE: 10pt"></span></i> </p>
<div style="BORDER-BOTTOM: medium none; BORDER-LEFT: blue 1.5pt solid; PADDING-BOTTOM: 0cm; PADDING-LEFT: 4pt; PADDING-RIGHT: 0cm; BORDER-TOP: medium none; BORDER-RIGHT: medium none; PADDING-TOP: 0cm">
<div>
<div>
<p style="MARGIN-BOTTOM: 12pt" class="MsoNormal"> </p></div></div></div></div></div></div></body></html>