ovn-controller: Introduce helpers for looking up datapaths.

The new helpers get_local_datapath() and get_patched_datapath() make code
a little shorter and easier to read.  They also avoid a pitfall that was
present in at least a few of the instances: CONTAINER_OF is not safe on a
null pointer, because it does a raw pointer subtraction and will change
NULL to something else.  This wasn't actually a problem in these particular
cases because the value it was subtracting was zero (although arguably it
is still undefined behavior because the compiler is allowed to assume that
a pointer on which arithmetic is performed is nonnull).

Signed-off-by: Ben Pfaff <blp@ovn.org>
Acked-by: Russell Bryant <russell@ovn.org>

diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c
index d3ca9c9..32fcb85 100644 (file)
--- a/ovn/controller/binding.c
+++ b/ovn/controller/binding.c
@@ -125,8 +125,8 @@ static void
 add_local_datapath(struct hmap *local_datapaths,
         const struct sbrec_port_binding *binding_rec)
 {
-    if (hmap_first_with_hash(local_datapaths,
-                             binding_rec->datapath->tunnel_key)) {
+    if (get_local_datapath(local_datapaths,
+                           binding_rec->datapath->tunnel_key)) {
         return;
     }
 

diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c
index 7a3466f..91a3eee 100644 (file)
--- a/ovn/controller/lflow.c
+++ b/ovn/controller/lflow.c
@@ -241,15 +241,12 @@ add_logical_flows(struct controller_ctx *ctx, const struct lport_index *lports,
              * large lrouters and lswitches. This need to be studied further.
              */
 
-            struct hmap_node *ld;
-            ld = hmap_first_with_hash(local_datapaths, ldp->tunnel_key);
-            if (!ld) {
+            if (!get_local_datapath(local_datapaths, ldp->tunnel_key)) {
                 if (!ingress) {
                     continue;
                 }
-                struct hmap_node *pd;
-                pd = hmap_first_with_hash(patched_datapaths, ldp->tunnel_key);
-                if (!pd) {
+                if (!get_patched_datapath(patched_datapaths,
+                                          ldp->tunnel_key)) {
                     continue;
                 }
             }

diff --git a/ovn/controller/ovn-controller.c b/ovn/controller/ovn-controller.c
index 6027011..7c68c9d 100644 (file)
--- a/ovn/controller/ovn-controller.c
+++ b/ovn/controller/ovn-controller.c
@@ -62,6 +62,25 @@ OVS_NO_RETURN static void usage(void);
 
 static char *ovs_remote;
 
+struct local_datapath *
+get_local_datapath(const struct hmap *local_datapaths, uint32_t tunnel_key)
+{
+    struct hmap_node *node = hmap_first_with_hash(local_datapaths, tunnel_key);
+    return (node
+            ? CONTAINER_OF(node, struct local_datapath, hmap_node)
+            : NULL);
+}
+
+struct patched_datapath *
+get_patched_datapath(const struct hmap *patched_datapaths, uint32_t tunnel_key)
+{
+    struct hmap_node *node = hmap_first_with_hash(patched_datapaths,
+                                                  tunnel_key);
+    return (node
+            ? CONTAINER_OF(node, struct patched_datapath, hmap_node)
+            : NULL);
+}
+
 const struct sbrec_chassis *
 get_chassis(struct ovsdb_idl *ovnsb_idl, const char *chassis_id)
 {

diff --git a/ovn/controller/ovn-controller.h b/ovn/controller/ovn-controller.h
index 9955097..9af7959 100644 (file)
--- a/ovn/controller/ovn-controller.h
+++ b/ovn/controller/ovn-controller.h
@@ -1,4 +1,4 @@
-/* Copyright (c) 2015 Nicira, Inc.
+/* Copyright (c) 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.
@@ -41,12 +41,18 @@ struct local_datapath {
     const struct sbrec_port_binding *localnet_port;
 };
 
+struct local_datapath *get_local_datapath(const struct hmap *,
+                                          uint32_t tunnel_key);
+
 /* Contains hmap_node whose hash values are the tunnel_key of datapaths
  * with at least one logical patch port binding. */
 struct patched_datapath {
     struct hmap_node hmap_node;
 };
 
+struct patched_datapath *get_patched_datapath(const struct hmap *,
+                                              uint32_t tunnel_key);
+
 const struct ovsrec_bridge *get_bridge(struct ovsdb_idl *,
                                        const char *br_name);
 

diff --git a/ovn/controller/patch.c b/ovn/controller/patch.c
index 943ac99..4808146 100644 (file)
--- a/ovn/controller/patch.c
+++ b/ovn/controller/patch.c
@@ -1,4 +1,4 @@
-/* Copyright (c) 2015 Nicira, Inc.
+/* Copyright (c) 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.
@@ -176,10 +176,9 @@ add_bridge_mappings(struct controller_ctx *ctx,
     const struct sbrec_port_binding *binding;
     SBREC_PORT_BINDING_FOR_EACH (binding, ctx->ovnsb_idl) {
         if (!strcmp(binding->type, "localnet")) {
-            struct local_datapath *ld;
-            ld = CONTAINER_OF(hmap_first_with_hash(local_datapaths,
-                              binding->datapath->tunnel_key),
-                              struct local_datapath, hmap_node);
+            struct local_datapath *ld
+                = get_local_datapath(local_datapaths,
+                                     binding->datapath->tunnel_key);
             if (!ld) {
                 /* This localnet port is on a datapath with no
                  * logical ports bound to this chassis, so there's no need
@@ -233,7 +232,7 @@ static void
 add_patched_datapath(struct hmap *patched_datapaths,
                      const struct sbrec_port_binding *binding_rec)
 {
-    if (hmap_first_with_hash(patched_datapaths,
+    if (get_patched_datapath(patched_datapaths,
                              binding_rec->datapath->tunnel_key)) {
         return;
     }

diff --git a/ovn/controller/physical.c b/ovn/controller/physical.c
index d1a41c1..a210b39 100644 (file)
--- a/ovn/controller/physical.c
+++ b/ovn/controller/physical.c
@@ -138,9 +138,8 @@ put_stack(enum mf_field_id field, struct ofpact_stack *stack)
 static const struct sbrec_port_binding*
 get_localnet_port(struct hmap *local_datapaths, int64_t tunnel_key)
 {
-    struct local_datapath *ld;
-    ld = CONTAINER_OF(hmap_first_with_hash(local_datapaths, tunnel_key),
-                      struct local_datapath, hmap_node);
+    struct local_datapath *ld = get_local_datapath(local_datapaths,
+                                                   tunnel_key);
     return ld ? ld->localnet_port : NULL;
 }
 
@@ -255,14 +254,9 @@ physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve,
          */
         uint32_t dp_key = binding->datapath->tunnel_key;
         uint32_t port_key = binding->tunnel_key;
-        struct hmap_node *ld;
-        ld = hmap_first_with_hash(local_datapaths, dp_key);
-        if (!ld) {
-            struct hmap_node *pd;
-            pd = hmap_first_with_hash(patched_datapaths, dp_key);
-            if (!pd) {
-                continue;
-            }
+        if (!get_local_datapath(local_datapaths, dp_key)
+            && !get_patched_datapath(patched_datapaths, dp_key)) {
+            continue;
         }
 
         /* Find the OpenFlow port for the logical port, as 'ofport'.  This is