SNMP: Bugfixes, reconfiguration changes, cleanup

Major rework of reconfiguration handling.

proto/snmp/bgp4_mib.c

@@ -829,15 +829,14 @@ snmp_bgp4_show_info(struct snmp_proto *p)
 /*
  * snmp_bgp4_start - prepare BGP4-MIB
  * @p: SNMP protocol instance holding memory pool
+ * @with_mib_tree: flag choosing to insert BGP4-MIB into MIB tree
  *
  * This function create all runtime bindings to BGP procotol structures.
  * It is gruaranteed that the BGP protocols exist.
  */
 void
-snmp_bgp4_start(struct snmp_proto *p)
+snmp_bgp4_start(struct snmp_proto *p, int with_mib_tree)
 {
-  agentx_available_mibs[BGP4_MIB_ID] = (struct oid *) &bgp4_mib_oid;
-
   struct snmp_config *cf = SKIP_BACK(struct snmp_config, cf, p->p.cf);
 
   /* Create binding to BGP protocols */
@@ -862,6 +861,9 @@ snmp_bgp4_start(struct snmp_proto *p)
     snmp_hash_add_peer(p, peer);
   }
 
+  if (!with_mib_tree)
+    return;
+
   const STATIC_OID(4) bgp4_mib_peer_entry = STATIC_OID_INITIALIZER(4, SNMP_MGMT,
     /* ids */ SNMP_MIB_2, SNMP_BGP4_MIB, BGP4_MIB_PEER_TABLE, BGP4_MIB_PEER_ENTRY);
 

proto/snmp/bgp4_mib.h

@@ -48,7 +48,7 @@ enum bgp4_admin_status {
   BGP4_ADMIN_START = 2,
 };
 
-void snmp_bgp4_start(struct snmp_proto *p);
+void snmp_bgp4_start(struct snmp_proto *p, int with_mib);
 void snmp_bgp4_register(struct snmp_proto *p);
 void snmp_bgp4_show_info(struct snmp_proto *p);
 

proto/snmp/mib_tree.c

@@ -262,7 +262,8 @@ int
 mib_tree_delete(struct mib_tree *t, struct mib_walk_state *walk)
 {
   int deleted = 0;
-  ASSUME(t);
+  if (!t)
+    return 0;
 
   /* (walk->stack_pos < 2) It is impossible to delete root node */
   if (!walk || walk->stack_pos == 0)

proto/snmp/snmp.c

@@ -129,45 +129,6 @@ static const char *snmp_state_str[] = {
   [SNMP_DOWN]	  = "protocol down",
 };
 
-/*
- * agentx_get_mib_init - init function for agentx_get_mib()
- * @p: SNMP instance protocol pool
- */
-void agentx_get_mib_init(pool *p)
-{
-  const struct oid *src = agentx_available_mibs[AGENTX_MIB_COUNT - 1];
-  size_t size = snmp_oid_size(src);
-  struct oid *dest = mb_alloc(p, size);
-
-  memcpy(dest, src, size);
-  u8 ids = src->n_subid;
-
-  if (ids > 0)
-    dest->ids[ids - 1] = src->ids[ids - 1] + 1;
-
-  agentx_available_mibs[AGENTX_MIB_COUNT] = dest;
-}
-
-/*
- * agentx_get_mib - classify an OID based on MIB prefix
- * @o: Object Identifier to classify
- */
-enum agentx_mibs agentx_get_mib(const struct oid *o)
-{
-  /* TODO: move me into MIB tree as hooks/MIB module root */
-  enum agentx_mibs mib = AGENTX_MIB_UNKNOWN;
-  for (uint i = 0; i < AGENTX_MIB_COUNT + 1; i++)
-  {
-    ASSERT(agentx_available_mibs[i]);
-    if (snmp_oid_compare(o, agentx_available_mibs[i]) < 0)
-      return mib;
-    mib = (enum agentx_mibs) i;
-  }
-
-  return AGENTX_MIB_UNKNOWN;
-}
-
-
 /*
  * snmp_rx_skip - skip all received data
  * @sk: communication socket
@@ -392,10 +353,16 @@ snmp_cleanup(struct snmp_proto *p)
   }
 
   HASH_FREE(p->bgp_hash);
-
   rfree(p->lp);
+  p->lp = NULL;
+  /* bgp_trie is allocated exclusively from linpool lp */
   p->bgp_trie = NULL;
 
+  struct mib_walk_state *walk = tmp_alloc(sizeof(struct mib_walk_state));
+  mib_tree_walk_init(walk, p->mib_tree);
+  (void) mib_tree_delete(p->mib_tree, walk);
+  p->mib_tree = NULL;
+
   p->state = SNMP_DOWN;
 }
 
