From: oetiker Date: Tue, 2 Mar 2010 23:22:35 +0000 (+0000) Subject: I think I just solved a longstanding bug in the rrd_update routine X-Git-Url: https://git.octo.it/?p=rrdtool.git;a=commitdiff_plain;h=2c2a2654452de47d1504e7a5b6e4e63d0abb860e I think I just solved a longstanding bug in the rrd_update routine ... I am writing a tool for seeding new rrd files from an existing one ... essentially enabeling arbitrary restructuring ... while doing this I found the following: pdp: 1 2 | 3 4 5 6 7 | 8 9 for a consolidation of 5 steps I get AVG RRA .. | 5 | .. MIN RRA .. | 2 | .. MAX RRA .. | 7 | .. while I would have expected to get AVG RRA .. | 5 | .. MIN RRA .. | 3 | .. MAX RRA .. | 7 | .. the behavior has been like this at least since 1.2 looking at the code I found that the behavior had been introduced by a patch optimizing rrd_update for updates covering multiple pdps in one go ... in that optimization, the normal case where an update covers exactly ONE pdp had been sort of neglected ... so much for tunel vision. The problem was, that the cdp value for MIN and MAX got initialized from the last pdp value BEFORE the consolidation interval ... for normal data no one would notice (or did notice) but it is still wrong ... git-svn-id: svn://svn.oetiker.ch/rrdtool/trunk/program@2024 a5681a0c-68f1-0310-ab6d-d61299d08faa --- diff --git a/src/rrd_update.c b/src/rrd_update.c index ec1e5fd..83ef706 100644 --- a/src/rrd_update.c +++ b/src/rrd_update.c @@ -212,7 +212,6 @@ static void initialize_cdp_val( unival *scratch, int current_cf, rrd_value_t pdp_temp_val, - unsigned long elapsed_pdp_st, unsigned long start_pdp_offset, unsigned long pdp_cnt); @@ -227,8 +226,9 @@ static void reset_cdp( int cdp_idx, enum cf_en current_cf); -static rrd_value_t initialize_average_carry_over( +static rrd_value_t initialize_carry_over( rrd_value_t pdp_temp_val, + int current_cf, unsigned long elapsed_pdp_st, unsigned long start_pdp_offset, unsigned long pdp_cnt); @@ -1593,18 +1593,15 @@ static void update_cdp( if (*cdp_unkn_pdp_cnt > pdp_cnt * xff) { *cdp_primary_val = DNAN; - if (current_cf == CF_AVERAGE) { - *cdp_val = - initialize_average_carry_over(pdp_temp_val, - elapsed_pdp_st, - start_pdp_offset, pdp_cnt); - } else { - *cdp_val = pdp_temp_val; - } } else { initialize_cdp_val(scratch, current_cf, pdp_temp_val, - elapsed_pdp_st, start_pdp_offset, pdp_cnt); - } /* endif meets xff value requirement for a valid value */ + start_pdp_offset, pdp_cnt); + } + *cdp_val = + initialize_carry_over(pdp_temp_val,current_cf, + elapsed_pdp_st, + start_pdp_offset, pdp_cnt); + /* endif meets xff value requirement for a valid value */ /* initialize carry over CDP_unkn_pdp_cnt, this must after CDP_primary_val * is set because CDP_unkn_pdp_cnt is required to compute that value. */ if (isnan(pdp_temp_val)) @@ -1640,7 +1637,6 @@ static void initialize_cdp_val( unival *scratch, int current_cf, rrd_value_t pdp_temp_val, - unsigned long elapsed_pdp_st, unsigned long start_pdp_offset, unsigned long pdp_cnt) { @@ -1653,13 +1649,11 @@ static void initialize_cdp_val( scratch[CDP_primary_val].u_val = (cum_val + cur_val * start_pdp_offset) / (pdp_cnt - scratch[CDP_unkn_pdp_cnt].u_cnt); - scratch[CDP_val].u_val = - initialize_average_carry_over(pdp_temp_val, elapsed_pdp_st, - start_pdp_offset, pdp_cnt); break; - case CF_MAXIMUM: + case CF_MAXIMUM: cum_val = IFDNAN(scratch[CDP_val].u_val, -DINF); cur_val = IFDNAN(pdp_temp_val, -DINF); + #if 0 #ifdef DEBUG if (isnan(scratch[CDP_val].u_val) && isnan(pdp_temp)) { @@ -1674,8 +1668,6 @@ static void initialize_cdp_val( scratch[CDP_primary_val].u_val = cur_val; else scratch[CDP_primary_val].u_val = cum_val; - /* initialize carry over value */ - scratch[CDP_val].u_val = pdp_temp_val; break; case CF_MINIMUM: cum_val = IFDNAN(scratch[CDP_val].u_val, DINF); @@ -1694,14 +1686,10 @@ static void initialize_cdp_val( scratch[CDP_primary_val].u_val = cur_val; else scratch[CDP_primary_val].u_val = cum_val; - /* initialize carry over value */ - scratch[CDP_val].u_val = pdp_temp_val; break; case CF_LAST: default: scratch[CDP_primary_val].u_val = pdp_temp_val; - /* initialize carry over value */ - scratch[CDP_val].u_val = pdp_temp_val; break; } } @@ -1763,17 +1751,34 @@ static void reset_cdp( } } -static rrd_value_t initialize_average_carry_over( +static rrd_value_t initialize_carry_over( rrd_value_t pdp_temp_val, + int current_cf, unsigned long elapsed_pdp_st, unsigned long start_pdp_offset, unsigned long pdp_cnt) { - /* initialize carry over value */ - if (isnan(pdp_temp_val)) { - return DNAN; - } - return pdp_temp_val * ((elapsed_pdp_st - start_pdp_offset) % pdp_cnt); + unsigned long pdp_into_cdp_cnt = ((elapsed_pdp_st - start_pdp_offset) % pdp_cnt); + if ( pdp_into_cdp_cnt == 0 || isnan(pdp_temp_val)){ + switch (current_cf) { + case CF_MAXIMUM: + return -DINF; + case CF_MINIMUM: + return DINF; + case CF_AVERAGE: + return 0; + default: + return DNAN; + } + } + else { + switch (current_cf) { + case CF_AVERAGE: + return pdp_temp_val * pdp_into_cdp_cnt ; + default: + return pdp_temp_val; + } + } } /*