snmp_agent: Implement SNMP Agent plugin. #2105

rkorynkx · 2016-12-14T10:50:35Z

The SNMP Agent plugin is an AgentX subagent that receives and handles queries from SNMP master agent and returns the data collected by read plugins. The SNMP Agent plugin handles requests only for OIDs specified in configuration file. To handle SNMP queries the plugin gets data from collectd and translates requested values from collectd's internal format to SNMP format. This plugin is a generic plugin and cannot work without configuration. For more details on AgentX subagent see http://www.net-snmp.org/tutorial/tutorial-5/toolkit/demon/

rubenk · 2016-12-14T11:26:56Z

Did you forget to check-in snmp.agent.c and snmp_agent.h?

rkorynkx · 2016-12-14T11:29:20Z

Hi @rubenk, snmp_agent.c is already there. Working on compilation fix.

rkorynkx · 2016-12-16T15:09:51Z

Hi @octo, buildbot/collectd-solaris10-i386 task is in pending state more than 2 days but in Details it shows that is has finished successfully. Could you please take a look? Should I perform any actions?

octo · 2016-12-16T16:34:44Z

@rkorynkx the build finished successfully but the status change didn't make it to GitHub. :-(

octo · 2016-12-16T17:02:57Z

src/snmp_agent.c

+#define CHECK_DD_TYPE(_dd, _p, _pi, _t, _ti)                                   \
+  (_dd->plugin ? !strcmp(_dd->plugin, _p) : 0) &&                              \
+      (_dd->plugin_instance ? !strcmp(_dd->plugin_instance, _pi) : 1) &&       \
+      (_dd->type ? !strcmp(_dd->type, _t) : 1) &&                              \


I would have expected the default to be 1 in the "type" case. If this is intentional, please add a comment.

octo

Hi @rkorynkx, thank you very much for this, I love the idea!

Unfortunately, my train ride comes to an end, so I'll publish the review comments I have to far and will come back to you later. Overall it looks pretty good, thank you!

Best regards,
—octo

octo · 2016-12-16T17:20:13Z

src/snmp_agent.c

