diff mbox series

[ovs-dev,RFC,2/3] northd: refactor init_ipam_info_for_datapath

Message ID 20201009152928.1312612-3-mmichels@redhat.com
State Superseded
Headers show
Series Unit Testing in OVN | expand

Commit Message

Mark Michelson Oct. 9, 2020, 3:29 p.m. UTC
This refactor focuses on two efforts:
* Break a large function into smaller functions
* Pass more specific data types to functions

Smaller functions have clearer purposes, have fewer chances of unwanted
side effects, and are easier to test. The next commit in this series
will add some unit tests that exercise the new functions created in
this commit.

Signed-off-by: Mark Michelson <mmichels@redhat.com>
---
 northd/ovn-northd.c | 186 ++++++++++++++++++++++++++------------------
 1 file changed, 110 insertions(+), 76 deletions(-)
diff mbox series

Patch

diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index 8bab9b047..d989ddf53 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -893,57 +893,49 @@  lrouter_is_enabled(const struct nbrec_logical_router *lrouter)
 }
 
 static void
-init_ipam_info_for_datapath(struct ovn_datapath *od)
+init_ipam_ipv6_prefix(const char *ipv6_prefix, struct ipam_info *info)
 {
-    if (!od->nbs) {
+    if (!ipv6_prefix) {
         return;
     }
 
-    const char *subnet_str = smap_get(&od->nbs->other_config, "subnet");
-    const char *ipv6_prefix = smap_get(&od->nbs->other_config, "ipv6_prefix");
-
-    if (ipv6_prefix) {
-        if (strstr(ipv6_prefix, "/")) {
-            /* If a prefix length was specified, it must be 64. */
-            struct in6_addr mask;
-            char *error
-                = ipv6_parse_masked(ipv6_prefix,
-                                    &od->ipam_info.ipv6_prefix, &mask);
-            if (error) {
-                static struct vlog_rate_limit rl
-                    = VLOG_RATE_LIMIT_INIT(5, 1);
-                VLOG_WARN_RL(&rl, "bad 'ipv6_prefix' %s: %s",
-                             ipv6_prefix, error);
-                free(error);
-            } else {
-                if (ipv6_count_cidr_bits(&mask) == 64) {
-                    od->ipam_info.ipv6_prefix_set = true;
-                } else {
-                    static struct vlog_rate_limit rl
-                        = VLOG_RATE_LIMIT_INIT(5, 1);
-                    VLOG_WARN_RL(&rl, "bad 'ipv6_prefix' %s: must be /64",
-                                 ipv6_prefix);
-                }
-            }
+    if (strchr(ipv6_prefix, '/')) {
+        /* If a prefix length was specified, it must be 64. */
+        struct in6_addr mask;
+        char *error
+            = ipv6_parse_masked(ipv6_prefix,
+                                &info->ipv6_prefix, &mask);
+        if (error) {
+            static struct vlog_rate_limit rl
+                = VLOG_RATE_LIMIT_INIT(5, 1);
+            VLOG_WARN_RL(&rl, "bad 'ipv6_prefix' %s: %s",
+                         ipv6_prefix, error);
+            free(error);
         } else {
-            od->ipam_info.ipv6_prefix_set = ipv6_parse(
-                ipv6_prefix, &od->ipam_info.ipv6_prefix);
-            if (!od->ipam_info.ipv6_prefix_set) {
+            if (ipv6_count_cidr_bits(&mask) == 64) {
+                info->ipv6_prefix_set = true;
+            } else {
                 static struct vlog_rate_limit rl
                     = VLOG_RATE_LIMIT_INIT(5, 1);
-                VLOG_WARN_RL(&rl, "bad 'ipv6_prefix' %s", ipv6_prefix);
+                VLOG_WARN_RL(&rl, "bad 'ipv6_prefix' %s: must be /64",
+                             ipv6_prefix);
             }
         }
-    }
-
-    if (!subnet_str) {
-        if (!ipv6_prefix) {
-            od->ipam_info.mac_only = smap_get_bool(&od->nbs->other_config,
-                                                   "mac_only", false);
+    } else {
+        info->ipv6_prefix_set = ipv6_parse(
+            ipv6_prefix, &info->ipv6_prefix);
+        if (!info->ipv6_prefix_set) {
+            static struct vlog_rate_limit rl
+                = VLOG_RATE_LIMIT_INIT(5, 1);
+            VLOG_WARN_RL(&rl, "bad 'ipv6_prefix' %s", ipv6_prefix);
         }
-        return;
     }
+}
 
+static void
+init_ipam_ipv4(const char *subnet_str, const char *exclude_ip_list,
+               struct ipam_info *info)
+{
     ovs_be32 subnet, mask;
     char *error = ip_parse_masked(subnet_str, &subnet, &mask);
     if (error || mask == OVS_BE32_MAX || !ip_is_cidr(mask)) {
@@ -954,17 +946,14 @@  init_ipam_info_for_datapath(struct ovn_datapath *od)
         return;
     }
 
-    od->ipam_info.start_ipv4 = ntohl(subnet & mask) + 1;
-    od->ipam_info.total_ipv4s = ~ntohl(mask);
-    od->ipam_info.allocated_ipv4s =
-        bitmap_allocate(od->ipam_info.total_ipv4s);
+    info->start_ipv4 = ntohl(subnet & mask) + 1;
+    info->total_ipv4s = ~ntohl(mask);
+    info->allocated_ipv4s =
+        bitmap_allocate(info->total_ipv4s);
 
     /* Mark first IP as taken */
-    bitmap_set1(od->ipam_info.allocated_ipv4s, 0);
+    bitmap_set1(info->allocated_ipv4s, 0);
 
-    /* Check if there are any reserver IPs (list) to be excluded from IPAM */
-    const char *exclude_ip_list = smap_get(&od->nbs->other_config,
-                                           "exclude_ips");
     if (!exclude_ip_list) {
         return;
     }
@@ -994,11 +983,11 @@  init_ipam_info_for_datapath(struct ovn_datapath *od)
         }
 
         /* Clamp start...end to fit the subnet. */
-        start = MAX(od->ipam_info.start_ipv4, start);
-        end = MIN(od->ipam_info.start_ipv4 + od->ipam_info.total_ipv4s, end);
+        start = MAX(info->start_ipv4, start);
+        end = MIN(info->start_ipv4 + info->total_ipv4s, end);
         if (end > start) {
-            bitmap_set_multiple(od->ipam_info.allocated_ipv4s,
-                                start - od->ipam_info.start_ipv4,
+            bitmap_set_multiple(info->allocated_ipv4s,
+                                start - info->start_ipv4,
                                 end - start, 1);
         } else {
             lexer_error(&lexer, "excluded addresses not in subnet");
@@ -1006,12 +995,44 @@  init_ipam_info_for_datapath(struct ovn_datapath *od)
     }
     if (lexer.error) {
         static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
-        VLOG_WARN_RL(&rl, "logical switch "UUID_FMT": bad exclude_ips (%s)",
-                     UUID_ARGS(&od->key), lexer.error);
+        /*
+         * VLOG_WARN_RL(&rl, "logical switch "UUID_FMT": bad exclude_ips (%s)",
+         *              UUID_ARGS(&od->key), lexer.error);
+         */
+        VLOG_WARN_RL(&rl, "logical switch: bad exclude_ips (%s)", lexer.error);
     }
     lexer_destroy(&lexer);
 }
 
+static void
+init_ipam_info(struct ipam_info *info, const struct smap *config)
+{
+    const char *subnet_str = smap_get(config, "subnet");
+    const char *ipv6_prefix = smap_get(config, "ipv6_prefix");
+    const char *exclude_ips = smap_get(config, "exclude_ips");
+
+    init_ipam_ipv6_prefix(ipv6_prefix, info);
+
+    if (!subnet_str) {
+        if (!ipv6_prefix) {
+            info->mac_only = smap_get_bool(config, "mac_only", false);
+        }
+        return;
+    }
+
+    init_ipam_ipv4(subnet_str, exclude_ips, info); 
+}
+
+static void
+init_ipam_info_for_datapath(struct ovn_datapath *od)
+{
+    if (!od->nbs) {
+        return;
+    }
+
+    init_ipam_info(&od->ipam_info, &od->nbs->other_config);
+}
+
 static void
 init_mcast_info_for_router_datapath(struct ovn_datapath *od)
 {
@@ -1578,23 +1599,36 @@  ipam_insert_mac(struct eth_addr *ea, bool check)
     hmap_insert(&macam, &new_macam_node->hmap_node, hash_uint64(mac64));
 }
 
+static bool
+ipam_insert_ip(struct ipam_info *info, uint32_t ip)
+{
+    if (!info->allocated_ipv4s) {
+        return true;
+    }
+
+    if (ip >= info->start_ipv4 &&
+        ip < (info->start_ipv4 + info->total_ipv4s)) {
+        if (bitmap_is_set(info->allocated_ipv4s,
+                          ip - info->start_ipv4)) {
+            return false;
+        }
+        bitmap_set1(info->allocated_ipv4s,
+                    ip - info->start_ipv4);
+    }
+    return true;
+}
+
 static void
-ipam_insert_ip(struct ovn_datapath *od, uint32_t ip)
+ipam_insert_ip_for_datapath(struct ovn_datapath *od, uint32_t ip)
 {
-    if (!od || !od->ipam_info.allocated_ipv4s) {
+    if (!od) {
         return;
     }
 
-    if (ip >= od->ipam_info.start_ipv4 &&
-        ip < (od->ipam_info.start_ipv4 + od->ipam_info.total_ipv4s)) {
-        if (bitmap_is_set(od->ipam_info.allocated_ipv4s,
-                          ip - od->ipam_info.start_ipv4)) {
-            static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
-            VLOG_WARN_RL(&rl, "Duplicate IP set on switch %s: "IP_FMT,
-                         od->nbs->name, IP_ARGS(htonl(ip)));
-        }
-        bitmap_set1(od->ipam_info.allocated_ipv4s,
-                    ip - od->ipam_info.start_ipv4);
+    if (!ipam_insert_ip(&od->ipam_info, ip)) {
+        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
+        VLOG_WARN_RL(&rl, "Duplicate IP set on switch %s: "IP_FMT,
+                     od->nbs->name, IP_ARGS(htonl(ip)));
     }
 }
 
@@ -1624,7 +1658,7 @@  ipam_insert_lsp_addresses(struct ovn_datapath *od, struct ovn_port *op,
 
     for (size_t j = 0; j < laddrs.n_ipv4_addrs; j++) {
         uint32_t ip = ntohl(laddrs.ipv4_addrs[j].addr);
-        ipam_insert_ip(od, ip);
+        ipam_insert_ip_for_datapath(od, ip);
     }
 
     destroy_lport_addresses(&laddrs);
@@ -1666,7 +1700,7 @@  ipam_add_port_addresses(struct ovn_datapath *od, struct ovn_port *op)
              * about a duplicate IP address.
              */
             if (ip != op->peer->od->ipam_info.start_ipv4) {
-                ipam_insert_ip(op->peer->od, ip);
+                ipam_insert_ip_for_datapath(op->peer->od, ip);
             }
         }
 
@@ -1701,21 +1735,21 @@  ipam_get_unused_mac(ovs_be32 ip)
 }
 
 static uint32_t
-ipam_get_unused_ip(struct ovn_datapath *od)
+ipam_get_unused_ip(struct ipam_info *info)
 {
-    if (!od || !od->ipam_info.allocated_ipv4s) {
+    if (!info->allocated_ipv4s) {
         return 0;
     }
 
-    size_t new_ip_index = bitmap_scan(od->ipam_info.allocated_ipv4s, 0, 0,
-                                      od->ipam_info.total_ipv4s - 1);
-    if (new_ip_index == od->ipam_info.total_ipv4s - 1) {
+    size_t new_ip_index = bitmap_scan(info->allocated_ipv4s, 0, 0,
+                                      info->total_ipv4s - 1);
+    if (new_ip_index == info->total_ipv4s - 1) {
         static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
         VLOG_WARN_RL( &rl, "Subnet address space has been exhausted.");
         return 0;
     }
 
-    return od->ipam_info.start_ipv4 + new_ip_index;
+    return info->start_ipv4 + new_ip_index;
 }
 
 enum dynamic_update_type {
@@ -1919,7 +1953,7 @@  update_unchanged_dynamic_addresses(struct dynamic_address_update *update)
         ipam_insert_mac(&update->current_addresses.ea, false);
     }
     if (update->ipv4 == NONE && update->current_addresses.n_ipv4_addrs) {
-        ipam_insert_ip(update->op->od,
+        ipam_insert_ip_for_datapath(update->op->od,
                        ntohl(update->current_addresses.ipv4_addrs[0].addr));
     }
 }
@@ -1999,7 +2033,7 @@  update_dynamic_addresses(struct dynamic_address_update *update)
         ip4 = update->static_ip;
         break;
     case DYNAMIC:
-        ip4 = htonl(ipam_get_unused_ip(update->od));
+        ip4 = htonl(ipam_get_unused_ip(&update->od->ipam_info));
         VLOG_INFO("Assigned dynamic IPv4 address '"IP_FMT"' to port '%s'",
                   IP_ARGS(ip4), update->op->nbsp->name);
     }
@@ -2048,7 +2082,7 @@  update_dynamic_addresses(struct dynamic_address_update *update)
     ipam_insert_mac(&mac, true);
 
     if (ip4) {
-        ipam_insert_ip(update->od, ntohl(ip4));
+        ipam_insert_ip_for_datapath(update->od, ntohl(ip4));
         ds_put_format(&new_addr, " "IP_FMT, IP_ARGS(ip4));
     }
     if (!IN6_ARE_ADDR_EQUAL(&ip6, &in6addr_any)) {