@@ -548,8 +515,8 @@ snmp_start(struct proto *P)
 
   p->pool = p->p.pool;
   p->lp = lp_new(p->pool);
-  p->mib_tree = mb_alloc(p->pool, sizeof(struct mib_tree));
   p->bgp_trie = f_new_trie(p->lp, 0);
+  p->mib_tree = mb_alloc(p->pool, sizeof(struct mib_tree));
 
   p->startup_timer = tm_new_init(p->pool, snmp_startup_timeout, p, 0, 0);
   p->ping_timer = tm_new_init(p->pool, snmp_ping_timeout, p, p->timeout, 0);
@@ -560,61 +527,42 @@ snmp_start(struct proto *P)
   HASH_INIT(p->bgp_hash, p->pool, 10);
 
   mib_tree_init(p->pool, p->mib_tree);
-  snmp_bgp4_start(p);
-  agentx_get_mib_init(p->pool);
+  snmp_bgp4_start(p, 1);
 
   return snmp_set_state(p, SNMP_INIT);
 }
 
+/*
+ * snmp_reconfigure_logic - find changes in configuration
+ * @p: SNMP protocol instance
+ * @new: new SNMP protocol configuration
+ *
+ * Return 1 if only minor changes have occured, 0 if we need full down-up cycle.
+ */
 static inline int
 snmp_reconfigure_logic(struct snmp_proto *p, const struct snmp_config *new)
 {
   const struct snmp_config *old = SKIP_BACK(struct snmp_config, cf, p->p.cf);
 
-  if (old->bonds != new->bonds)
+  if ((old->trans_type != SNMP_TRANS_TCP) && (new->trans_type == SNMP_TRANS_TCP)
+    || (old->trans_type == SNMP_TRANS_TCP) && (new->trans_type != SNMP_TRANS_TCP))
     return 0;
 
-  uint bonds = old->bonds;
-  struct snmp_bond *b1, *b2;
-  WALK_LIST(b1, new->bgp_entries)
-  {
-    WALK_LIST(b2, old->bgp_entries)
-    {
-      if (!bstrcmp(b1->config->name, b2->config->name))
-	goto skip;
-    }
-
-    return 0;
-skip:
-    bonds--;
-  }
-
-  if (bonds != 0)
+  if (old->trans_type == SNMP_TRANS_TCP &&
+      (ipa_compare(old->remote_ip, new->remote_ip)
+      || old->remote_port != new->remote_port))
     return 0;
 
-  if (old->trans_type != new->trans_type
-      || ip4_compare(old->local_ip, new->local_ip)
-      || old->local_port != new->local_port
-      || ipa_compare(old->remote_ip, new->remote_ip)
-      || !bstrcmp(old->remote_path, new->remote_path)
-      || old->remote_port != new->remote_port
-	  // TODO can be changed on the fly
-      || !ip4_compare(old->bgp_local_id, new->bgp_local_id)
-      || old->bgp_local_as != new->bgp_local_as // TODO can be changed on the fly
-      || old->timeout != new->timeout
-    //|| old->startup_delay != new->startup_delay
+  if (old->trans_type != SNMP_TRANS_TCP &&
+      bstrcmp(old->remote_path, new->remote_path))
+    return 0;
+
+  return !(ip4_compare(old->bgp_local_id, new->bgp_local_id)
+      || old->bgp_local_as != new->bgp_local_as
+      || old->timeout != new->timeout	// TODO distinguish message timemout
+	//(Open.timeout and timeout for timer)
       || old->priority != new->priority
-      || !strncmp(old->description, new->description, UINT32_MAX))
-    return 0;
-
-  return 1;
-
-/*
-  return !memcmp(((byte *) old) + sizeof(struct proto_config),
-      ((byte *) new) + sizeof(struct proto_config),
-      OFFSETOF(struct snmp_config, description) - sizeof(struct proto_config))
-    && ! strncmp(old->description, new->description, UINT32_MAX);
-*/
+      || strncmp(old->description, new->description, UINT32_MAX));
 }
 
 /*
@@ -632,19 +580,34 @@ snmp_reconfigure(struct proto *P, struct proto_config *CF)
   struct snmp_proto *p = SKIP_BACK(struct snmp_proto, p, P);
   const struct snmp_config *new = SKIP_BACK(struct snmp_config, cf, CF);
 
-  // TODO do not reject reconfiguration when only BGP peer list changed
-
   /* We are searching for configuration changes */
