From 0643de05abd288f6864df1d62c9df603dc85e79a Mon Sep 17 00:00:00 2001 From: Yves Mettier Date: Tue, 26 Mar 2013 18:25:33 +0100 Subject: [PATCH] Fixes for code quality Signed-off-by: Florian Forster --- src/configfile.c | 27 +++++++++ src/configfile.h | 2 + src/plugin.c | 148 +++++++++++++++-------------------------------- 3 files changed, 77 insertions(+), 100 deletions(-) diff --git a/src/configfile.c b/src/configfile.c index 88bed1c4..193e05d2 100644 --- a/src/configfile.c +++ b/src/configfile.c @@ -917,6 +917,33 @@ const char *global_option_get (const char *option) : cf_global_options[i].def); } /* char *global_option_get */ +long global_option_get_long (const char *option, long default_value) +{ + const char *str; + long value; + + str = global_option_get(option); + if(NULL == str) return(default_value); + + errno = 0; + value = strtol(str, NULL, 10); + if (errno == ERANGE && (value == LONG_MAX || value == LONG_MIN)) return(default_value); + if (errno != 0 && value == 0) return(default_value); + return(value); +} /* char *global_option_get_long */ + +long global_option_get_long_in_range (const char *option, long default_value, long min, long max) +{ + long value; + + assert(min < max); + value = global_option_get_long(option, default_value); + if(value < min) return(default_value); + if(value > max) return(default_value); + return(value); + +} /* char *global_option_get_long_in_range */ + cdtime_t cf_get_default_interval (void) { char const *str = global_option_get ("Interval"); diff --git a/src/configfile.h b/src/configfile.h index 5a719a42..c91fcd5f 100644 --- a/src/configfile.h +++ b/src/configfile.h @@ -86,6 +86,8 @@ int cf_read (char *filename); int global_option_set (const char *option, const char *value); const char *global_option_get (const char *option); +long global_option_get_long (const char *option, long default_value); +long global_option_get_long_in_range (const char *option, long default_value, long min, long max); cdtime_t cf_get_default_interval (void); diff --git a/src/plugin.c b/src/plugin.c index 4e106a5b..4f1f6e16 100644 --- a/src/plugin.c +++ b/src/plugin.c @@ -31,6 +31,7 @@ #include "utils_llist.h" #include "utils_heap.h" #include "utils_time.h" +#include "utils_random.h" #if HAVE_PTHREAD_H # include @@ -108,7 +109,7 @@ static int read_threads_num = 0; static write_queue_t *write_queue_head; static write_queue_t *write_queue_tail; -static long write_queue_size = 0; +static long write_queue_length = 0; static _Bool write_loop = 1; static pthread_mutex_t write_lock = PTHREAD_MUTEX_INITIALIZER; static pthread_cond_t write_cond = PTHREAD_COND_INITIALIZER; @@ -118,10 +119,9 @@ static size_t write_threads_num = 0; static pthread_key_t plugin_ctx_key; static _Bool plugin_ctx_key_initialized = 0; -static long writequeuelengthlimit_high = 0; -static long writequeuelengthlimit_low = 0; - -static unsigned int random_seed; +static long write_limit_high = 0; +static long write_limit_low = 0; +cdtime_t last_drop_time = 0; /* * Static functions @@ -676,13 +676,13 @@ static int plugin_write_enqueue (value_list_t const *vl) /* {{{ */ { write_queue_head = q; write_queue_tail = q; - write_queue_size = 1; + write_queue_length = 1; } else { write_queue_tail->next = q; write_queue_tail = q; - write_queue_size += 1; + write_queue_length += 1; } pthread_cond_signal (&write_cond); @@ -709,10 +709,10 @@ static value_list_t *plugin_write_dequeue (void) /* {{{ */ q = write_queue_head; write_queue_head = q->next; - write_queue_size -= 1; + write_queue_length -= 1; if (write_queue_head == NULL) { write_queue_tail = NULL; - write_queue_size = 0; /* Maybe instead of setting write_queue_size to 0, we should assert(write_queue_size == 0) ? */ + assert(0 == write_queue_length); } pthread_mutex_unlock (&write_lock); @@ -816,7 +816,7 @@ static void stop_write_threads (void) /* {{{ */ } write_queue_head = NULL; write_queue_tail = NULL; - write_queue_size = 0; + write_queue_length = 0; pthread_mutex_unlock (&write_lock); if (i > 0) @@ -1448,60 +1448,8 @@ void plugin_init_all (void) chain_name = global_option_get ("PostCacheChain"); post_cache_chain = fc_chain_get_by_name (chain_name); - { - const char *str_writequeuelengthlimithigh = global_option_get ("WriteQueueLengthLimitHigh"); - const char *str_writequeuelengthlimitlow = global_option_get ("WriteQueueLengthLimitLow"); - - writequeuelengthlimit_high = 0; - writequeuelengthlimit_low = 0; - - if(NULL != str_writequeuelengthlimithigh) { - errno = 0; - /* get high limit */ - writequeuelengthlimit_high = strtol(str_writequeuelengthlimithigh, NULL, 10); - if ((errno == ERANGE && (writequeuelengthlimit_high == LONG_MAX || writequeuelengthlimit_high == LONG_MIN)) - || (errno != 0 && writequeuelengthlimit_high == 0) - ) { - writequeuelengthlimit_high = 0; - ERROR("Config 'WriteQueueLengthLimitHigh' accepts one integer value only. Running with no limit !"); - } - if(writequeuelengthlimit_high < 0) { - ERROR("Config 'WriteQueueLengthLimitHigh' accepts positive values only. Running with no limit !"); - writequeuelengthlimit_high = 0; - } - } - - if((writequeuelengthlimit_high > 0) && (NULL != str_writequeuelengthlimitlow)) { - errno = 0; - /* get low limit */ - writequeuelengthlimit_low = strtol(str_writequeuelengthlimitlow, NULL, 10); - if ((errno == ERANGE && (writequeuelengthlimit_low == LONG_MAX || writequeuelengthlimit_low == LONG_MIN)) - || (errno != 0 && writequeuelengthlimit_low == 0) - ) { - writequeuelengthlimit_low = 0; - ERROR("Config 'WriteQueueLengthLimitLow' accepts one integer value only. Using default low limit instead"); - } - - if(writequeuelengthlimit_low < 0) { - ERROR("Config 'WriteQueueLengthLimitLow' accepts positive values only. Using default low limit instead"); - writequeuelengthlimit_low = 0; - } else if(writequeuelengthlimit_low > writequeuelengthlimit_high) { - ERROR("Config 'WriteQueueLengthLimitLow' (%ld) cannot be bigger than high limit (%ld). Using default low limit instead", - writequeuelengthlimit_low, writequeuelengthlimit_high); - writequeuelengthlimit_low = 0; - } - } - /* Check/compute low limit if not/badly defined */ - if(writequeuelengthlimit_high > 0) { - if(0 == writequeuelengthlimit_low) { - writequeuelengthlimit_low = .5 * writequeuelengthlimit_high; - } - INFO("Config 'WriteQueueLengthLimit*' : Running with limits high=%ld low=%ld", writequeuelengthlimit_high, writequeuelengthlimit_low); - random_seed = time(0); - } else { - writequeuelengthlimit_low = 0; /* This should be useless, but in case... */ - } - } + write_limit_high = global_option_get_long_in_range("WriteQueueLengthLimitHigh",0, 0, LONG_MAX); + write_limit_low = global_option_get_long_in_range("WriteQueueLengthLimitLow", (write_limit_high+1)/2, 0, write_limit_high-1 ); { char const *tmp = global_option_get ("WriteThreads"); @@ -2017,63 +1965,63 @@ static int plugin_dispatch_values_internal (value_list_t *vl) return (0); } /* int plugin_dispatch_values_internal */ -int plugin_dispatch_values (value_list_t const *vl) -{ - int status; - int wq_size = write_queue_size; - /* We store write_queue_size in a local variable because other threads may update write_queue_size. +static _Bool drop_metric(void) { + _Bool drop = 0; + int wq_len = write_queue_length; + /* We store write_queue_length in a local variable because other threads may update write_queue_length. * Having this in a local variable (like a cache) is better : we do not need a lock */ - short metric_will_be_dropped = 0; - if((writequeuelengthlimit_high > 0) && (wq_size > writequeuelengthlimit_low)) { - if(wq_size >= writequeuelengthlimit_high) { + if(wq_len < write_limit_low) return(0); + + if((write_limit_high > 0) && (wq_len > write_limit_low)) { + if(wq_len >= write_limit_high) { /* if high == low, we come here too */ - metric_will_be_dropped = 1; + drop = 1; } else { /* here, high != low */ long probability_to_drop; long n; - probability_to_drop = (wq_size - writequeuelengthlimit_low); + probability_to_drop = (wq_len - write_limit_low); - /* We use rand_r with its bad RNG because it's enough for playing dices. - * There is no security consideration here so rand_r() should be enough here. - */ - n = rand_r(&random_seed) % (writequeuelengthlimit_high - writequeuelengthlimit_low) ; + n = cdrand_range(write_limit_low, write_limit_high); /* Let's have X = high - low. * n is in range [0..X] * probability_to_drop is in range [1..X[ - * probability_to_drop gets bigger when wq_size gets bigger. + * probability_to_drop gets bigger when wq_len gets bigger. */ if(n <= probability_to_drop) { - metric_will_be_dropped = 1; + drop = 1; } } } - - if( ! metric_will_be_dropped) { - status = plugin_write_enqueue (vl); - if (status != 0) - { - char errbuf[1024]; - ERROR ("plugin_dispatch_values: plugin_write_enqueue failed " - "with status %i (%s).", status, - sstrerror (status, errbuf, sizeof (errbuf))); - return (status); + if(drop) { + cdtime_t now = cdtime(); + if((now - last_drop_time) > TIME_T_TO_CDTIME_T (60)) { + last_drop_time = now; + /* If you want to count dropped metrics, don't forget to add a lock here */ + /* dropped_metrics++; */ + ERROR ("plugin_dispatch_values : Low water mark reached, dropping a metric"); } } - else + return(drop); +} + +int plugin_dispatch_values (value_list_t const *vl) +{ + int status; + + if(drop_metric ()) return(0); + + status = plugin_write_enqueue (vl); + if (status != 0) { - /* If you want to count dropped metrics, don't forget to add a lock here */ - /* dropped_metrics++; */ - ERROR ("plugin_dispatch_values: value dropped (write queue %ld > %ld) : time = %.3f; interval = %.3f ; %s/%s%s%s/%s%s%s", - write_queue_size, writequeuelengthlimit_low, - CDTIME_T_TO_DOUBLE (vl->time), - CDTIME_T_TO_DOUBLE (vl->interval), - vl->host, - vl->plugin, vl->plugin_instance[0]?"-":"", vl->plugin_instance, - vl->type, vl->type_instance[0]?"-":"", vl->type_instance); + char errbuf[1024]; + ERROR ("plugin_dispatch_values: plugin_write_enqueue failed " + "with status %i (%s).", status, + sstrerror (status, errbuf, sizeof (errbuf))); + return (status); } return (0); -- 2.30.2