+  snmp_agent_dump_data();
+
+  for (llentry_t *te = llist_head(g_agent->tables); te != NULL; te = te->next) {
+    table_definition_t *td = (table_definition_t *)te->value;


At your option: the explicit cast is not required when assigning a void * to a variable of different pointer type. Assignment without a cast may be easier to read.

octo · 2016-12-16T17:45:02Z

src/snmp_agent.c

@@ -0,0 +1,1513 @@
+/**


Could you please reformat this file with:

clang-format -style=LLVM

That will re-order includes. Be sure to add an empty line after "collectd.h" and, presumably, <net-snmp/net-snmp-config.h>.

octo · 2016-12-16T17:52:28Z

src/snmp_agent.c

+  DEBUG(PLUGIN_NAME ": Removed row for '%s' table [%d, %s].", td->name, *index,
+        ins);
+
+  notification_t n = {NOTIF_WARNING, cdtime(), "", "", PLUGIN_NAME, "", "", "",


This looks like a job for C99's Designated Initializers:

notification_t n = { .severity = NOTIF_WARNING, .time = cdtime(), .plugin = PLUGIN_NAME, };

octo · 2016-12-16T17:58:50Z

src/snmp_agent.c

+  sfree((*dd)->type_instance);
+  sfree((*dd)->oids);
+
+  sfree(*dd);


This leaks (*dd)->table.

Actually it's not. dd is not the owner of the table as it does not allocate it. It holds the reference to the table it belongs to. I will make it const for better understanding.

Thanks for the explanation, I didn't realize that this was a back reference. How about we rename data_definition_t to column_definition_t? That would make it more intuitive for me. Also it would then match the name of the "columns" field in table_definition_t.

data_definition_t is used not only for table data type, it's also used for scalar data type. And in this case columns won't be the proper name.

octo · 2016-12-16T18:02:25Z

src/snmp_agent.c

+    for (int i = 0; i < (*dd)->oids_len; i++)
+      unregister_mib((*dd)->oids[i].oid, (*dd)->oids[i].oid_len);
+  } else {
+    /* unregister all table OIDs */


Why don't you just call snmp_agent_free_table((*dd)->table) here?

snmp_agent_free_data() frees scalar and tabular data. If data is tabular it will remove all rows of table for specified column. For example, we have unicast and broadcast packets counters as columns in the table, so if to call this function with broadcast dd as a parameter it will free data for each interface for broadcast counter only. In turn, snmp_agent_free_table() iterates through all columns (dd) and calls snmp_agent_free_data().

octo · 2016-12-16T18:09:47Z

src/snmp_agent.c

+          DEBUG(PLUGIN_NAME ": Identifier '%s'", temp);
+
+          value_t *values;
+          size_t values_num;


Please initialize values and values_num to NULL / zero so that it's obvious that they are never used uninitialized.

octo · 2016-12-16T18:15:09Z

src/snmp_agent.c

+      int index = oid.oid[oid.oid_len - 1];
+      char *instance;
+
+      int ret = c_avl_get(td->index_instance, &index, (void **)&instance);


Compound literals are great for when you need to pass a pointer but don't actually need about the value that's assigned. By using an anonymous variable, you're signaling to the reader that you don't care what's assigned.

int ret = c_avl_get(td->index_instance, &index, &(void *){NULL});

octo · 2016-12-16T18:17:19Z

src/snmp_agent.c

+
+  snmp_agent_oid_to_string(temp, sizeof(temp), &oid);
+
+  DEBUG(PLUGIN_NAME ": Get request received for scalar OID '%s'.", temp);


As above: s/temp/oid_str/?

octo · 2016-12-16T18:19:07Z

src/snmp_agent.c

+
+
+static int
+snmp_agent_scalar_oid_handler(struct netsnmp_mib_handler_s *handler,


This function has a lot of duplicate code that's also in snmp_agent_table_oid_handler(). Is it possible to deduplicate this a bit?

octo · 2016-12-16T18:24:23Z

src/snmp_agent.c

+  return (0);
+}
+
+static int snmp_agent_config_data(oconfig_item_t *ci) {


This duplicates a lot of code from snmp_agent_config_table_data(). Could this be simplified?

rkorynkx · 2016-12-28T13:33:28Z

Hi @octo, I'm seeing Makefile.am has changed and currently there are conflicts. Should I rebase the branch with the latest master to solve this?

octo

Hi @rkorynkx,

thank you for following up on this! I have a few more comments, when they're addressed, it would be good if you could rebase on master to fix the merge conflicts. I'm afraid that Ruben's automake work conflicts with this :\ If you need any help resolving conflicts, please let me know. Once that's done, this should be good to go.

Best regards,
—octo

octo · 2016-12-30T10:13:09Z

src/snmp_agent.c

+  sfree((*dd)->type_instance);
+  sfree((*dd)->oids);
+
+  sfree(*dd);


Thanks for the explanation, I didn't realize that this was a back reference. How about we rename data_definition_t to column_definition_t? That would make it more intuitive for me. Also it would then match the name of the "columns" field in table_definition_t.

octo · 2016-12-30T10:24:50Z

src/snmp_agent.c

+static int snmp_agent_form_reply(struct netsnmp_request_info_s *requests,
+                                 data_definition_t *dd, char *instance,
+                                 int oid_index) {
+  char buf[DATA_MAX_NAME_LEN];


Could you please rename "buf" to "name"? For me, "buf" implies that it is a very short lived temporary variable, but it is actually read multiple times.

octo · 2016-12-30T10:26:26Z

src/snmp_agent.c

+  size_t values_num;
+  const data_set_t *ds = plugin_get_ds(dd->type);
+  if (ds == NULL) {
+    DEBUG(PLUGIN_NAME ": Data set not found for '%s' type", dd->type);


This is a configuration error that the user should be told about. Since you cannot continue, this should be an ERROR() instead.

octo · 2016-12-30T10:55:22Z

src/snmp_agent.c

+      return (-ENOMEM);
+    }
+  }
+


You should check that td.index_oid.oid_len != 0 here – it appears the implementation can't really function without it and the manpage implies that this is a required setting.

octo · 2016-12-30T11:00:09Z

src/snmp_agent.c

+        if (ret != 0)
+          continue;
+
+        if (!td->index_oid.oid_len) {


You can do this right after initializing td and avoid two nested loops that will never be able to perform any useful action.

rkorynkx · 2016-12-30T15:20:51Z

Hi @octo, thanks for quick review. I'll fix the review comments. Have one question, should I do rebase and merge conflicts as a separate commit after new patch (with fixed review comments) will be uploaded? What is the procedure?

Thanks,
Roman.

The SNMP Agent plugin is an AgentX subagent that receives and handles queries from SNMP master agent and returns the data collected by read plugins. The SNMP Agent plugin handles requests only for OIDs specified in configuration file. To handle SNMP queries the plugin gets data from collectd and translates requested values from collectd's internal format to SNMP format. This plugin is a generic plugin and cannot work without configuration. For more details on AgentX subagent see <http://www.net-snmp.org/tutorial/tutorial-5/toolkit/demon/> Change-Id: I0e82131685f2138e1af0a9283b06f85dd2028878 Signed-off-by: Korynkevych, RomanX <romanx.korynkevych@intel.com>

Change-Id: Ic57c14abb8b6b31547e97e6c394b8c3147b1e6b2 Signed-off-by: Korynkevych, RomanX <romanx.korynkevych@intel.com>

Change-Id: I697d80d39d8f37d9bd36280bad874a1f4d600518 Signed-off-by: Korynkevych, RomanX <romanx.korynkevych@intel.com>

Performed code cleanup and optimizations: - Reformat file using clang-format. - Removed duplicate code. Change-Id: Ia6d7cde0e44cb5d3639df11e712fe655becd9712 Signed-off-by: Korynkevych, RomanX <romanx.korynkevych@intel.com>

Additional validation and variable names changes. Change-Id: Ic6c3c3226e2ee6586c03b64d063822b99c242af3 Signed-off-by: Korynkevych, RomanX <romanx.korynkevych@intel.com>

The location of SNMP Agent sources in Makefile.am is `src/*` folder now. Signed-off-by: Roman Korynkevych <romanx.korynkevych@intel.com>

rkorynkx · 2017-01-05T13:00:42Z

Hi @octo, I've fixed the review comments, did the rebase and merged the conflicts. Could you please take a look?

Thanks,
Roman.

rkorynkx · 2017-01-17T09:37:27Z

Hi @octo, just a reminder that review comments have been fixed, waiting for your feedback.

Segmentation fault caused by c_avl_pick() occurs when NULL iterator is passed. Change-Id: I3dc960f80b556e18616717ca53fd992c8cf9d78b Signed-off-by: Korynkevych, RomanX <romanx.korynkevych@intel.com>

…ex is not specfified. Based on the SNMP rules when table column is represented as a key of string type, key should be used while generating OID for each row. When index is not specified, then it's assumed that instance value is the key for the specified table. Change-Id: I9f3fbd9aa018bc9037bd3202e2ed107a302fe1b3 Signed-off-by: Korynkevych, RomanX <romanx.korynkevych@intel.com>

maryamtahhan · 2017-04-18T13:20:03Z

Hi folks
would appreciate any additional feedback here

See also: collectd#2277

octo · 2017-05-18T07:09:32Z

Hi, sorry for the long delay, I lost sight of this :(

The code looks good now. I've done some minor cleanups (re-ran clang-format; removed parenthesis around return values, see #2277). I'll push those and merge once the CI build is complete.

Thanks again for all your work! This is a really awesome addition to collectd!
—octo

octo added the Pending contributor action label Dec 14, 2016

octo reviewed Dec 16, 2016

View reviewed changes

octo added the New plugin label Dec 19, 2016

octo reviewed Dec 30, 2016

View reviewed changes

rkorynkx added 5 commits January 3, 2017 14:55

snmp_agent: define MIN macro if not defined.

4fd753b

Change-Id: Ic57c14abb8b6b31547e97e6c394b8c3147b1e6b2 Signed-off-by: Korynkevych, RomanX <romanx.korynkevych@intel.com>

snmp_agent: removed references to snmp_agent.h file from Makefile.

8ad2f84

Change-Id: I697d80d39d8f37d9bd36280bad874a1f4d600518 Signed-off-by: Korynkevych, RomanX <romanx.korynkevych@intel.com>

snmp_agent: Addressing PR comments.

ad9a924

Performed code cleanup and optimizations: - Reformat file using clang-format. - Removed duplicate code. Change-Id: Ia6d7cde0e44cb5d3639df11e712fe655becd9712 Signed-off-by: Korynkevych, RomanX <romanx.korynkevych@intel.com>

snmp_agent: addressing PR comments.

8cb7522

Additional validation and variable names changes. Change-Id: Ic6c3c3226e2ee6586c03b64d063822b99c242af3 Signed-off-by: Korynkevych, RomanX <romanx.korynkevych@intel.com>

rkorynkx force-pushed the feat_snmp branch from 870dbe3 to 8cb7522 Compare January 3, 2017 14:57

snmp_agent: fix root Makefile.am

0b852c1

The location of SNMP Agent sources in Makefile.am is `src/*` folder now. Signed-off-by: Roman Korynkevych <romanx.korynkevych@intel.com>

rkorynkx added 2 commits January 27, 2017 14:15

snmp_agent: fix c_avl_pick() segmentation fault.

9e30a0c

Segmentation fault caused by c_avl_pick() occurs when NULL iterator is passed. Change-Id: I3dc960f80b556e18616717ca53fd992c8cf9d78b Signed-off-by: Korynkevych, RomanX <romanx.korynkevych@intel.com>

octo added 3 commits May 17, 2017 11:31

snmp_agent plugin: Format with clang-format.

7dde174

snmp_agent plugin: Remove parenthesis around return values.

be87504

See also: collectd#2277

snmp_agent plugin: Don't export the g_agent variable.

1634cdf

octo self-assigned this May 18, 2017

octo approved these changes May 18, 2017

View reviewed changes

octo merged commit 1634cdf into collectd:master May 18, 2017

rkorynkx deleted the feat_snmp branch May 18, 2017 09:11

octo added this to the 5.8 milestone Oct 11, 2017

snmp_agent: Implement SNMP Agent plugin. #2105

snmp_agent: Implement SNMP Agent plugin. #2105

rkorynkx commented Dec 14, 2016

rubenk commented Dec 14, 2016

rkorynkx commented Dec 14, 2016

rkorynkx commented Dec 16, 2016

octo commented Dec 16, 2016

octo Dec 16, 2016

octo left a comment

octo Dec 16, 2016

octo Dec 16, 2016

octo Dec 16, 2016

octo Dec 16, 2016

rkorynkx Dec 28, 2016

octo Dec 30, 2016

rkorynkx Dec 30, 2016

octo Dec 16, 2016

rkorynkx Dec 28, 2016

octo Dec 16, 2016

octo Dec 16, 2016

octo Dec 16, 2016

octo Dec 16, 2016

octo Dec 16, 2016

rkorynkx commented Dec 28, 2016

octo left a comment

octo Dec 30, 2016

octo Dec 30, 2016

octo Dec 30, 2016

octo Dec 30, 2016

octo Dec 30, 2016

rkorynkx commented Dec 30, 2016

rkorynkx commented Jan 5, 2017

rkorynkx commented Jan 17, 2017

maryamtahhan commented Apr 18, 2017

octo commented May 18, 2017


		snmp_agent_oid_to_string(temp, sizeof(temp), &oid);

		DEBUG(PLUGIN_NAME ": Get request received for scalar OID '%s'.", temp);



		static int
		snmp_agent_scalar_oid_handler(struct netsnmp_mib_handler_s *handler,

snmp_agent: Implement SNMP Agent plugin. #2105

snmp_agent: Implement SNMP Agent plugin. #2105

Conversation

rkorynkx commented Dec 14, 2016

rubenk commented Dec 14, 2016

rkorynkx commented Dec 14, 2016

rkorynkx commented Dec 16, 2016

octo commented Dec 16, 2016

Choose a reason for hiding this comment

octo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rkorynkx commented Dec 28, 2016

octo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rkorynkx commented Dec 30, 2016

rkorynkx commented Jan 5, 2017

rkorynkx commented Jan 17, 2017

maryamtahhan commented Apr 18, 2017

octo commented May 18, 2017