X-Git-Url: https://git.octo.it/?a=blobdiff_plain;f=src%2Fsnmp.c;h=45d13c73e4d6f0959672985ec80cea558406ba5a;hb=69f341f4042f56279be79501d3222c6cb1382e85;hp=23e199ecf6113ad99ab62e68f54ed0a7f1d5d2bc;hpb=fe9ba13c8866ada19f7a347f3757d4c740613cdb;p=collectd.git

diff --git a/src/snmp.c b/src/snmp.c
index 23e199ec..45d13c73 100644
--- a/src/snmp.c
+++ b/src/snmp.c
@@ -127,7 +127,7 @@ static void csnmp_host_definition_destroy (void *arg) /* {{{ */
   if (hd->name != NULL)
   {
     DEBUG ("snmp plugin: Destroying host definition for host `%s'.",
-	hd->name);
+        hd->name);
   }
 
   csnmp_host_close_session (hd);
@@ -213,10 +213,10 @@ static int csnmp_config_add_data_instance (data_definition_t *dd, oconfig_item_t
     dd->instance.oid.oid_len = MAX_OID_LEN;
 
     if (!read_objid (ci->values[0].value.string,
-	  dd->instance.oid.oid, &dd->instance.oid.oid_len))
+          dd->instance.oid.oid, &dd->instance.oid.oid_len))
     {
       ERROR ("snmp plugin: read_objid (%s) failed.",
-	  ci->values[0].value.string);
+          ci->values[0].value.string);
       return (-1);
     }
   }
@@ -224,7 +224,7 @@ static int csnmp_config_add_data_instance (data_definition_t *dd, oconfig_item_t
   {
     /* Instance is a simple string */
     sstrncpy (dd->instance.string, ci->values[0].value.string,
-	sizeof (dd->instance.string));
+        sizeof (dd->instance.string));
   }
 
   return (0);
@@ -242,7 +242,7 @@ static int csnmp_config_add_data_instance_prefix (data_definition_t *dd,
   if (!dd->is_table)
   {
     WARNING ("snmp plugin: data %s: InstancePrefix is ignored when `Table' "
-	"is set to `false'.", dd->name);
+        "is set to `false'.", dd->name);
     return (-1);
   }
 
@@ -283,10 +283,10 @@ static int csnmp_config_add_data_values (data_definition_t *dd, oconfig_item_t *
     dd->values[i].oid_len = MAX_OID_LEN;
 
     if (NULL == snmp_parse_oid (ci->values[i].value.string,
-	  dd->values[i].oid, &dd->values[i].oid_len))
+          dd->values[i].oid, &dd->values[i].oid_len))
     {
       ERROR ("snmp plugin: snmp_parse_oid (%s) failed.",
-	  ci->values[i].value.string);
+          ci->values[i].value.string);
       free (dd->values);
       dd->values = NULL;
       dd->values_len = 0;
