snmp plugin: Implemented new configuration options

Added new options `PluginInstance`, `TypeInstance`, `TypeInstanceOID` and `PluginInstanceOID`.
These allows flexible configuration of reported metrics.

Existing `Instance` option marked as deprecated.

Closes: #2636

diff --git a/src/collectd-snmp.pod b/src/collectd-snmp.pod
index ed041fa..1415cb1 100644 (file)
--- a/src/collectd-snmp.pod
+++ b/src/collectd-snmp.pod
@@ -10,23 +10,22 @@ collectd-snmp - Documentation of collectd's C<snmp plugin>
   # ...
   <Plugin snmp>
     <Data "powerplus_voltge_input">
-      Type "voltage"
       Table false
-      Instance "input_line1"
+      Type "voltage"
+      TypeInstance "input_line1"
       Scale 0.1
       Values "SNMPv2-SMI::enterprises.6050.5.4.1.1.2.1"
     </Data>
     <Data "hr_users">
-      Type "users"
       Table false
-      Instance ""
+      Type "users"
       Shift -1
       Values "HOST-RESOURCES-MIB::hrSystemNumUsers.0"
     </Data>
     <Data "std_traffic">
-      Type "if_octets"
       Table true
-      Instance "IF-MIB::ifDescr"
+      Type "if_octets"
+      TypeInstanceOID "IF-MIB::ifDescr"
       Values "IF-MIB::ifInOctets" "IF-MIB::ifOutOctets"
     </Data>
 
@@ -146,22 +145,59 @@ behavior.
 Use I<Plugin> as the plugin name of the values that are dispatched.
 Defaults to C<snmp>.
 
+=item B<PluginInstance> I<Instance>
+
+Sets the plugin-instance of the values that are dispatched to I<Instance> value.
+
+When B<Table> is set to I<true> and B<PluginInstanceOID> is set then this option
+has no effect.
+
+Defaults to an empty string.
+
+=item B<TypeInstance> I<Instance>
+
+Sets the type-instance of the values that are dispatched to I<Instance> value.
+
+When B<Table> is set to I<true> and B<TypeInstanceOID> is set then this option
+has no effect.
+
+Defaults to an empty string.
+
+=item B<TypeInstanceOID> I<OID>
+
+=item B<PluginInstanceOID> I<OID>
+
+If B<Table> is set to I<true>, I<OID> is interpreted as an SNMP-prefix that will
+return a list of values. Those values are then used as the actual type-instance
+or plugin-instance of dispatched metrics. An example would be the
+C<IF-MIB::ifDescr> subtree. L<variables(5)> from the SNMP distribution describes
+the format of OIDs. When set to empty string, then "SUBID" will be used as the
+instance.
+
+Prefix may be set for values with use of B<InstancePrefix> option.
+
+When B<Table> is set to I<false> these options has no effect.
+Only one of these options may be used in the same B<Data> block.
+
+Defaults:
+
+B<TypeInstanceOID> defaults to an empty string.
+
+B<PluginInstanceOID> is not configured.
+
 =item B<Instance> I<Instance>
 
-Sets the type-instance of the values that are dispatched. The meaning of this
-setting depends on whether B<Table> is set to I<true> or I<false>:
+Attention: this option exists for backwards compatibility only and will be
+removed in next major release. Please use B<TypeInstance> / B<TypeInstanceOID>
+instead.
 
-If B<Table> is set to I<true>, I<Instance> is interpreted as an SNMP-prefix
-that will return a list of values. Those values are then used as the actual
-type-instance. An example would be the C<IF-MIB::ifDescr> subtree.
-L<variables(5)> from the SNMP distribution describes the format of OIDs.
+The meaning of this setting depends on whether B<Table> is set to I<true> or
+I<false>.
 
-If B<Table> is set to I<true> and B<Instance> is omitted, then "SUBID" will be
-used as the instance.
+If B<Table> is set to I<true>, option behaves as B<TypeInstanceOID>.
+If B<Table> is set to I<false>, option behaves as B<TypeInstance>.
 
-If B<Table> is set to I<false> the actual string configured for I<Instance> is
-copied into the value-list. In this case I<Instance> may be empty, i.E<nbsp>e.
-"".
+Note what B<Table> option must be set before setting B<Instance>.
 
 =item B<InstancePrefix> I<String>
 

diff --git a/src/collectd.conf.in b/src/collectd.conf.in
index f23706d..ee89e94 100644 (file)
--- a/src/collectd.conf.in
+++ b/src/collectd.conf.in
@@ -1337,22 +1337,29 @@
 
 #<Plugin snmp>
 #   <Data "powerplus_voltge_input">
