From 6cfa5416e106f2c4d13de1b57813e47f65c9aa80 Mon Sep 17 00:00:00 2001 From: Vojtech Vilimek Date: Wed, 14 Aug 2024 16:16:10 +0200 Subject: [PATCH] SNMP: Bugfixes, reconfiguration changes, cleanup Major rework of reconfiguration handling. --- proto/snmp/bgp4_mib.c | 8 ++- proto/snmp/bgp4_mib.h | 2 +- proto/snmp/mib_tree.c | 3 +- proto/snmp/snmp.c | 137 +++++++++++++++------------------------- proto/snmp/snmp.h | 4 -- proto/snmp/snmp_utils.c | 14 ++-- proto/snmp/snmp_utils.h | 11 +++- proto/snmp/subagent.c | 15 +++-- 8 files changed, 87 insertions(+), 107 deletions(-) diff --git a/proto/snmp/bgp4_mib.c b/proto/snmp/bgp4_mib.c index 446e45f11..3f9fce29a 100644 --- a/proto/snmp/bgp4_mib.c +++ b/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); diff --git a/proto/snmp/bgp4_mib.h b/proto/snmp/bgp4_mib.h index c486a0df0..21a6767fa 100644 --- a/proto/snmp/bgp4_mib.h +++ b/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); diff --git a/proto/snmp/mib_tree.c b/proto/snmp/mib_tree.c index 23fcdfc54..03f942e91 100644 --- a/proto/snmp/mib_tree.c +++ b/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) diff --git a/proto/snmp/snmp.c b/proto/snmp/snmp.c index e8d30d7ed..0c55b58b0 100644 --- a/proto/snmp/snmp.c +++ b/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; - } - + if (old->trans_type == SNMP_TRANS_TCP && + (ipa_compare(old->remote_ip, new->remote_ip) + || old->remote_port != new->remote_port)) return 0; -skip: - bonds--; - } - if (bonds != 0) + if (old->trans_type != SNMP_TRANS_TCP && + bstrcmp(old->remote_path, new->remote_path)) 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 + 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; } /* diff --git a/proto/snmp/snmp.h b/proto/snmp/snmp.h index 5c5a6429f..3759ccfc0 100644 --- a/proto/snmp/snmp.h +++ b/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); diff --git a/proto/snmp/snmp_utils.c b/proto/snmp/snmp_utils.c index 51cdb6b6d..9540517e1 100644 --- a/proto/snmp/snmp_utils.c +++ b/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; } diff --git a/proto/snmp/snmp_utils.h b/proto/snmp/snmp_utils.h index 5455f0d6f..bde431797 100644 --- a/proto/snmp/snmp_utils.h +++ b/proto/snmp/snmp_utils.h @@ -31,7 +31,14 @@ void snmp_oid_copy2(struct oid *dest, const struct oid *src); int snmp_oid_compare(const struct oid *first, const struct oid *second); void snmp_oid_common_ancestor(const struct oid *left, const struct oid *right, struct oid *result); void snmp_oid_from_buf(struct oid *dest, const struct oid *src); -void snmp_oid_to_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); diff --git a/proto/snmp/subagent.c b/proto/snmp/subagent.c index c423a8407..d86917c82 100644 --- a/proto/snmp/subagent.c +++ b/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 -- 2.47.2