@@ -302,7 +302,7 @@ static int csnmp_config_add_data_shift (data_definition_t *dd, oconfig_item_t *c
   if ((ci->values_num != 1)
       || (ci->values[0].type != OCONFIG_TYPE_NUMBER))
   {
-    WARNING ("snmp plugin: The `Scale' config option needs exactly one number argument.");
+    WARNING ("snmp plugin: The `Shift' config option needs exactly one number argument.");
     return (-1);
   }
 
@@ -524,17 +524,17 @@ static int csnmp_config_add_host_collect (host_definition_t *host,
   {
     for (data = data_head; data != NULL; data = data->next)
       if (strcasecmp (ci->values[i].value.string, data->name) == 0)
-	break;
+        break;
 
     if (data == NULL)
     {
       WARNING ("snmp plugin: No such data configured: `%s'",
-	  ci->values[i].value.string);
+          ci->values[i].value.string);
       continue;
     }
 
     DEBUG ("snmp plugin: Collect: host = %s, data[%i] = %s;",
-	host->name, host->data_list_len, data->name);
+        host->name, host->data_list_len, data->name);
 
     host->data_list[host->data_list_len] = data;
     host->data_list_len++;
@@ -655,8 +655,9 @@ static int csnmp_config_add_host (oconfig_item_t *ci)
   if (hd->interval != 0)
     cb_interval.tv_sec = (time_t) hd->interval;
 
-  status = plugin_register_complex_read (cb_name, csnmp_read_host,
-      /* interval = */ &cb_interval, /* user_data = */ &cb_data);
+  status = plugin_register_complex_read (/* group = */ NULL, cb_name,
+      csnmp_read_host, /* interval = */ &cb_interval,
+      /* user_data = */ &cb_data);
   if (status != 0)
   {
     ERROR ("snmp plugin: Registering complex read function failed.");
@@ -714,7 +715,7 @@ static void csnmp_host_open_session (host_definition_t *host)
     snmp_error (&sess, NULL, NULL, &errstr);
 
     ERROR ("snmp plugin: host %s: snmp_sess_open failed: %s",
-	host->name, (errstr == NULL) ? "Unknown problem" : errstr);
+        host->name, (errstr == NULL) ? "Unknown problem" : errstr);
     sfree (errstr);
   }
 } /* void csnmp_host_open_session */