-#       Type "voltage"
 #       Table false
-#       Instance "input_line1"
+#       Type "voltage"
+#       TypeInstance "input_line1"
 #       Values "SNMPv2-SMI::enterprises.6050.5.4.1.1.2.1"
 #   </Data>
 #   <Data "hr_users">
-#       Type "users"
 #       Table false
-#       Instance ""
+#       Type "users"
+#       TypeInstance ""
 #       Values "HOST-RESOURCES-MIB::hrSystemNumUsers.0"
 #   </Data>
 #   <Data "std_traffic">
+#       Table true
 #       Type "if_octets"
+#       TypeInstanceOID "IF-MIB::ifDescr"
+#       #InstancePrefix "port"
+#       Values "IF-MIB::ifInOctets" "IF-MIB::ifOutOctets"
+#   </Data>
+#   <Data "interface_traffic">
 #       Table true
-#       #Plugin "interface"
-#       Instance "IF-MIB::ifDescr"
+#       Type "if_octets"
+#       Plugin "interface"
+#       PluginInstanceOID "IF-MIB::ifDescr"
 #       Values "IF-MIB::ifInOctets" "IF-MIB::ifOutOctets"
 #   </Data>
 #

diff --git a/src/snmp.c b/src/snmp.c
index db36a09..dde6079 100644 (file)
--- a/src/snmp.c
+++ b/src/snmp.c
@@ -44,11 +44,12 @@ struct oid_s {
 };
 typedef struct oid_s oid_t;
 
