From: jkrabbe Date: Tue, 26 Mar 2013 14:25:02 +0000 (+0100) Subject: csnmp_read_table: Change GETNEXT request behaviour (+ bugfix 235) X-Git-Tag: collectd-5.1.3~2^2~3 X-Git-Url: https://git.verplant.org/?a=commitdiff_plain;h=080f01b95a5f6006438e2ffee6ae3d093263d148;p=collectd.git csnmp_read_table: Change GETNEXT request behaviour (+ bugfix 235) This patch changes the snmp GETNEXT request behaviour implemented in snmp.c. The old implementation requested all OIDs using GETNEXT requests until all OIDs left their own subtree. In cases were trees in a Data template are much longer than other trees the shorter subtrees were re-requested over and over again. The new implementation will only request OIDs that did not already leave their subtrees (see the oid_todo_list implementation for details). This renders the function csnmp_check_res_left_subtree useless as the oid_todo_list keeps track if all OIDs have finished. During tests against Cat6500 (CatOS/IOS) as well as Nexus5k (NX-OS) it looks as though GETNEXT requests (when requesting multiple OIDs like all 14 dot3Stats errors from Etherlike-MIB) can take about 5-10ms (CatOS 30ms) longer if they wrap to the next OID. This does not sound much but when collecting data for the Etherlike-MIB (that only has entries for physical interfaces) with a collectd "Instance" variable in IF-MIB (that has entries for all physical as well as pseudo [SVIs, VLANs, ...] interfaces) this can make a notable difference (e.g. for core routers that have all SVIs and VLANs but only some switches attached): IOS-Core-Router ifName 550 entries dot3StatsFCSErrors 70 entries ------------ 480 entries * 10ms = 4.8s overhead CatOS-Access-Sw. ifName 840 entries dot3StatsFCSErrors 490 entries ------------ 350 entries * 30ms = 10.5s overhead After refactoring csnmp_read_table "Instance" and "Value" OIDs are now handled consistently (so no pointer-forward foo needed). It doesn't change any logic and data structures, though - so there should not be any impact to other functions. The refactored code also fixes GitHub bugs #235 and #258. This bug is due to reusing the status variable in following code section which might lead to errors if the subtrees are of different length: 1436 /* Calculate the current suffix. This is later used to check that the 1437 * suffix is increasing. This also checks if we left the subtree */ 1438 status = csnmp_oid_suffix (&suffix, &vb_name, data->values + i); Signed-off-by: Florian Forster --- diff --git a/src/snmp.c b/src/snmp.c index f4966694..7f325e97 100644 --- a/src/snmp.c +++ b/src/snmp.c @@ -896,71 +896,6 @@ static value_t csnmp_value_list_to_value (struct variable_list *vl, int type, return (ret); } /* value_t csnmp_value_list_to_value */ -/* Returns true if all OIDs have left their subtree */ -static int csnmp_check_res_left_subtree (const host_definition_t *host, - const data_definition_t *data, - struct snmp_pdu *res) -{ - struct variable_list *vb; - int num_checked; - int num_left_subtree; - int i; - - vb = res->variables; - if (vb == NULL) - return (-1); - - num_checked = 0; - num_left_subtree = 0; - - /* check all the variables and count how many have left their subtree */ - for (vb = res->variables, i = 0; - (vb != NULL) && (i < data->values_len); - vb = vb->next_variable, i++) - { - num_checked++; - - if ((vb->type == SNMP_ENDOFMIBVIEW) - || (snmp_oid_ncompare (data->values[i].oid, - data->values[i].oid_len, - vb->name, vb->name_length, - data->values[i].oid_len) != 0)) - num_left_subtree++; - } - - /* check if enough variables have been returned */ - if (i < data->values_len) - { - ERROR ("snmp plugin: host %s: Expected %i variables, but got only %i", - host->name, data->values_len, i); - return (-1); - } - - if (data->instance.oid.oid_len > 0) - { - if (vb == NULL) - { - ERROR ("snmp plugin: host %s: Expected one more variable for " - "the instance..", host->name); - return (-1); - } - - num_checked++; - if (snmp_oid_ncompare (data->instance.oid.oid, - data->instance.oid.oid_len, - vb->name, vb->name_length, - data->instance.oid.oid_len) != 0) - num_left_subtree++; - } - - DEBUG ("snmp plugin: csnmp_check_res_left_subtree: %i of %i variables have " - "left their subtree", - num_left_subtree, num_checked); - if (num_left_subtree >= num_checked) - return (1); - return (0); -} /* int csnmp_check_res_left_subtree */ - static int csnmp_strvbcopy_hexstring (char *dst, /* {{{ */ const struct variable_list *vb, size_t dst_size) { @@ -1268,7 +1203,7 @@ static int csnmp_read_table (host_definition_t *host, data_definition_t *data) uint32_t oid_list_len; int status; - int i; + int i, j; /* `value_list_head' and `value_list_tail' implement a linked list for each * value. `instance_list_head' and `instance_list_tail' implement a linked list of @@ -1315,6 +1250,11 @@ static int csnmp_read_table (host_definition_t *host, data_definition_t *data) else oid_list_len--; + /* We also need a 0(=finished)|1(=todo) mask for a todo list */ + int oid_todo_list[oid_list_len]; + for (i=0;isess_handle, req, &res); @@ -1382,78 +1334,70 @@ static int csnmp_read_table (host_definition_t *host, data_definition_t *data) break; } - /* Check if all values (and possibly the instance) have left their - * subtree */ - if (csnmp_check_res_left_subtree (host, data, res) != 0) + for (vb = res->variables, i = 0; (vb != NULL); vb = vb->next_variable, i++) { - status = 0; - break; - } + /* Calculate value index from todo list */ + while(oid_todo_list[i] == 0 && i < oid_list_len) + i++; + + /* An instance is configured and the res variable we process is the instance value (last index) */ + if (data->instance.oid.oid_len > 0 && i == data->values_len) { + + if ((vb->type == SNMP_ENDOFMIBVIEW) + || (snmp_oid_ncompare (data->instance.oid.oid, + data->instance.oid.oid_len, + vb->name, vb->name_length, + data->instance.oid.oid_len) != 0)) + { + DEBUG ("snmp plugin: host = %s; data = %s; Instance left its subtree.", + host->name, data->name); + oid_todo_list[i] = 0; + continue; + } - /* Copy the OID of the value used as instance to oid_list, if an instance - * is configured. */ - if (data->instance.oid.oid_len > 0) - { /* Allocate a new `csnmp_list_instances_t', insert the instance name and * add it to the list */ if (csnmp_instance_list_add (&instance_list_head, &instance_list_tail, - res, host, data) != 0) + res, host, data) != 0) { ERROR ("snmp plugin: csnmp_instance_list_add failed."); status = -1; break; } - /* The instance OID is added to the list of OIDs to GET from the - * snmp agent last, so set vb on the last variable returned and copy - * that OID. */ - for (vb = res->variables; - (vb != NULL) && (vb->next_variable != NULL); - vb = vb->next_variable) - /* do nothing */; - assert (vb != NULL); - - /* Copy the OID of the instance value to oid_list[data->values_len]. - * "oid_list" is used for the next GETNEXT request. */ - memcpy (oid_list[data->values_len].oid, vb->name, - sizeof (oid) * vb->name_length); - oid_list[data->values_len].oid_len = vb->name_length; - } - - /* Iterate over all the (non-instance) values returned by the agent. The - * (i < value_len) check will make sure we're not handling the instance OID - * twice. */ - for (vb = res->variables, i = 0; - (vb != NULL) && (i < data->values_len); - vb = vb->next_variable, i++) - { - csnmp_table_values_t *vt; - oid_t vb_name; - oid_t suffix; - - csnmp_oid_init (&vb_name, vb->name, vb->name_length); - - /* Calculate the current suffix. This is later used to check that the - * suffix is increasing. This also checks if we left the subtree */ - status = csnmp_oid_suffix (&suffix, &vb_name, data->values + i); - if (status != 0) - { - DEBUG ("snmp plugin: host = %s; data = %s; Value %i failed. " - "It probably left its subtree.", - host->name, data->name, i); - continue; - } - - /* Make sure the OIDs returned by the agent are increasing. Otherwise our - * table matching algorithm will get confused. */ - if ((value_list_tail[i] != NULL) - && (csnmp_oid_compare (&suffix, &value_list_tail[i]->suffix) <= 0)) - { - DEBUG ("snmp plugin: host = %s; data = %s; i = %i; " - "Suffix is not increasing.", - host->name, data->name, i); - continue; - } + /* The variable we are processing is a normal value */ + } else { + + csnmp_table_values_t *vt; + oid_t vb_name; + oid_t suffix; + + csnmp_oid_init (&vb_name, vb->name, vb->name_length); + + /* Calculate the current suffix. This is later used to check that the + * suffix is increasing. This also checks if we left the subtree */ + int ret; + ret = csnmp_oid_suffix (&suffix, &vb_name, data->values + i); + if (ret != 0) + { + DEBUG ("snmp plugin: host = %s; data = %s; i = %i; " + "Value probably left its subtree.", + host->name, data->name, i); + oid_todo_list[i] = 0; + continue; + } + + /* Make sure the OIDs returned by the agent are increasing. Otherwise our + * table matching algorithm will get confused. */ + if ((value_list_tail[i] != NULL) + && (csnmp_oid_compare (&suffix, &value_list_tail[i]->suffix) <= 0)) + { + DEBUG ("snmp plugin: host = %s; data = %s; i = %i; " + "Suffix is not increasing.", + host->name, data->name, i); + oid_todo_list[i] = 0; + continue; + } vt = malloc (sizeof (*vt)); if (vt == NULL) @@ -1475,10 +1419,12 @@ static int csnmp_read_table (host_definition_t *host, data_definition_t *data) value_list_tail[i]->next = vt; value_list_tail[i] = vt; - /* Copy OID to oid_list[i + 1] */ + } + /* Copy OID to oid_list[i] */ memcpy (oid_list[i].oid, vb->name, sizeof (oid) * vb->name_length); oid_list[i].oid_len = vb->name_length; - } /* for (i = data->values_len) */ + + } /* for (vb = res->variables ...) */ if (res != NULL) snmp_free_pdu (res);