@@ -724,8 +725,11 @@ static value_t csnmp_value_list_to_value (struct variable_list *vl, int type,
     double scale, double shift)
 {
   value_t ret;
-  uint64_t temp = 0;
-  int defined = 1;
+  uint64_t tmp_unsigned = 0;
+  int64_t tmp_signed = 0;
+  _Bool defined = 1;
+  /* Set to true when the original SNMP type appears to have been signed. */
+  _Bool prefer_signed = 0;
 
   if ((vl->type == ASN_INTEGER)
       || (vl->type == ASN_UINTEGER)
@@ -735,15 +739,22 @@ static value_t csnmp_value_list_to_value (struct variable_list *vl, int type,
 #endif
       || (vl->type == ASN_GAUGE))
   {
-    temp = (uint32_t) *vl->val.integer;
-    DEBUG ("snmp plugin: Parsed int32 value is %"PRIu64".", temp);
+    tmp_unsigned = (uint32_t) *vl->val.integer;
+    tmp_signed = (int32_t) *vl->val.integer;
+
+    if ((vl->type == ASN_INTEGER)
+        || (vl->type == ASN_GAUGE))
+      prefer_signed = 1;
+
+    DEBUG ("snmp plugin: Parsed int32 value is %"PRIu64".", tmp_unsigned);
   }
   else if (vl->type == ASN_COUNTER64)
   {
-    temp = (uint32_t) vl->val.counter64->high;
-    temp = temp << 32;
-    temp += (uint32_t) vl->val.counter64->low;
-    DEBUG ("snmp plugin: Parsed int64 value is %"PRIu64".", temp);
+    tmp_unsigned = (uint32_t) vl->val.counter64->high;
+    tmp_unsigned = tmp_unsigned << 32;
+    tmp_unsigned += (uint32_t) vl->val.counter64->low;
+    tmp_signed = (int64_t) tmp_unsigned;
+    DEBUG ("snmp plugin: Parsed int64 value is %"PRIu64".", tmp_unsigned);
   }
   else if (vl->type == ASN_OCTET_STR)
   {
@@ -751,15 +762,28 @@ static value_t csnmp_value_list_to_value (struct variable_list *vl, int type,
   }
   else
   {
-    WARNING ("snmp plugin: I don't know the ASN type `%i'", (int) vl->type);
+    char oid_buffer[1024];
+
+    memset (oid_buffer, 0, sizeof (oid_buffer));
+    snprint_objid (oid_buffer, sizeof (oid_buffer) - 1,
+        vl->name, vl->name_length);
+
+#ifdef ASN_NULL
+    if (vl->type == ASN_NULL)
+      INFO ("snmp plugin: OID \"%s\" is undefined (type ASN_NULL)",
+          oid_buffer);
+    else
+#endif
+      WARNING ("snmp plugin: I don't know the ASN type \"%i\" (OID: %s)",
+          (int) vl->type, oid_buffer);
+
     defined = 0;
   }
 
   if (vl->type == ASN_OCTET_STR)
   {
-    char *endptr;
+    int status = -1;
 
-    endptr = NULL;
     if (vl->val.string != NULL)
     {
       char string[64];
@@ -767,47 +791,73 @@ static value_t csnmp_value_list_to_value (struct variable_list *vl, int type,
 
       string_length = sizeof (string) - 1;
       if (vl->val_len < string_length)
-	string_length = vl->val_len;
+        string_length = vl->val_len;
 
       /* The strings we get from the Net-SNMP library may not be null
-       * terminated. That is why we're using `membpy' here and not `strcpy'.
+       * terminated. That is why we're using `memcpy' here and not `strcpy'.
        * `string_length' is set to `vl->val_len' which holds the length of the
        * string.  -octo */
       memcpy (string, vl->val.string, string_length);
       string[string_length] = 0;
 
-      if (type == DS_TYPE_COUNTER)
-      {
-	ret.counter = (counter_t) strtoll (string, &endptr, /* base = */ 0);
-	DEBUG ("snmp plugin: csnmp_value_list_to_value: String to counter: %s -> %llu",
-	    string, (unsigned long long) ret.counter);
-      }
-      else if (type == DS_TYPE_GAUGE)
+      status = parse_value (string, &ret, type);
+      if (status != 0)
       {
-	ret.gauge = (gauge_t) strtod (string, &endptr);
-	DEBUG ("snmp plugin: csnmp_value_list_to_value: String to gauge: %s -> %g",
-	    string, (double) ret.gauge);
+        ERROR ("snmp plugin: csnmp_value_list_to_value: Parsing string as %s failed: %s",
+            DS_TYPE_TO_STRING (type), string);
       }
     }
 
-    /* Check if an error occurred */
-    if ((vl->val.string == NULL) || (endptr == (char *) vl->val.string))
+    if (status != 0)
     {
-      if (type == DS_TYPE_COUNTER)
-	ret.counter = 0;
-      else if (type == DS_TYPE_GAUGE)
-	ret.gauge = NAN;
+      switch (type)
+      {
+        case DS_TYPE_COUNTER:
+        case DS_TYPE_DERIVE:
+        case DS_TYPE_ABSOLUTE:
+          memset (&ret, 0, sizeof (ret));
+          break;
+
+        case DS_TYPE_GAUGE:
+          ret.gauge = NAN;
+          break;
+
+        default:
+          ERROR ("snmp plugin: csnmp_value_list_to_value: Unknown "
+              "data source type: %i.", type);
+          ret.gauge = NAN;
+      }
     }
-  }
+  } /* if (vl->type == ASN_OCTET_STR) */
   else if (type == DS_TYPE_COUNTER)
   {
-    ret.counter = temp;
+    ret.counter = tmp_unsigned;
   }
   else if (type == DS_TYPE_GAUGE)
   {
+    if (!defined)
+      ret.gauge = NAN;
+    else if (prefer_signed)
+      ret.gauge = (scale * tmp_signed) + shift;
+    else
+      ret.gauge = (scale * tmp_unsigned) + shift;
+  }
+  else if (type == DS_TYPE_DERIVE)
+  {
+    if (prefer_signed)
+      ret.derive = (derive_t) tmp_signed;
+    else
+      ret.derive = (derive_t) tmp_unsigned;
+  }
+  else if (type == DS_TYPE_ABSOLUTE)
+  {
+    ret.absolute = (absolute_t) tmp_unsigned;
+  }
+  else
+  {
+    ERROR ("snmp plugin: csnmp_value_list_to_value: Unknown data source "
+        "type: %i.", type);
     ret.gauge = NAN;
-    if (defined != 0)
-      ret.gauge = (scale * temp) + shift;
   }
 
   return (ret);
@@ -836,10 +886,12 @@ static int csnmp_check_res_left_subtree (const host_definition_t *host,
       vb = vb->next_variable, i++)
   {
     num_checked++;
-    if (snmp_oid_ncompare (data->values[i].oid,
-	  data->values[i].oid_len,
-	  vb->name, vb->name_length,
-	  data->values[i].oid_len) != 0)
+
+    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++;
   }
 
@@ -847,7 +899,7 @@ static int csnmp_check_res_left_subtree (const host_definition_t *host,
   if (i < data->values_len)
   {
     ERROR ("snmp plugin: host %s: Expected %i variables, but got only %i",
-	host->name, data->values_len, i);
+        host->name, data->values_len, i);
     return (-1);
   }
 
@@ -856,15 +908,15 @@ static int csnmp_check_res_left_subtree (const host_definition_t *host,
     if (vb == NULL)
     {
       ERROR ("snmp plugin: host %s: Expected one more variable for "
-	  "the instance..", host->name);
+          "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)
+          data->instance.oid.oid_len,
+          vb->name, vb->name_length,
+          data->instance.oid.oid_len) != 0)
       num_left_subtree++;
   }
 
@@ -876,6 +928,72 @@ static int csnmp_check_res_left_subtree (const host_definition_t *host,
   return (0);
 } /* int csnmp_check_res_left_subtree */
 
+static int csnmp_strvbcopy_hexstring (char *dst, /* {{{ */
+    const struct variable_list *vb, size_t dst_size)
+{
+  char *buffer_ptr;
+  size_t buffer_free;
+  size_t i;
+
+  buffer_ptr = dst;
+  buffer_free = dst_size;
+
+  for (i = 0; i < vb->val_len; i++)
+  {
+    int status;
+
+    status = snprintf (buffer_ptr, buffer_free,
+        (i == 0) ? "%02x" : ":%02x", (unsigned int) vb->val.bitstring[i]);
+
+    if (status >= buffer_free)
+    {
+      buffer_ptr += (buffer_free - 1);
+      *buffer_ptr = 0;
+      return (dst_size + (buffer_free - status));
+    }
+    else /* if (status < buffer_free) */
+    {
+      buffer_ptr += status;
+      buffer_free -= status;
+    }
+  }
+
+  return ((int) (dst_size - buffer_free));
+} /* }}} int csnmp_strvbcopy_hexstring */
+
+static int csnmp_strvbcopy (char *dst, /* {{{ */
+    const struct variable_list *vb, size_t dst_size)
+{
+  char *src;
+  size_t num_chars;
+  size_t i;
+
+  if (vb->type == ASN_OCTET_STR)
+    src = (char *) vb->val.string;
+  else if (vb->type == ASN_BIT_STR)
+    src = (char *) vb->val.bitstring;
+  else
+  {
+    dst[0] = 0;
+    return (EINVAL);
+  }
+
+  num_chars = dst_size - 1;
+  if (num_chars > vb->val_len)
+    num_chars = vb->val_len;
+
+  for (i = 0; i < num_chars; i++)
+  {
+    /* Check for control characters. */
+    if ((unsigned char)src[i] < 32)
+      return (csnmp_strvbcopy_hexstring (dst, vb, dst_size));
+    dst[i] = src[i];
+  }
+  dst[num_chars] = 0;
+
+  return ((int) vb->val_len);
+} /* }}} int csnmp_strvbcopy */
+
 static int csnmp_instance_list_add (csnmp_list_instances_t **head,
     csnmp_list_instances_t **tail,
     const struct snmp_pdu *res)
@@ -891,12 +1009,13 @@ static int csnmp_instance_list_add (csnmp_list_instances_t **head,
   if (vb == NULL)
     return (-1);
 
-  il = (csnmp_list_instances_t *) malloc (sizeof (csnmp_list_instances_t));
+  il = malloc (sizeof (*il));
   if (il == NULL)
   {
     ERROR ("snmp plugin: malloc failed.");
     return (-1);
   }
+  /* XXX copy entire OID */
   il->subid = vb->name[vb->name_length - 1];
   il->next = NULL;
 
@@ -904,24 +1023,15 @@ static int csnmp_instance_list_add (csnmp_list_instances_t **head,
   if ((vb->type == ASN_OCTET_STR) || (vb->type == ASN_BIT_STR))
   {
     char *ptr;
-    size_t instance_len;
 
-    memset (il->instance, 0, sizeof (il->instance));
-    instance_len = sizeof (il->instance) - 1;
-    if (instance_len > vb->val_len)
-      instance_len = vb->val_len;
-
-    sstrncpy (il->instance, (char *) ((vb->type == ASN_OCTET_STR)
-	  ? vb->val.string
-	  : vb->val.bitstring),
-	instance_len + 1);
+    csnmp_strvbcopy (il->instance, vb, sizeof (il->instance));
 
     for (ptr = il->instance; *ptr != '\0'; ptr++)
     {
       if ((*ptr > 0) && (*ptr < 32))
-	*ptr = ' ';
+        *ptr = ' ';
       else if (*ptr == '/')
-	*ptr = '_';
+        *ptr = '_';
     }
     DEBUG ("snmp plugin: il->instance = `%s';", il->instance);
   }
@@ -929,7 +1039,7 @@ static int csnmp_instance_list_add (csnmp_list_instances_t **head,
   {
     value_t val = csnmp_value_list_to_value (vb, DS_TYPE_COUNTER, 1.0, 0.0);
     ssnprintf (il->instance, sizeof (il->instance),
-	"%llu", val.counter);
+        "%llu", val.counter);
   }
 
   /* TODO: Debugging output */
@@ -1001,13 +1111,13 @@ static int csnmp_dispatch_table (host_definition_t *host, data_definition_t *dat
 
       if (instance_list_ptr == NULL)
       {
-	have_more = 0;
-	continue;
+        have_more = 0;
+        continue;
       }
       else if (instance_list_ptr->subid > subid)
       {
 	subid = instance_list_ptr->subid;
-	continue;
+        continue;
       }
     } /* if (instance_list != NULL) */
 
@@ -1019,13 +1129,13 @@ static int csnmp_dispatch_table (host_definition_t *host, data_definition_t *dat
 
       if (value_table_ptr[i] == NULL)
       {
-	have_more = 0;
-	break;
+        have_more = 0;
+        break;
       }
       else if (value_table_ptr[i]->subid > subid)
       {
 	subid = value_table_ptr[i]->subid;
-	break;
+        break;
       }
     } /* for (i = 0; i < columns; i++) */
     /* The subid has been increased - start scanning from the beginning
@@ -1052,15 +1162,15 @@ static int csnmp_dispatch_table (host_definition_t *host, data_definition_t *dat
       char temp[DATA_MAX_NAME_LEN];
 
       if (instance_list_ptr == NULL)
-	ssnprintf (temp, sizeof (temp), "%u", (uint32_t) subid);
+	ssnprintf (temp, sizeof (temp), "%"PRIu32, (uint32_t) subid);
       else
-	sstrncpy (temp, instance_list_ptr->instance, sizeof (temp));
+        sstrncpy (temp, instance_list_ptr->instance, sizeof (temp));
 
       if (data->instance_prefix == NULL)
-	sstrncpy (vl.type_instance, temp, sizeof (vl.type_instance));
+        sstrncpy (vl.type_instance, temp, sizeof (vl.type_instance));
       else
-	ssnprintf (vl.type_instance, sizeof (vl.type_instance), "%s%s",
-	    data->instance_prefix, temp);
+        ssnprintf (vl.type_instance, sizeof (vl.type_instance), "%s%s",
+            data->instance_prefix, temp);
     }
 
     for (i = 0; i < data->values_len; i++)
@@ -1091,13 +1201,13 @@ static int csnmp_read_table (host_definition_t *host, data_definition_t *data)
   int status;
   int i;
 
-  /* `value_table' and `value_table_ptr' implement a linked list for each
-   * value. `instance_list' and `instance_list_ptr' implement a linked list of
+  /* `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
    * instance names. This is used to jump gaps in the table. */
-  csnmp_list_instances_t *instance_list;
-  csnmp_list_instances_t *instance_list_ptr;
-  csnmp_table_values_t **value_table;
-  csnmp_table_values_t **value_table_ptr;
+  csnmp_list_instances_t *instance_list_head;
+  csnmp_list_instances_t *instance_list_tail;
+  csnmp_table_values_t **value_list_head;
+  csnmp_table_values_t **value_list_tail;
 
   DEBUG ("snmp plugin: csnmp_read_table (host = %s, data = %s)",
       host->name, data->name);
@@ -1118,7 +1228,7 @@ static int csnmp_read_table (host_definition_t *host, data_definition_t *data)
   if (ds->ds_num != data->values_len)
   {
     ERROR ("snmp plugin: DataSet `%s' requires %i values, but config talks about %i",
-	data->type, ds->ds_num, data->values_len);
+        data->type, ds->ds_num, data->values_len);
     return (-1);
   }
 
@@ -1136,20 +1246,22 @@ static int csnmp_read_table (host_definition_t *host, data_definition_t *data)
   else
     oid_list_len--;
 
-  /* Allocate the `value_table' */
-  value_table = (csnmp_table_values_t **) malloc (sizeof (csnmp_table_values_t *)
-      * 2 * data->values_len);
-  if (value_table == NULL)
+  /* We're going to construct n linked lists, one for each "value".
+   * value_list_head will contain pointers to the heads of these linked lists,
+   * value_list_tail will contain pointers to the tail of the lists. */
+  value_list_head = calloc (data->values_len, sizeof (*value_list_head));
+  value_list_tail = calloc (data->values_len, sizeof (*value_list_tail));
+  if ((value_list_head == NULL) || (value_list_tail == NULL))
   {
-    ERROR ("snmp plugin: csnmp_read_table: malloc failed.");
+    ERROR ("snmp plugin: csnmp_read_table: calloc failed.");
     sfree (oid_list);
+    sfree (value_list_head);
+    sfree (value_list_tail);
     return (-1);
   }
-  memset (value_table, '\0', sizeof (csnmp_table_values_t *) * 2 * data->values_len);
-  value_table_ptr = value_table + data->values_len;
-  
-  instance_list = NULL;
-  instance_list_ptr = NULL;
+
+  instance_list_head = NULL;
+  instance_list_tail = NULL;
 
   status = 0;
   while (status == 0)
@@ -1175,11 +1287,11 @@ static int csnmp_read_table (host_definition_t *host, data_definition_t *data)
       snmp_sess_error (host->sess_handle, NULL, NULL, &errstr);
 
       c_complain (LOG_ERR, &host->complaint,
-	  "snmp plugin: host %s: snmp_sess_synch_response failed: %s",
-	  host->name, (errstr == NULL) ? "Unknown problem" : errstr);
+          "snmp plugin: host %s: snmp_sess_synch_response failed: %s",
+          host->name, (errstr == NULL) ? "Unknown problem" : errstr);
 
       if (res != NULL)
-	snmp_free_pdu (res);
+        snmp_free_pdu (res);
       res = NULL;
 
       sfree (errstr);
@@ -1191,8 +1303,8 @@ static int csnmp_read_table (host_definition_t *host, data_definition_t *data)
     status = 0;
     assert (res != NULL);
     c_release (LOG_INFO, &host->complaint,
-	"snmp plugin: host %s: snmp_sess_synch_response successful.",
-	host->name);
+        "snmp plugin: host %s: snmp_sess_synch_response successful.",
+        host->name);
 
     vb = res->variables;
     if (vb == NULL)
@@ -1209,76 +1321,82 @@ static int csnmp_read_table (host_definition_t *host, data_definition_t *data)
       break;
     }
 