-union instance_u {
-  char string[DATA_MAX_NAME_LEN];
+struct instance_s {
+  bool configured;
+  bool is_plugin;
   oid_t oid;
 };
-typedef union instance_u instance_t;
+typedef struct instance_s instance_t;
 
 struct data_definition_s {
   char *name; /* used to reference this from the `Collect' option */
@@ -56,6 +57,8 @@ struct data_definition_s {
   bool is_table;
   instance_t instance;
   char *plugin_name;
+  char *plugin_instance;
+  char *type_instance;
   char *instance_prefix;
   oid_t *values;
   size_t values_len;
@@ -210,7 +213,6 @@ static void csnmp_host_definition_destroy(void *arg) /* {{{ */
  *  +-> call_snmp_init_once
  *  +-> csnmp_config_add_data
  *  !   +-> csnmp_config_add_data_instance
- *  !   +-> csnmp_config_add_data_instance_prefix
  *  !   +-> csnmp_config_add_data_values
  *  +-> csnmp_config_add_host
  *      +-> csnmp_config_add_host_version
@@ -227,8 +229,9 @@ static void call_snmp_init_once(void) {
   have_init = 1;
 } /* void call_snmp_init_once */
 
-static int csnmp_config_add_data_instance(data_definition_t *dd,
-                                          oconfig_item_t *ci) {
+static int csnmp_config_add_data_instance_oid(data_definition_t *dd,
+                                              oconfig_item_t *ci,
+                                              bool is_plugin) {
   char buffer[DATA_MAX_NAME_LEN];
   int status;
 
@@ -236,36 +239,28 @@ static int csnmp_config_add_data_instance(data_definition_t *dd,
   if (status != 0)
     return status;
 
-  if (dd->is_table) {
-    /* Instance is an OID */
-    dd->instance.oid.oid_len = MAX_OID_LEN;
-
-    if (!read_objid(buffer, dd->instance.oid.oid, &dd->instance.oid.oid_len)) {
-      ERROR("snmp plugin: read_objid (%s) failed.", buffer);
-      return -1;
-    }
-  } else {
-    /* Instance is a simple string */
-    sstrncpy(dd->instance.string, buffer, sizeof(dd->instance.string));
+  if (dd->instance.configured) {
+    ERROR("snmp plugin: Only one of options `TypeInstanceOID', "
+          "`PluginInstanceOID' or `Instance' can be used in `Data' block.");
+    return -1;
   }
 
-  return 0;
-} /* int csnmp_config_add_data_instance */
+  dd->instance.is_plugin = is_plugin;
+  dd->instance.configured = true;
 
-static int csnmp_config_add_data_instance_prefix(data_definition_t *dd,
-                                                 oconfig_item_t *ci) {
-  int status;
+  if (strlen(buffer) == 0) {
+    return 0;
+  }
+
+  dd->instance.oid.oid_len = MAX_OID_LEN;
 
-  if (!dd->is_table) {
-    WARNING("snmp plugin: data %s: InstancePrefix is ignored when `Table' "
-            "is set to `false'.",
-            dd->name);
+  if (!read_objid(buffer, dd->instance.oid.oid, &dd->instance.oid.oid_len)) {
+    ERROR("snmp plugin: read_objid (%s) failed.", buffer);
     return -1;
   }
 
-  status = cf_util_get_string(ci, &dd->instance_prefix);
-  return status;
-} /* int csnmp_config_add_data_instance_prefix */
+  return 0;
+} /* int csnmp_config_add_data_instance_oid */
 
 static int csnmp_config_add_data_values(data_definition_t *dd,
                                         oconfig_item_t *ci) {
@@ -345,8 +340,8 @@ static int csnmp_config_add_data(oconfig_item_t *ci) {
 
   dd->plugin_name = strdup("snmp");
   if (dd->plugin_name == NULL) {
-      ERROR("snmp plugin: Can't allocate memory");
-      return ENOMEM;
+    ERROR("snmp plugin: Can't allocate memory");
+    return ENOMEM;
   }
 
   for (int i = 0; i < ci->children_num; i++) {
@@ -358,10 +353,33 @@ static int csnmp_config_add_data(oconfig_item_t *ci) {
       status = cf_util_get_boolean(option, &dd->is_table);
     else if (strcasecmp("Plugin", option->key) == 0)
       status = cf_util_get_string(option, &dd->plugin_name);
-    else if (strcasecmp("Instance", option->key) == 0)
-      status = csnmp_config_add_data_instance(dd, option);
+    else if (strcasecmp("Instance", option->key) == 0) {
+      if (dd->is_table) {
+        /* Instance is OID */
+        WARNING("snmp plugin: Option `Instance' is deprecated, please update "
+                "Data \"%s\" block to use option `TypeInstanceOID'.",
+                dd->name);
+        status = csnmp_config_add_data_instance_oid(dd, option,
+                                                    false /* type instance */);
+      } else {
+        /* Instance is a simple string */
+        WARNING("snmp plugin: Option `Instance' is deprecated, please update "
+                "Data \"%s\" block to use option `TypeInstance'.",
+                dd->name);
+        status = cf_util_get_string(option, &dd->type_instance);
+      }
+    } else if (strcasecmp("PluginInstance", option->key) == 0)
+      status = cf_util_get_string(option, &dd->plugin_instance);
+    else if (strcasecmp("TypeInstance", option->key) == 0)
+      status = cf_util_get_string(option, &dd->type_instance);
+    else if (strcasecmp("PluginInstanceOID", option->key) == 0)
+      status = csnmp_config_add_data_instance_oid(dd, option,
+                                                  true /* plugin instance */);
+    else if (strcasecmp("TypeInstanceOID", option->key) == 0)
+      status = csnmp_config_add_data_instance_oid(dd, option,
+                                                  false /* type instance */);
     else if (strcasecmp("InstancePrefix", option->key) == 0)
-      status = csnmp_config_add_data_instance_prefix(dd, option);
+      status = cf_util_get_string(option, &dd->instance_prefix);
     else if (strcasecmp("Values", option->key) == 0)
       status = csnmp_config_add_data_values(dd, option);
     else if (strcasecmp("Shift", option->key) == 0)
@@ -382,6 +400,37 @@ static int csnmp_config_add_data(oconfig_item_t *ci) {
   } /* for (ci->children) */
 
   while (status == 0) {
+    if (dd->is_table) {
+      if (dd->plugin_instance && dd->instance.is_plugin) {
+        WARNING("snmp plugin: Option `PluginInstance' will be ignored for "
+                "Data `%s'",
+                dd->name);
+      }
+      if (dd->type_instance && !dd->instance.is_plugin) {
+        WARNING("snmp plugin: Option `TypeInstance' will be ignored for Data "
+                "`%s'",
+                dd->name);
+      }
+    } else {
+      if (dd->instance.configured) {
+        if (dd->instance.is_plugin) {
+          WARNING("snmp plugin: Option `PluginInstanceOID' will be ignored for "
+                  "Data `%s'",
+                  dd->name);
+        } else {
+          WARNING("snmp plugin: Option `TypeInstanceOID' will be ignored for "
+                  "Data `%s'",
+                  dd->name);
+        }
+      }
+
+      if (dd->instance_prefix) {
+        WARNING("snmp plugin: data %s: InstancePrefix is ignored when `Table' "
+                "is set to `false'.",
+                dd->name);
+      }
+    }
+
     if (dd->type == NULL) {
       WARNING("snmp plugin: `Type' not given for data `%s'", dd->name);
       status = -1;
@@ -400,6 +449,8 @@ static int csnmp_config_add_data(oconfig_item_t *ci) {
     sfree(dd->name);
     sfree(dd->type);
     sfree(dd->plugin_name);
+    sfree(dd->plugin_instance);
+    sfree(dd->type_instance);
     sfree(dd->instance_prefix);
     sfree(dd->values);
     sfree(dd->ignores);
@@ -408,9 +459,16 @@ static int csnmp_config_add_data(oconfig_item_t *ci) {
   }
 
   DEBUG("snmp plugin: dd = { name = %s, type = %s, is_table = %s, values_len = "
-        "%" PRIsz " }",
+        "%" PRIsz ",",
         dd->name, dd->type, (dd->is_table) ? "true" : "false", dd->values_len);
 
+  DEBUG("snmp plugin:        plugin_instance = %s, type_instance = %s,",
+        dd->plugin_instance, dd->type_instance);
+
+  DEBUG("snmp plugin:        instance_by_oid = %s, to_plugin_instance = %s }",
+        (dd->instance.oid.oid_len > 0) ? "true" : "SUBID",
+        (dd->instance.is_plugin) ? "true" : "false");
+
   if (data_head == NULL)
     data_head = dd;
   else {
@@ -1229,11 +1287,27 @@ static int csnmp_dispatch_table(host_definition_t *host,
       else
         sstrncpy(temp, instance_list_ptr->instance, sizeof(temp));
 
-      if (data->instance_prefix == NULL)
-        sstrncpy(vl.type_instance, temp, sizeof(vl.type_instance));
-      else
-        snprintf(vl.type_instance, sizeof(vl.type_instance), "%s%s",
-                 data->instance_prefix, temp);
+      if (data->instance.is_plugin) {
+        if (data->instance_prefix == NULL)
+          sstrncpy(vl.plugin_instance, temp, sizeof(vl.plugin_instance));
+        else
+          snprintf(vl.plugin_instance, sizeof(vl.plugin_instance), "%s%s",
+                   data->instance_prefix, temp);
+
+        if (data->type_instance)
+          sstrncpy(vl.type_instance, data->type_instance,
+                   sizeof(vl.type_instance));
+      } else {
+        if (data->instance_prefix == NULL)
+          sstrncpy(vl.type_instance, temp, sizeof(vl.type_instance));
+        else
+          snprintf(vl.type_instance, sizeof(vl.type_instance), "%s%s",
+                   data->instance_prefix, temp);
+
+        if (data->plugin_instance)
+          sstrncpy(vl.plugin_instance, data->plugin_instance,
+                   sizeof(vl.plugin_instance));
+      }
     }
 
     vl.values_len = data->values_len;
@@ -1243,12 +1317,7 @@ static int csnmp_dispatch_table(host_definition_t *host,
     for (i = 0; i < data->values_len; i++)
       vl.values[i] = value_table_ptr[i]->value;
 
-    /* If we get here `vl.type_instance' and all `vl.values' have been set
-     * vl.type_instance can be empty, i.e. a blank port description on a
-     * switch if you're using IF-MIB::ifDescr as Instance.
-     */
-    if (vl.type_instance[0] != '\0')
-      plugin_dispatch_values(&vl);
+    plugin_dispatch_values(&vl);
 
     /* prevent leakage of pointer to local variable. */
     vl.values_len = 0;
@@ -1609,7 +1678,11 @@ static int csnmp_read_value(host_definition_t *host, data_definition_t *data) {
   sstrncpy(vl.host, host->name, sizeof(vl.host));
   sstrncpy(vl.plugin, data->plugin_name, sizeof(vl.plugin));
   sstrncpy(vl.type, data->type, sizeof(vl.type));
-  sstrncpy(vl.type_instance, data->instance.string, sizeof(vl.type_instance));
+  if (data->type_instance)
+    sstrncpy(vl.type_instance, data->type_instance, sizeof(vl.type_instance));
+  if (data->plugin_instance)
+    sstrncpy(vl.plugin_instance, data->plugin_instance,
+             sizeof(vl.plugin_instance));
 
   vl.interval = host->interval;
 
@@ -1724,6 +1797,8 @@ static int csnmp_shutdown(void) {
     sfree(data_this->name);
     sfree(data_this->type);
     sfree(data_this->plugin_name);
+    sfree(data_this->plugin_instance);
+    sfree(data_this->type_instance);
     sfree(data_this->instance_prefix);
     sfree(data_this->values);
     sfree(data_this->ignores);