hmap: Add extra build-time iteration checks for types derived from hmap.

Some of our data structures derived from hmap use the same member names.
This means it's possible to confuse them in iteration, e.g. to iterate a
shash with SIMAP_FOR_EACH.  Of course this will crash at runtime, but it
seems even better to catch it at compile time.

An alternative would be to use unique member names, e.g. shash_map and
simap_map instead of just map.  I like short names, though.

It's kind of nasty that we need support from the hmap code to do this.
An alternative would be to insert the build assertions as statements before
the for loop.  But that would cause nasty surprises if someone forgets the
{} around a block of statements; even though the OVS coding style requires
them in all cases, I suspect that programmers doing debugging, etc. tend
to omit them sometimes.

It's not actually necessary to have multiple variants of these macros,
e.g. one can write a C99-compliant HMAP_FOR_EACH that accepts 3 or 4 or
more arguments.  But such a macro is harder to read, so I don't know
whether this is a good tradeoff.

Signed-off-by: Ben Pfaff <blp@ovn.org>
Acked-by: Andy Zhou <azhou@ovn.org>

diff --git a/lib/hmap.h b/lib/hmap.h
index d2327cb..53e75cc 100644 (file)
--- a/lib/hmap.h
+++ b/lib/hmap.h
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2008, 2009, 2010, 2012, 2013, 2015 Nicira, Inc.
+ * Copyright (c) 2008, 2009, 2010, 2012, 2013, 2015, 2016 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -153,26 +153,43 @@ static inline struct hmap_node *hmap_next_in_bucket(const struct hmap_node *);
 
 bool hmap_contains(const struct hmap *, const struct hmap_node *);
 
-/* Iteration. */
+/* Iteration.
+ *
+ * The *_INIT variants of these macros additionally evaluate the expressions
+ * supplied following the HMAP argument once during the loop initialization.
+ * This makes it possible for data structures that wrap around hmaps to insert
+ * additional initialization into their iteration macros without having to
+ * completely rewrite them.  In particular, it can be a good idea to insert
+ * BUILD_ASSERT_TYPE checks for map and node types that wrap hmap, since
+ * otherwise it is possible for clients to accidentally confuse two derived
+ * data structures that happen to use the same member names for struct hmap and
+ * struct hmap_node. */
 
 /* Iterates through every node in HMAP. */
-#define HMAP_FOR_EACH(NODE, MEMBER, HMAP)                               \
-    for (INIT_CONTAINER(NODE, hmap_first(HMAP), MEMBER);                \
+#define HMAP_FOR_EACH(NODE, MEMBER, HMAP) \
+    HMAP_FOR_EACH_INIT(NODE, MEMBER, HMAP, (void) 0)
+#define HMAP_FOR_EACH_INIT(NODE, MEMBER, HMAP, ...)                     \
+    for (INIT_CONTAINER(NODE, hmap_first(HMAP), MEMBER), __VA_ARGS__;   \
          (NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER)) || (NODE = NULL); \
          ASSIGN_CONTAINER(NODE, hmap_next(HMAP, &(NODE)->MEMBER), MEMBER))
 
 /* Safe when NODE may be freed (not needed when NODE may be removed from the
  * hash map but its members remain accessible and intact). */
-#define HMAP_FOR_EACH_SAFE(NODE, NEXT, MEMBER, HMAP)                    \
-    for (INIT_CONTAINER(NODE, hmap_first(HMAP), MEMBER);                \
+#define HMAP_FOR_EACH_SAFE(NODE, NEXT, MEMBER, HMAP) \
+    HMAP_FOR_EACH_SAFE_INIT(NODE, NEXT, MEMBER, HMAP, (void) 0)
+#define HMAP_FOR_EACH_SAFE_INIT(NODE, NEXT, MEMBER, HMAP, ...)          \
+    for (INIT_CONTAINER(NODE, hmap_first(HMAP), MEMBER), __VA_ARGS__;   \
          ((NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER)) || (NODE = NULL) \
           ? INIT_CONTAINER(NEXT, hmap_next(HMAP, &(NODE)->MEMBER), MEMBER), 1 \
           : 0);                                                         \
          (NODE) = (NEXT))
 
 /* Continues an iteration from just after NODE. */