-    /* if an instance-OID is configured.. */
+    /* 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, &instance_list_ptr,
-	    res) != 0)
+      if (csnmp_instance_list_add (&instance_list_head, &instance_list_tail,
+            res, &data->instance.oid) != 0)
       {
-	ERROR ("snmp plugin: csnmp_instance_list_add failed.");
-	status = -1;
-	break;
+        ERROR ("snmp plugin: csnmp_instance_list_add failed.");
+        status = -1;
+        break;
       }
 
-      /* Set vb on the last variable */
+      /* 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 */;
+          (vb != NULL) && (vb->next_variable != NULL);
+          vb = vb->next_variable)
+        /* do nothing */;
       assert (vb != NULL);
 
-      /* Copy OID to oid_list[data->values_len] */
+      /* Copy the OID of the instance value to oid_list[data->values_len] */
       memcpy (oid_list[data->values_len].oid, vb->name,
-	  sizeof (oid) * vb->name_length);
+          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++)
+        (vb != NULL) && (i < data->values_len);
+        vb = vb->next_variable, i++)
     {
       csnmp_table_values_t *vt;
 
       /* Check if we left the subtree */
       if (snmp_oid_ncompare (data->values[i].oid,
-	    data->values[i].oid_len,
-	    vb->name, vb->name_length,
-	    data->values[i].oid_len) != 0)
+            data->values[i].oid_len,
+            vb->name, vb->name_length,
+            data->values[i].oid_len) != 0)
       {
-	DEBUG ("snmp plugin: host = %s; data = %s; Value %i left its subtree.",
-	    host->name, data->name, i);
-	continue;
+        DEBUG ("snmp plugin: host = %s; data = %s; Value %i left its subtree.",
+            host->name, data->name, i);
+        continue;
       }
 
