Print this page
NEX-18695 libfmd_snmp is unable to handle multiple OIDs in GET request
Reviewed by: Joyce McIntosh <joyce.mcintosh@nexenta.com>
Reviewed by: Cynthia Eastham <cynthia.eastham@nexenta.com>
Reviewed by: Rick McNeal <rick.mcneal@nexenta.com>
NEX-17777 snmptable produces bogus output for FM tables
Reviewed by: Cynthia Eastham <cynthia.eastham@nexenta.com>
Reviewed by: Evan Layton <evan.layton@nexenta.com>
NEX-3125 libfmd_snmp should compile with newer net-snmp
Reviewed by: Gordon Ross <gwr@nexenta.com>
Reviewed by: Jean McCormack <jean.mccormack@nexenta.com>
Reviewed by: Josef 'Jeff' Sipek <josef.sipek@nexenta.com>

@@ -22,19 +22,26 @@
 /*
  * Copyright 2009 Sun Microsystems, Inc.  All rights reserved.
  * Use is subject to license terms.
  */
 
+/*
+ * Copyright 2018 Nexenta Systems, Inc.
+ */
+
 #include <fm/fmd_adm.h>
 #include <fm/fmd_snmp.h>
+
 #include <net-snmp/net-snmp-config.h>
 #include <net-snmp/net-snmp-includes.h>
 #include <net-snmp/agent/net-snmp-agent-includes.h>
-#include <pthread.h>
-#include <stddef.h>
+
 #include <errno.h>
 #include <libuutil.h>
+#include <pthread.h>
+#include <stddef.h>
+
 #include "sunFM_impl.h"
 #include "resource.h"
 
 static uu_avl_pool_t    *rsrc_fmri_avl_pool;
 static uu_avl_pool_t    *rsrc_index_avl_pool;

@@ -43,12 +50,10 @@
 
 #define VALID_AVL_STATE (rsrc_fmri_avl_pool != NULL &&          \
         rsrc_index_avl_pool != NULL && rsrc_fmri_avl != NULL && \
         rsrc_index_avl != NULL)
 
-#define UPDATE_WAIT_MILLIS      10      /* poll interval in milliseconds */
-
 /*
  * Update types: single-index and all are mutually exclusive; a count
  * update is optional.
  */
 #define UCT_INDEX       0x1

@@ -56,19 +61,14 @@
 #define UCT_COUNT       0x4
 #define UCT_FLAGS       0x7
 
 #define RESOURCE_DATA_VALID(d)  ((d)->d_valid == valid_stamp)
 
-/*
- * Locking strategy is described in module.c.
- */
 static ulong_t          max_index;
 static int              valid_stamp;
 static uint32_t         rsrc_count;
 static pthread_mutex_t  update_lock;
-static pthread_cond_t   update_cv;
-static volatile enum { US_QUIET, US_NEEDED, US_INPROGRESS } update_status;
 
 static Netsnmp_Node_Handler     sunFmResourceTable_handler;
 static Netsnmp_Node_Handler     sunFmResourceCount_handler;
 
 static sunFmResource_data_t *

@@ -263,54 +263,33 @@
         }
 
         return (SNMP_ERR_NOERROR);
 }
 
-/*ARGSUSED*/
 static void
-update_thread(void *arg)
+request_update(void)
 {
+        sunFmResource_update_ctx_t      uc;
+
         /*
          * The current rsrcinfo_update implementation offers minimal savings
          * for the use of index-only updates; therefore we always do a full
          * update.  If it becomes advantageous to limit updates to a single
          * index, the contexts can be queued by the handler instead.
          */
-        sunFmResource_update_ctx_t      uc;
 
         uc.uc_host = NULL;
         uc.uc_prog = FMD_ADM_PROGRAM;
         uc.uc_version = FMD_ADM_VERSION;
 
         uc.uc_all = 0;
         uc.uc_index = 0;
         uc.uc_type = UCT_ALL;
 
-        for (;;) {
-                (void) pthread_mutex_lock(&update_lock);
-                update_status = US_QUIET;
-                while (update_status == US_QUIET)
-                        (void) pthread_cond_wait(&update_cv, &update_lock);
-                update_status = US_INPROGRESS;
-                (void) pthread_mutex_unlock(&update_lock);
                 (void) rsrcinfo_update(&uc);
-        }
 }
 