-#define HMAP_FOR_EACH_CONTINUE(NODE, MEMBER, HMAP)                      \
-    for (ASSIGN_CONTAINER(NODE, hmap_next(HMAP, &(NODE)->MEMBER), MEMBER); \
+#define HMAP_FOR_EACH_CONTINUE(NODE, MEMBER, HMAP) \
+    HMAP_FOR_EACH_CONTINUE_INIT(NODE, MEMBER, HMAP, (void) 0)
+#define HMAP_FOR_EACH_CONTINUE_INIT(NODE, MEMBER, HMAP, ...)            \
+    for (ASSIGN_CONTAINER(NODE, hmap_next(HMAP, &(NODE)->MEMBER), MEMBER), \
+         __VA_ARGS__;                                                   \
          (NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER)) || (NODE = NULL); \
          ASSIGN_CONTAINER(NODE, hmap_next(HMAP, &(NODE)->MEMBER), MEMBER))
 

diff --git a/lib/hmapx.h b/lib/hmapx.h
index 9b00a5d..f977d9e 100644 (file)
--- a/lib/hmapx.h
+++ b/lib/hmapx.h
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2011 Nicira, Inc.
+ * Copyright (c) 2011, 2016 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -60,12 +60,17 @@ bool hmapx_equals(const struct hmapx *, const struct hmapx *);
 /* Iteration. */
 
 /* Iterates through every hmapx_node in HMAPX. */
-#define HMAPX_FOR_EACH(NODE, HMAPX)             \
-    HMAP_FOR_EACH(NODE, hmap_node, &(HMAPX)->map)
+#define HMAPX_FOR_EACH(NODE, HMAPX)                                     \
+    HMAP_FOR_EACH_INIT(NODE, hmap_node, &(HMAPX)->map,                  \
+                       BUILD_ASSERT_TYPE(NODE, struct hmapx_node *),    \
+                       BUILD_ASSERT_TYPE(HMAPX, struct hmapx *))
 
 /* Safe when NODE may be freed (not needed when NODE may be removed from the
  * hash map but its members remain accessible and intact). */
-#define HMAPX_FOR_EACH_SAFE(NODE, NEXT, HMAPX)                  \
-    HMAP_FOR_EACH_SAFE(NODE, NEXT, hmap_node, &(HMAPX)->map)
+#define HMAPX_FOR_EACH_SAFE(NODE, NEXT, HMAPX)                          \
+    HMAP_FOR_EACH_SAFE_INIT(NODE, NEXT, hmap_node, &(HMAPX)->map,       \
+                            BUILD_ASSERT_TYPE(NODE, struct hmapx_node *), \
+                            BUILD_ASSERT_TYPE(NEXT, struct hmapx_node *), \
+                            BUILD_ASSERT_TYPE(HMAPX, struct hmapx *))
 
 #endif /* hmapx.h */

diff --git a/lib/shash.h b/lib/shash.h
index 97d36ba..5f94725 100644 (file)
--- a/lib/shash.h
+++ b/lib/shash.h
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2009, 2010, 2011 Nicira, Inc.
+ * Copyright (c) 2009, 2010, 2011, 2016 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -18,6 +18,7 @@
 #define SHASH_H 1
 
 #include "hmap.h"