-      if ((value_table_ptr[i] != NULL)
-	  && (vb->name[vb->name_length - 1] <= value_table_ptr[i]->subid))
+      if ((value_list_tail[i] != NULL)
+	  && (vb->name[vb->name_length - 1] <= value_list_tail[i]->subid))
       {
-	DEBUG ("snmp plugin: host = %s; data = %s; i = %i; "
-	    "SUBID is not increasing.",
-	    host->name, data->name, i);
-	continue;
+        DEBUG ("snmp plugin: host = %s; data = %s; i = %i; "
+            "SUBID is not increasing.",
+            host->name, data->name, i);
+        continue;
       }
 
       vt = (csnmp_table_values_t *) malloc (sizeof (csnmp_table_values_t));
       if (vt == NULL)
       {
-	ERROR ("snmp plugin: malloc failed.");
-	status = -1;
-	break;
+        ERROR ("snmp plugin: malloc failed.");
+        status = -1;
+        break;
       }
 
       vt->subid = vb->name[vb->name_length - 1];
       vt->value = csnmp_value_list_to_value (vb, ds->ds[i].type,
-	  data->scale, data->shift);
+          data->scale, data->shift);
       vt->next = NULL;
 
-      if (value_table_ptr[i] == NULL)
-	value_table[i] = vt;
+      if (value_list_tail[i] == NULL)
+        value_list_head[i] = vt;
       else
