From 297c8f6a31979637c43227902a8b71f87088e46b Mon Sep 17 00:00:00 2001
From: David Lamparter <equinox@opensourcerouting.org>
Date: Mon, 8 May 2017 06:07:52 +0200
Subject: [PATCH] lib: qobj: MT-guard with rwlock

Make qobj_* calls MT-Safe/LF-Blocking.

Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
---
 lib/qobj.c | 38 ++++++++++++++++++++++++++++++++------
 lib/qobj.h | 13 ++++++++++++-
 2 files changed, 44 insertions(+), 7 deletions(-)

diff --git a/lib/qobj.c b/lib/qobj.c
index fd7b4c8c5b..4cf7fbca7b 100644
--- a/lib/qobj.c
+++ b/lib/qobj.c
@@ -26,6 +26,7 @@
 #include "log.h"
 #include "qobj.h"
 
+static pthread_rwlock_t nodes_lock;
 static struct hash *nodes = NULL;
 
 static unsigned int qobj_key (void *data)
@@ -43,37 +44,61 @@ static int qobj_cmp (const void *a, const void *b)
 void qobj_reg(struct qobj_node *node, struct qobj_nodetype *type)
 {
   node->type = type;
+  pthread_rwlock_wrlock (&nodes_lock);
   do
     {
       node->nid  = (uint64_t)random();
       node->nid ^= (uint64_t)random() << 32;
     }
   while (!node->nid || hash_get (nodes, node, hash_alloc_intern) != node);
+  pthread_rwlock_unlock (&nodes_lock);
 }
 
 void qobj_unreg(struct qobj_node *node)
 {
+  pthread_rwlock_wrlock (&nodes_lock);
   hash_release (nodes, node);
+  pthread_rwlock_unlock (&nodes_lock);
 }
 
 struct qobj_node *qobj_get(uint64_t id)
 {
-  struct qobj_node dummy = { .nid = id };
-  return hash_lookup (nodes, &dummy);
+  struct qobj_node dummy = { .nid = id }, *rv;
+  pthread_rwlock_rdlock (&nodes_lock);
+  rv = hash_lookup (nodes, &dummy);
+  pthread_rwlock_unlock (&nodes_lock);
+  return rv;
 }
 
 void *qobj_get_typed(uint64_t id, struct qobj_nodetype *type)
 {
-  struct qobj_node *node = qobj_get(id);
+  struct qobj_node dummy = { .nid = id };
+  struct qobj_node *node;
+  void *rv;
+
+  pthread_rwlock_rdlock (&nodes_lock);
+  node = hash_lookup (nodes, &dummy);
+
+  /* note: we explicitly hold the lock until after we have checked the type.
+   * if the caller holds a lock that for example prevents the deletion of
+   * route-maps, we can still race against a delete of something that isn't
+   * a route-map. */
   if (!node || node->type != type)
-    return NULL;
-  return (char *)node - node->type->node_member_offset;
+    rv = NULL;
+  else
+    rv = (char *)node - node->type->node_member_offset;
+
+  pthread_rwlock_unlock (&nodes_lock);
+  return rv;
 }
 
 void qobj_init (void)
 {
   if (!nodes)
-    nodes = hash_create (qobj_key, qobj_cmp);
+    {
+      pthread_rwlock_init (&nodes_lock, NULL);
+      nodes = hash_create (qobj_key, qobj_cmp);
+    }
 }
 
 void qobj_finish (void)
@@ -81,4 +106,5 @@ void qobj_finish (void)
   hash_clean (nodes, NULL);
   hash_free (nodes);
   nodes = NULL;
+  pthread_rwlock_destroy (&nodes_lock);
 }
diff --git a/lib/qobj.h b/lib/qobj.h
index 64a6774bff..c24bed4f6a 100644
--- a/lib/qobj.h
+++ b/lib/qobj.h
@@ -81,7 +81,18 @@ struct qobj_node {
 #define QOBJ_UNREG(n) \
 	qobj_unreg(&n->qobj_node)
 
-/* internals - should not be directly used without a good reason*/
+/* internals - should not be directly used without a good reason
+ *
+ * note: qobj_get is essentially never safe to use in MT context because
+ * the object could be deleted by another thread -- and worse, it could be
+ * of the "wrong" type and deleted.
+ *
+ * with qobj_get_typed, the type check is done under lock, which means that
+ * it can be used as long as another lock prevents the deletion of objects
+ * of the expected type.
+ *
+ * in the long this may need another touch, e.g. built-in per-object locking.
+ */
 void qobj_reg(struct qobj_node *node, struct qobj_nodetype *type);
 void qobj_unreg(struct qobj_node *node);
 struct qobj_node *qobj_get(uint64_t id);
-- 
2.39.5