-  int config_changed = snmp_reconfigure_logic(p, new);
+  int reconfigurable = snmp_reconfigure_logic(p, new);
 
-  if (config_changed)
+  if (reconfigurable)
   {
-    /* Reinitialize the hash after snmp_shutdown() */
+    /* copy possibly changed values */
+    p->startup_delay = new->startup_delay;
+
+    ASSERT(p->ping_timer);
+    int active = tm_active(p->ping_timer);
+    rfree(p->ping_timer);
+    p->ping_timer = tm_new_init(p->pool, snmp_ping_timeout, p, p->timeout, 0);
+
+    if (active)
+      tm_start(p->ping_timer, p->timeout);
+
+    HASH_FREE(p->bgp_hash);
     HASH_INIT(p->bgp_hash, p->pool, 10);
-    snmp_bgp4_start(p);
+
+    rfree(p->lp);
+    p->lp = lp_new(p->pool);
+    p->bgp_trie = f_new_trie(p->lp, 0);
+
+    /* We repopulate BGP related data structures (bgp_hash, bgp_trie). */
+    snmp_bgp4_start(p, 0);
   }
 
-  return config_changed;
+  return reconfigurable;
 }
 
 /*

proto/snmp/snmp.h

@@ -142,10 +142,6 @@ enum agentx_mibs {
   AGENTX_MIB_UNKNOWN,
 };
 
-extern const struct oid *agentx_available_mibs[AGENTX_MIB_COUNT + 1];
-void agentx_get_mib_init(pool *p);
-enum agentx_mibs agentx_get_mib(const struct oid *o);
-
 struct snmp_registration;
 struct agentx_response; /* declared in subagent.h */
 typedef void (*snmp_reg_hook_t)(struct snmp_proto *p, const struct agentx_response *res, struct snmp_registration *reg);

proto/snmp/snmp_utils.c