+#include "util.h"
 
 #ifdef  __cplusplus
 extern "C" {
@@ -35,11 +36,17 @@ struct shash {
 
 #define SHASH_INITIALIZER(SHASH) { HMAP_INITIALIZER(&(SHASH)->map) }
 
-#define SHASH_FOR_EACH(SHASH_NODE, SHASH) \
-    HMAP_FOR_EACH (SHASH_NODE, node, &(SHASH)->map)
+#define SHASH_FOR_EACH(SHASH_NODE, SHASH)                               \
+    HMAP_FOR_EACH_INIT (SHASH_NODE, node, &(SHASH)->map,                \
+                        BUILD_ASSERT_TYPE(SHASH_NODE, struct shash_node *), \
+                        BUILD_ASSERT_TYPE(SHASH, struct shash *))
 
-#define SHASH_FOR_EACH_SAFE(SHASH_NODE, NEXT, SHASH) \
-    HMAP_FOR_EACH_SAFE (SHASH_NODE, NEXT, node, &(SHASH)->map)
+#define SHASH_FOR_EACH_SAFE(SHASH_NODE, NEXT, SHASH)        \
+    HMAP_FOR_EACH_SAFE_INIT (                               \
+        SHASH_NODE, NEXT, node, &(SHASH)->map,              \
+        BUILD_ASSERT_TYPE(SHASH_NODE, struct shash_node *), \
+        BUILD_ASSERT_TYPE(NEXT, struct shash_node *),       \
+        BUILD_ASSERT_TYPE(SHASH, struct shash *))
 
 void shash_init(struct shash *);
 void shash_destroy(struct shash *);

diff --git a/lib/simap.h b/lib/simap.h
index 3e247fc..113db93 100644 (file)
--- a/lib/simap.h
+++ b/lib/simap.h
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2009, 2010, 2011, 2012 Nicira, Inc.
+ * Copyright (c) 2009, 2010, 2011, 2012, 2016 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -36,11 +36,16 @@ struct simap_node {
 
 #define SIMAP_INITIALIZER(SIMAP) { HMAP_INITIALIZER(&(SIMAP)->map) }
 
-#define SIMAP_FOR_EACH(SIMAP_NODE, SIMAP) \
-    HMAP_FOR_EACH (SIMAP_NODE, node, &(SIMAP)->map)
+#define SIMAP_FOR_EACH(SIMAP_NODE, SIMAP)                               \
+    HMAP_FOR_EACH_INIT (SIMAP_NODE, node, &(SIMAP)->map,                \
+                        BUILD_ASSERT_TYPE(SIMAP_NODE, struct simap_node *), \
+                        BUILD_ASSERT_TYPE(SIMAP, struct simap *))
 
-#define SIMAP_FOR_EACH_SAFE(SIMAP_NODE, NEXT, SIMAP) \
-    HMAP_FOR_EACH_SAFE (SIMAP_NODE, NEXT, node, &(SIMAP)->map)
+#define SIMAP_FOR_EACH_SAFE(SIMAP_NODE, NEXT, SIMAP)                    \
+    HMAP_FOR_EACH_SAFE_INIT (SIMAP_NODE, NEXT, node, &(SIMAP)->map,     \
+                        BUILD_ASSERT_TYPE(SIMAP_NODE, struct simap_node *), \
+                        BUILD_ASSERT_TYPE(NEXT, struct simap_node *),   \
+                        BUILD_ASSERT_TYPE(SIMAP, struct simap *))
 
 void simap_init(struct simap *);
 void simap_destroy(struct simap *);

diff --git a/lib/smap.h b/lib/smap.h
index 489497a..7562f38 100644 (file)
--- a/lib/smap.h
+++ b/lib/smap.h
@@ -1,4 +1,4 @@
-/* Copyright (c) 2012, 2014, 2015 Nicira, Inc.
+/* Copyright (c) 2012, 2014, 2015, 2016 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -33,11 +33,17 @@ struct smap_node {
 
 #define SMAP_INITIALIZER(SMAP) { HMAP_INITIALIZER(&(SMAP)->map) }
 
-#define SMAP_FOR_EACH(SMAP_NODE, SMAP) \
-    HMAP_FOR_EACH (SMAP_NODE, node, &(SMAP)->map)
-
-#define SMAP_FOR_EACH_SAFE(SMAP_NODE, NEXT, SMAP) \
-    HMAP_FOR_EACH_SAFE (SMAP_NODE, NEXT, node, &(SMAP)->map)
+#define SMAP_FOR_EACH(SMAP_NODE, SMAP)                                  \
+    HMAP_FOR_EACH_INIT (SMAP_NODE, node, &(SMAP)->map,                  \
+                        BUILD_ASSERT_TYPE(SMAP_NODE, struct smap_node *), \
+                        BUILD_ASSERT_TYPE(SMAP, struct smap *))
+
+#define SMAP_FOR_EACH_SAFE(SMAP_NODE, NEXT, SMAP)           \
+    HMAP_FOR_EACH_SAFE_INIT (                               \
+        SMAP_NODE, NEXT, node, &(SMAP)->map,                \
+        BUILD_ASSERT_TYPE(SMAP_NODE, struct smap_node *),   \
+        BUILD_ASSERT_TYPE(NEXT, struct smap_node *),        \
+        BUILD_ASSERT_TYPE(SMAP, struct smap *))
 
 /* Initializer for an immutable struct smap 'SMAP' that contains a single
  * 'KEY'-'VALUE' pair, e.g.