-static void
-request_update(void)
-{
-        (void) pthread_mutex_lock(&update_lock);
-        if (update_status != US_QUIET) {
-                (void) pthread_mutex_unlock(&update_lock);
-                return;
-        }
-        update_status = US_NEEDED;
-        (void) pthread_cond_signal(&update_cv);
-        (void) pthread_mutex_unlock(&update_lock);
-}
-
 /*ARGSUSED*/
 static int
 resource_compare_fmri(const void *l, const void *r, void *private)
 {
         sunFmResource_data_t    *l_data = (sunFmResource_data_t *)l;

@@ -346,23 +325,11 @@
         if ((err = pthread_mutex_init(&update_lock, NULL)) != 0) {
                 (void) snmp_log(LOG_ERR, MODNAME_STR ": mutex_init failure: "
                     "%s\n", strerror(err));
                 return (MIB_REGISTRATION_FAILED);
         }
-        if ((err = pthread_cond_init(&update_cv, NULL)) != 0) {
-                (void) snmp_log(LOG_ERR, MODNAME_STR ": cond_init failure: "
-                    "%s\n", strerror(err));
-                return (MIB_REGISTRATION_FAILED);
-        }
 
-        if ((err = pthread_create(NULL, NULL, (void *(*)(void *))update_thread,
-            NULL)) != 0) {
-                (void) snmp_log(LOG_ERR, MODNAME_STR ": error creating update "
-                    "thread: %s\n", strerror(err));
-                return (MIB_REGISTRATION_FAILED);
-        }
-
         if ((table_info =
             SNMP_MALLOC_TYPEDEF(netsnmp_table_registration_info)) == NULL)
                 return (MIB_REGISTRATION_FAILED);
 
         if ((handler = netsnmp_create_handler_registration("sunFmResourceTable",

@@ -459,16 +426,16 @@
 
         return (MIB_REGISTERED_OK);
 }
 
 /*
- * These two functions form the core of GET/GETNEXT/GETBULK handling (the
+ * These two functions form the core of GET/GETNEXT handling (the
  * only kind we do).  They perform two functions:
  *
  * - First, frob the request to set all the index variables to correspond
  *   to the value that's going to be returned.  For GET, this is a nop;
- *   for GETNEXT/GETBULK it always requires some work.
+ *   for GETNEXT it always requires some work.
  * - Second, find and return the fmd resource information corresponding to
  *   the (possibly updated) indices.
  *
  * These should be as fast as possible; they run in the agent thread.
  */

@@ -557,42 +524,33 @@
 
         return (resource_lookup_index_exact(table_info->index_oid[0]));
 }
 
 /*ARGSUSED*/
-static void
-sunFmResourceTable_return(unsigned int reg, void *arg)
+static int
+sunFmResourceTable_handler(netsnmp_mib_handler *handler,
+    netsnmp_handler_registration *reginfo, netsnmp_agent_request_info *reqinfo,
+    netsnmp_request_info *request)
 {
-        netsnmp_delegated_cache         *cache = (netsnmp_delegated_cache *)arg;
-        netsnmp_request_info            *request;
-        netsnmp_agent_request_info      *reqinfo;
-        netsnmp_handler_registration    *reginfo;
         netsnmp_table_request_info      *table_info;
         sunFmResource_data_t            *data;
         ulong_t                         rsrcstate;
+        int                             ret = SNMP_ERR_NOERROR;
 
-        ASSERT(netsnmp_handler_check_cache(cache) != NULL);
+        /*
+         * We don't support MODE_GETBULK directly, so all bulk requests should
+         * come through bulk_to_next helper.  Make sure it stays that way.
+         */
+        ASSERT(reqinfo->mode == MODE_GET || reqinfo->mode == MODE_GETNEXT);
 
         (void) pthread_mutex_lock(&update_lock);
-        if (update_status != US_QUIET) {
-                struct timeval                  tv;
+        request_update();
 
-                tv.tv_sec = UPDATE_WAIT_MILLIS / 1000;
-                tv.tv_usec = (UPDATE_WAIT_MILLIS % 1000) * 1000;
-
-                (void) snmp_alarm_register_hr(tv, 0, sunFmResourceTable_return,
-                    cache);
-                (void) pthread_mutex_unlock(&update_lock);
-                return;
-        }
-
-        request = cache->requests;
-        reqinfo = cache->reqinfo;
-        reginfo = cache->reginfo;
-
+        for (; request != NULL; request = request->next) {
         table_info = netsnmp_extract_table_info(request);
-        request->delegated = 0;
+                if (table_info == NULL)
+                        continue;
 
         ASSERT(table_info->colnum >= SUNFMRESOURCE_COLMIN);
         ASSERT(table_info->colnum <= SUNFMRESOURCE_COLMAX);
 
         /*

@@ -607,41 +565,33 @@
          * - We will never receive requests outside our table nor
          *   those with the first subid anything other than 1 (Entry)
          *   nor those without a column number.  This is true even
          *   for GETNEXT requests.
          */
-
         switch (reqinfo->mode) {
         case MODE_GET:
-                if ((data = sunFmResourceTable_rsrc(reginfo, table_info)) ==
-                    NULL) {
-                        netsnmp_free_delegated_cache(cache);
-                        (void) pthread_mutex_unlock(&update_lock);
-                        return;
-                }
+                        data = sunFmResourceTable_rsrc(reginfo, table_info);
+                        if (data == NULL)
+                                goto out;
                 break;
         case MODE_GETNEXT:
-        case MODE_GETBULK:
-                if ((data = sunFmResourceTable_nextrsrc(reginfo, table_info)) ==
-                    NULL) {
-                        netsnmp_free_delegated_cache(cache);
-                        (void) pthread_mutex_unlock(&update_lock);
-                        return;
-                }
+                        data = sunFmResourceTable_nextrsrc(reginfo, table_info);
+                        if (data == NULL)
+                                goto out;
                 break;
         default:
-                (void) snmp_log(LOG_ERR, MODNAME_STR ": Unsupported request "
-                    "mode %d\n", reqinfo->mode);
-                netsnmp_free_delegated_cache(cache);
-                (void) pthread_mutex_unlock(&update_lock);
-                return;
+                        (void) snmp_log(LOG_ERR, MODNAME_STR
+                            ": unsupported request mode %d\n", reqinfo->mode);
+                        ret = SNMP_ERR_GENERR;
+                        goto out;
         }
 
         switch (table_info->colnum) {
         case SUNFMRESOURCE_COL_FMRI:
-                (void) netsnmp_table_build_result(reginfo, request, table_info,
-                    ASN_OCTET_STR, (uchar_t *)data->d_ari_fmri,
+                        (void) netsnmp_table_build_result(reginfo, request,
+                            table_info, ASN_OCTET_STR,
+                            (uchar_t *)data->d_ari_fmri,
                     strlen(data->d_ari_fmri));
                 break;
         case SUNFMRESOURCE_COL_STATUS:
                 switch (data->d_ari_flags &
                     (FMD_ADM_RSRC_FAULTY|FMD_ADM_RSRC_UNUSABLE)) {

@@ -656,136 +606,76 @@
                         break;
                 case FMD_ADM_RSRC_FAULTY | FMD_ADM_RSRC_UNUSABLE:
                         rsrcstate = SUNFMRESOURCE_STATE_FAULTED;
                         break;
                 }
-                (void) netsnmp_table_build_result(reginfo, request, table_info,
-                    ASN_INTEGER, (uchar_t *)&rsrcstate,
+                        (void) netsnmp_table_build_result(reginfo, request,
+                            table_info, ASN_INTEGER, (uchar_t *)&rsrcstate,
                     sizeof (rsrcstate));
                 break;
         case SUNFMRESOURCE_COL_DIAGNOSISUUID:
-                (void) netsnmp_table_build_result(reginfo, request, table_info,
-                    ASN_OCTET_STR, (uchar_t *)data->d_ari_case,
+                        (void) netsnmp_table_build_result(reginfo, request,
+                            table_info, ASN_OCTET_STR,
+                            (uchar_t *)data->d_ari_case,
                     strlen(data->d_ari_case));
                 break;
         default:
                 break;
         }
-        netsnmp_free_delegated_cache(cache);
+        }
+
+out:
         (void) pthread_mutex_unlock(&update_lock);
+        return (ret);
 }
 
+/*ARGSUSED*/
 static int
-sunFmResourceTable_handler(netsnmp_mib_handler *handler,
+sunFmResourceCount_handler(netsnmp_mib_handler *handler,
     netsnmp_handler_registration *reginfo, netsnmp_agent_request_info *reqinfo,
-    netsnmp_request_info *requests)
+    netsnmp_request_info *request)
 {
-        netsnmp_request_info            *request;
-        struct timeval                  tv;
-
-        tv.tv_sec = UPDATE_WAIT_MILLIS / 1000;
-        tv.tv_usec = (UPDATE_WAIT_MILLIS % 1000) * 1000;
-
-        request_update();
-
-        for (request = requests; request; request = request->next) {
-                if (request->processed != 0)
-                        continue;
-
-                if (netsnmp_extract_table_info(request) == NULL)
-                        continue;
-
-                request->delegated = 1;
-                (void) snmp_alarm_register_hr(tv, 0, sunFmResourceTable_return,
-                    (void *) netsnmp_create_delegated_cache(handler, reginfo,
-                    reqinfo, request, NULL));
-        }
-
-        return (SNMP_ERR_NOERROR);
-}
-
-/*ARGSUSED*/
-static void
-sunFmResourceCount_return(unsigned int reg, void *arg)
-{
-        netsnmp_delegated_cache         *cache = (netsnmp_delegated_cache *)arg;
-        netsnmp_request_info            *request;
-        netsnmp_agent_request_info      *reqinfo;
         ulong_t                         rsrc_count_long;
+        int                             ret = SNMP_ERR_NOERROR;
 
-        ASSERT(netsnmp_handler_check_cache(cache) != NULL);
+        /*
+         * We don't support MODE_GETBULK directly, so all bulk requests should
+         * come through bulk_to_next helper.  Make sure it stays that way.
+         */
+        ASSERT(reqinfo->mode == MODE_GET || reqinfo->mode == MODE_GETNEXT);
 
         (void) pthread_mutex_lock(&update_lock);
-        if (update_status != US_QUIET) {
-                struct timeval  tv;
+        request_update();
 
-                tv.tv_sec = UPDATE_WAIT_MILLIS / 1000;
-                tv.tv_usec = (UPDATE_WAIT_MILLIS % 1000) * 1000;
-
-                (void) snmp_alarm_register_hr(tv, 0, sunFmResourceCount_return,
-                    cache);
-                (void) pthread_mutex_unlock(&update_lock);
-                return;
-        }
-
-        request = cache->requests;
-        reqinfo = cache->reqinfo;
-
-        request->delegated = 0;
-
+        for (; request != NULL; request = request->next) {
         switch (reqinfo->mode) {
         /*
-         * According to the documentation, it's not possible for us ever to
-         * be called with MODE_GETNEXT.  However, Net-SNMP does the following:
+                 * According to the documentation, it's not possible for us ever
+                 * to be called with MODE_GETNEXT.  However, Net-SNMP does the
+                 * following:
          * - set reqinfo->mode to MODE_GET
          * - invoke the handler
-         * - set reqinfo->mode to MODE_GETNEXT (even if the request was not
-         *   actually processed; i.e. it's been delegated)
+                 * - set reqinfo->mode to MODE_GETNEXT (even if the request was
+                 *   not actually processed; i.e. it's been delegated)
          * Since we're called back later with the same reqinfo, we see
          * GETNEXT.  Therefore this case is needed to work around the
          * Net-SNMP bug.
          */
         case MODE_GET:
         case MODE_GETNEXT:
-                DEBUGMSGTL((MODNAME_STR, "resource count is %u\n", rsrc_count));
+                        DEBUGMSGTL((MODNAME_STR, "resource count is %u\n",
+                            rsrc_count));
                 rsrc_count_long = (ulong_t)rsrc_count;
-                (void) snmp_set_var_typed_value(request->requestvb, ASN_GAUGE,
-                    (uchar_t *)&rsrc_count_long, sizeof (rsrc_count_long));
+                        (void) snmp_set_var_typed_value(request->requestvb,
+                            ASN_GAUGE, (uchar_t *)&rsrc_count_long,
+                            sizeof (rsrc_count_long));
                 break;
         default:
-                (void) snmp_log(LOG_ERR, MODNAME_STR ": Unsupported request "
-                    "mode %d\n", reqinfo->mode);
+                        (void) snmp_log(LOG_ERR, MODNAME_STR
+                            ": unsupported request mode: %d\n", reqinfo->mode);
+                        ret = SNMP_ERR_GENERR;
         }
+        }
 
-        netsnmp_free_delegated_cache(cache);
         (void) pthread_mutex_unlock(&update_lock);
-}
-
-static int
-sunFmResourceCount_handler(netsnmp_mib_handler *handler,
-    netsnmp_handler_registration *reginfo, netsnmp_agent_request_info *reqinfo,
-    netsnmp_request_info *requests)
-{
-        struct timeval  tv;
-
-        tv.tv_sec = UPDATE_WAIT_MILLIS / 1000;
-        tv.tv_usec = (UPDATE_WAIT_MILLIS % 1000) * 1000;
-
-        request_update();
-
-        /*
-         * We are never called for a GETNEXT when registered as an
-         * instance; it's handled for us and converted to a GET.
-         * Also, an instance handler is given only one request at a time, so
-         * we don't need to loop over a list of requests.
-         */
-
-        if (requests->processed != 0)
-                return (SNMP_ERR_NOERROR);
-
-        requests->delegated = 1;
-        (void) snmp_alarm_register_hr(tv, 0, sunFmResourceCount_return,
-            (void *) netsnmp_create_delegated_cache(handler, reginfo,
-            reqinfo, requests, NULL));
-
-        return (SNMP_ERR_NOERROR);
+        return (ret);
 }