-	value_table_ptr[i]->next = vt;
-      value_table_ptr[i] = vt;
+        value_list_tail[i]->next = vt;
+      value_list_tail[i] = vt;
 
       /* Copy OID to oid_list[i + 1] */
       memcpy (oid_list[i].oid, vb->name, sizeof (oid) * vb->name_length);
@@ -1295,28 +1413,28 @@ static int csnmp_read_table (host_definition_t *host, data_definition_t *data)
   res = NULL;
 
   if (status == 0)
-    csnmp_dispatch_table (host, data, instance_list, value_table);
+    csnmp_dispatch_table (host, data, instance_list_head, value_list_head);
 
   /* Free all allocated variables here */
-  while (instance_list != NULL)
+  while (instance_list_head != NULL)
   {
-    instance_list_ptr = instance_list->next;
-    sfree (instance_list);
-    instance_list = instance_list_ptr;
+    csnmp_list_instances_t *next = instance_list_head->next;
+    sfree (instance_list_head);
+    instance_list_head = next;
   }
 
   for (i = 0; i < data->values_len; i++)
   {
-    csnmp_table_values_t *tmp;
-    while (value_table[i] != NULL)
+    while (value_list_head[i] != NULL)
     {
-      tmp = value_table[i]->next;
-      sfree (value_table[i]);
-      value_table[i] = tmp;
+      csnmp_table_values_t *next = value_list_head[i]->next;
+      sfree (value_list_head[i]);
+      value_list_head[i] = next;
     }
   }
 
