From: David Lamparter <equinox@opensourcerouting.org>
Date: Thu, 6 Jul 2017 13:30:31 +0000 (+0200)
Subject: lib: add some abstraction guards for table code
X-Git-Tag: reindent-master-before~3^2~9
X-Git-Url: https://git.puffer.fish/?a=commitdiff_plain;h=4cb260c33e7656d35167beb3d006e6a78766716e;p=matthieu%2Ffrr.git

lib: add some abstraction guards for table code

route_node->parent and route_node->link shouldn't be touched by user
code since that is a recipe for trouble once we have a hash table in
parallel.

Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
---

diff --git a/bgpd/rfapi/rfapi_import.c b/bgpd/rfapi/rfapi_import.c
index b0c6db2a1e..b6c0e30295 100644
--- a/bgpd/rfapi/rfapi_import.c
+++ b/bgpd/rfapi/rfapi_import.c
@@ -1783,6 +1783,8 @@ rfapiNhlAddSubtree (
   struct rfapi_ip_prefix rprefix;
   int                    rcount = 0;
 
+  /* FIXME: need to find a better way here to work without sticking our
+   * hands in node->link */
   if (rn->l_left && rn->l_left != omit_node)
     {
       if (rn->l_left->info)
diff --git a/bgpd/rfapi/rfapi_rib.c b/bgpd/rfapi/rfapi_rib.c
index 5c3976a0c1..855901b5a7 100644
--- a/bgpd/rfapi/rfapi_rib.c
+++ b/bgpd/rfapi/rfapi_rib.c
@@ -1804,6 +1804,8 @@ rfapiRibUpdatePendingNodeSubtree (
   struct route_node		*omit_subtree,    /* may be NULL */
   uint32_t			lifetime)
 {
+  /* FIXME: need to find a better way here to work without sticking our
+   * hands in node->link */
   if (it_node->l_left && (it_node->l_left != omit_subtree))
     {
       if (it_node->l_left->info)
diff --git a/lib/table.c b/lib/table.c
index fdbd2913b2..d4dbeb79cc 100644
--- a/lib/table.c
+++ b/lib/table.c
@@ -19,6 +19,8 @@
  * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
  */
 
+#define FRR_COMPILING_TABLE_C
+
 #include <zebra.h>
 
 #include "prefix.h"
diff --git a/lib/table.h b/lib/table.h
index 6a5291487b..a906eb2434 100644
--- a/lib/table.h
+++ b/lib/table.h
@@ -71,6 +71,41 @@ struct route_table
   void *info;
 };
 
+/*
+ * node->link is really internal to the table code and should not be
+ * accessed by outside code.  We don't have any writers (yay), though some
+ * readers are left to be fixed.
+ *
+ * rationale: we need to add a hash table in parallel, to speed up
+ * exact-match lookups.
+ *
+ * same really applies for node->parent, though that's less of an issue.
+ * table->link should be - and is - NEVER written by outside code
+ */
+#ifdef FRR_COMPILING_TABLE_C
+#define table_rdonly(x)		x
+#define table_internal(x)	x
+#else
+#define table_rdonly(x)		const x
+#define table_internal(x)	const x \
+	__attribute__((deprecated("this should only be accessed by lib/table.c")))
+/* table_internal is for node->link and node->lock, once we have done
+ * something about remaining accesses */
+#endif
+
+/* so... the problem with this is that "const" doesn't mean "readonly".
+ * It in fact may allow the compiler to optimize based on the assumption
+ * that the value doesn't change.  Hence, since the only purpose of this
+ * is to aid in development, don't put the "const" in release builds.
+ *
+ * (I haven't seen this actually break, but GCC and LLVM are getting ever
+ * more aggressive in optimizing...)
+ */
+#ifndef DEV_BUILD
+#undef table_rdonly
+#define table_rdonly(x) x
+#endif
+
 /*
  * Macro that defines all fields in a route node.
  */
@@ -79,12 +114,12 @@ struct route_table
   struct prefix p;				\
 						\
   /* Tree link. */				\
-  struct route_table *table;			\
-  struct route_node *parent;			\
-  struct route_node *link[2];			\
+  struct route_table * table_rdonly(table);	\
+  struct route_node * table_rdonly(parent);	\
+  struct route_node * table_rdonly(link[2]);	\
 						\
   /* Lock of this radix */			\
-  unsigned int lock;				\
+  unsigned int table_rdonly(lock);		\
 						\
   /* Each node of route. */			\
   void *info;					\
diff --git a/ospf6d/ospf6_lsdb.c b/ospf6d/ospf6_lsdb.c
index cb522226ac..ce31e25a2f 100644
--- a/ospf6d/ospf6_lsdb.c
+++ b/ospf6d/ospf6_lsdb.c
@@ -242,6 +242,9 @@ ospf6_lsdb_lookup_next (u_int16_t type, u_int32_t id, u_int32_t adv_router,
     zlog_debug ("lsdb_lookup_next: key: %s", buf);
   }
 
+  /* FIXME: need to find a better way here to work without sticking our
+   * hands in node->link, e.g. route_node_match_maynull() */
+
   node = lsdb->table->top;
   /* walk down tree. */
   while (node && node->p.prefixlen <= p->prefixlen &&