From 134f13f4fdde9ff7af82748c9cdbe004b1be4ef9 Mon Sep 17 00:00:00 2001
From: Igor Putovny <igor.putovny@nic.cz>
Date: Thu, 6 Mar 2025 16:29:03 +0100
Subject: [PATCH] Cleanup

---
 proto/aggregator/aggregator.c |  9 ++++-----
 proto/aggregator/aggregator.h |  1 -
 proto/aggregator/trie.c       | 14 ++++++++------
 3 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/proto/aggregator/aggregator.c b/proto/aggregator/aggregator.c
index 7a6be4321..3736d8f8b 100644
--- a/proto/aggregator/aggregator.c
+++ b/proto/aggregator/aggregator.c
@@ -47,9 +47,6 @@
 #include "filter/filter.h"
 #include "proto/aggregator/aggregator.h"
 
-#include <stdlib.h>
-#include <assert.h>
-
 extern linpool *rte_update_pool;
 
 /*
@@ -64,8 +61,9 @@ aggregator_get_new_bucket_id(struct aggregator_proto *p)
 }
 
 /*
- * Insert @bucket to the list of bucket pointers in @p to position @bucket.id
+ * Add @bucket to the list of bucket pointers in @p to position @bucket.id
  */
+// TODO: enable to reset bucket ptr?
 static void
 aggregator_add_bucket(struct aggregator_proto *p, struct aggregator_bucket *bucket)
 {
@@ -712,7 +710,7 @@ aggregator_rt_notify(struct proto *P, struct channel *src_ch, net *net, rte *new
     {
       aggregator_recalculate(p, old_route, new_route);
 
-      /* Process all route withdrawals which were caused by the update */
+      /* Process route withdrawals triggered by recalculation */
       aggregator_withdraw_rte(p);
     }
   }
@@ -766,6 +764,7 @@ aggregator_postconfig(struct proto_config *CF)
   cf->dst->debug = cf->src->debug;
 }
 
+// TODO: set pools to NULL?
 static struct proto *
 aggregator_init(struct proto_config *CF)
 {
diff --git a/proto/aggregator/aggregator.h b/proto/aggregator/aggregator.h
index 31b9a0a59..5e58526bb 100644
--- a/proto/aggregator/aggregator.h
+++ b/proto/aggregator/aggregator.h
@@ -55,7 +55,6 @@ struct aggregator_bucket {
   struct f_val aggr_data[0];
 };
 
-/* Information needed for route withdrawal */
 struct rte_withdrawal_item {
   struct rte_withdrawal_item *next;
   struct aggregator_bucket *bucket;
diff --git a/proto/aggregator/trie.c b/proto/aggregator/trie.c
index 49c4c6050..bd667d150 100644
--- a/proto/aggregator/trie.c
+++ b/proto/aggregator/trie.c
@@ -77,9 +77,6 @@
 #include "filter/filter.h"
 #include "proto/aggregator/aggregator.h"
 
-#include <stdlib.h>
-#include <assert.h>
-
 static const char *px_origin_str[] = {
   [FILLER]     = "filler",
   [ORIGINAL]   = "original",
@@ -94,6 +91,7 @@ static const u32 ipa_shift[] = {
 /*
  * Allocate new node in trie slab
  */
+// TODO: inline in aggregator.h?
 struct trie_node *
 aggregator_create_new_node(struct slab *trie_slab)
 {
@@ -109,6 +107,7 @@ aggregator_is_leaf(const struct trie_node *node)
 
 /*
  * Unlink node from the trie by setting appropriate child of parent node to NULL
+ * and free memory.
  */
 static inline void
 aggregator_remove_node(struct trie_node *node)
@@ -248,7 +247,7 @@ aggregator_merge_potential_buckets(struct trie_node *target, const struct trie_n
 
     /*
      * If old and new values are different, the result of their XOR will be
-     * non-zero, thus @changed will be set to non-zero - true, as well.
+     * non-zero, thus @has_changed will be set to non-zero -- true, as well.
      */
     has_changed |= !!(old[i] ^ target->potential_buckets[i]);
   }
@@ -534,7 +533,9 @@ aggregator_find_subtree_prefix(const struct trie_node *target, ip_addr *prefix,
     else
       ipa_setbit(prefix, node->depth + ipa_shift[type]);
 
+    // TODO: remove after testing, traversing back to target node is not neccessary
     node = node->child[path[i]];
+
     len++;
     ASSERT_DIE((u32)node->depth == len);
   }
@@ -612,7 +613,7 @@ aggregator_second_pass(struct trie_node *node, int recomputing)
    */
   aggregator_node_add_potential_bucket(&imaginary_node, node->original_bucket);
 
-  /* Nodes with exactly one child */
+  /* Nodes with only one child */
   if ((left && !right) || (!left && right))
   {
     if (left && !right)
@@ -887,7 +888,7 @@ aggregator_merge_buckets_above(struct trie_node *node)
 
     ASSERT_DIE(left != NULL && right != NULL);
 
-    /* The parent's set wasn't affected by merging, stop here */
+    /* The parent's set didn't change by merging, stop here */
     if (aggregator_merge_potential_buckets(parent, left, right) == 0)
       return node;
 
@@ -994,6 +995,7 @@ aggregator_recalculate(struct aggregator_proto *p, struct aggregator_route *old,
   }
   */
 
+  // TODO: root should never get here
   ASSERT_DIE(ancestor != NULL);
   ASSERT_DIE(ancestor != updated_node);
   ASSERT_DIE(ancestor->status == IN_FIB);
-- 
2.47.2