-  sfree (value_table);
+  sfree (value_list_head);
+  sfree (value_list_tail);
   sfree (oid_list);
 
   return (0);
@@ -1353,7 +1471,7 @@ static int csnmp_read_value (host_definition_t *host, data_definition_t *data)
   if (ds->ds_num != data->values_len)
   {
     ERROR ("snmp plugin: DataSet `%s' requires %i values, but config talks about %i",
-	data->type, ds->ds_num, data->values_len);
+        data->type, ds->ds_num, data->values_len);
     return (-1);
   }
 
@@ -1396,7 +1514,7 @@ static int csnmp_read_value (host_definition_t *host, data_definition_t *data)
 
     snmp_sess_error (host->sess_handle, NULL, NULL, &errstr);
     ERROR ("snmp plugin: host %s: snmp_sess_synch_response failed: %s",
-	host->name, (errstr == NULL) ? "Unknown problem" : errstr);
+        host->name, (errstr == NULL) ? "Unknown problem" : errstr);
 
     if (res != NULL)
       snmp_free_pdu (res);
@@ -1414,15 +1532,15 @@ static int csnmp_read_value (host_definition_t *host, data_definition_t *data)
 #if COLLECT_DEBUG
     char buffer[1024];
     snprint_variable (buffer, sizeof (buffer),
-	vb->name, vb->name_length, vb);
+        vb->name, vb->name_length, vb);
     DEBUG ("snmp plugin: Got this variable: %s", buffer);
 #endif /* COLLECT_DEBUG */
 
     for (i = 0; i < data->values_len; i++)
       if (snmp_oid_compare (data->values[i].oid, data->values[i].oid_len,
-	    vb->name, vb->name_length) == 0)
-	vl.values[i] = csnmp_value_list_to_value (vb, ds->ds[i].type,
-	    data->scale, data->shift);
+            vb->name, vb->name_length) == 0)
+        vl.values[i] = csnmp_value_list_to_value (vb, ds->ds[i].type,
+            data->scale, data->shift);
   } /* for (res->variables) */
 
   if (res != NULL)
@@ -1480,8 +1598,8 @@ static int csnmp_read_host (user_data_t *ud)
   if ((uint32_t) (time_end - time_start) > host->interval)
   {
     WARNING ("snmp plugin: Host `%s' should be queried every %"PRIu32
-	" seconds, but reading all values takes %u seconds.",
-	host->name, host->interval, (unsigned int) (time_end - time_start));
+        " seconds, but reading all values takes %u seconds.",
+        host->name, host->interval, (unsigned int) (time_end - time_start));
   }
 
   if (success == 0)