@@ -986,7 +986,7 @@ snmp_oid_common_ancestor(const struct oid *left, const struct oid *right, struct
  * SNMP MIB tree walking
  */
 struct mib_leaf *
-snmp_walk_init(struct mib_tree *tree, struct mib_walk_state *walk, const struct oid *oid, struct snmp_pdu *c)
+snmp_walk_init(struct mib_tree *tree, struct mib_walk_state *walk, struct snmp_pdu *c)
 {
   mib_tree_walk_init(walk, tree);
 
@@ -1071,10 +1071,16 @@ snmp_walk_fill(struct mib_leaf *leaf, struct mib_walk_state *walk, struct snmp_p
 {
   struct agentx_varbind *vb = c->sr_vb_start;
 
-  enum agentx_search_res res;
-  if (snmp_oid_compare(&c->sr_vb_start->name, c->sr_o_end) >= 0)
+  enum snmp_search_res res;
+  /* The OID c->sr_vb_start->name is either left untouched for agentx-Get-PDU,
+   * or updated by snmp_walk_next() for agentx-GetNext-PDU and agentx-GetBulk-PDU
+   *
+   * The null OID in c->sr_o_end means no limits. The OID c->sr_o_end is always
+   * null for agentx-Get-PDU and therefore evaluates to 0.
+   */
+  if (!snmp_check_search_limit(&c->sr_vb_start->name, c->sr_o_end))
   {
-    res = AGENTX_END_OF_MIB_VIEW;
+    res = SNMP_SEARCH_END_OF_VIEW;
     vb->type = snmp_search_res_to_type(res);
     return res;
   }

proto/snmp/snmp_utils.h

@@ -33,6 +33,13 @@ void snmp_oid_common_ancestor(const struct oid *left, const struct oid *right, s
 void snmp_oid_from_buf(struct oid *dest, const struct oid *src);
 void snmp_oid_to_buf(struct oid *dest, const struct oid *src);
 
+static inline int
+snmp_check_search_limit(const struct oid *search, const struct oid *limit)
+{
+  ASSERT(search && limit);
+  return snmp_is_oid_empty(limit) || snmp_oid_compare(search, limit) < 0;
+}
+
 static inline int
 snmp_oid_is_prefixed(const struct oid *oid)
 {
@@ -111,7 +118,7 @@ enum agentx_type snmp_search_res_to_type(enum snmp_search_res res);
 /*
  * SNMP MIB tree walking
  */
-struct mib_leaf *snmp_walk_init(struct mib_tree *tree, struct mib_walk_state *state, const struct oid *start_rx, struct snmp_pdu *context);
+struct mib_leaf *snmp_walk_init(struct mib_tree *tree, struct mib_walk_state *state, struct snmp_pdu *context);
 struct mib_leaf *snmp_walk_next(struct mib_tree *tree, struct mib_walk_state *state, struct snmp_pdu *context);
 enum snmp_search_res snmp_walk_fill(struct mib_leaf *leaf, struct mib_walk_state *state, struct snmp_pdu *context);
 

proto/snmp/subagent.c

@@ -1004,7 +1004,7 @@ void
 snmp_get_pdu(struct snmp_proto *p, struct snmp_pdu *c, struct mib_walk_state *walk)
 {
   struct mib_leaf *leaf;
-  leaf = snmp_walk_init(p->mib_tree, walk, &c->sr_vb_start->name, c);
+  leaf = snmp_walk_init(p->mib_tree, walk, c);
 
   enum snmp_search_res res;
   res = snmp_walk_fill(leaf, walk, c);
@@ -1017,7 +1017,7 @@ snmp_get_pdu(struct snmp_proto *p, struct snmp_pdu *c, struct mib_walk_state *wa
 int
 snmp_get_next_pdu(struct snmp_proto *p, struct snmp_pdu *c, struct mib_walk_state *walk)
 {
-  (void) snmp_walk_init(p->mib_tree, walk, &c->sr_vb_start->name, c);
+  (void) snmp_walk_init(p->mib_tree, walk, c);
   struct mib_leaf *leaf = snmp_walk_next(p->mib_tree, walk, c);
 
   enum snmp_search_res res;
@@ -1033,12 +1033,16 @@ snmp_get_next_pdu(struct snmp_proto *p, struct snmp_pdu *c, struct mib_walk_stat
 void
 snmp_get_bulk_pdu(struct snmp_proto *p, struct snmp_pdu *c, struct mib_walk_state *walk)
 {
-  if (c->index >= bulk->getbulk.non_repeaters)
-    bulk->repeaters++;
+  /* TODO */
+  (void) p;
+  (void) c;
+  (void) walk;
+  //if (c->index >= bulk->getbulk.non_repeaters)
+  //  bulk->repeaters++;
 
   // store the o_start and o_end
 
-  bulk->has_any |= snmp_get_next_pdu(p, c, walk);
+  //bulk->has_any |= snmp_get_next_pdu(p, c, walk);
 }
 
 int
@@ -1130,6 +1134,7 @@ parse_gets_pdu(struct snmp_proto *p, byte * const pkt_start)
   struct agentx_bulk_state bulk_state = { 0 };
   if (h->type == AGENTX_GET_BULK_PDU)
   {
+    (void)bulk_state;
     die("bulk");
 
 #if 0