From patchwork Fri Oct 9 15:29:27 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Mark Michelson X-Patchwork-Id: 1379265 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=openvswitch.org (client-ip=140.211.166.136; helo=silver.osuosl.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=) Authentication-Results: ozlabs.org; dmarc=fail (p=none dis=none) header.from=redhat.com Authentication-Results: ozlabs.org; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=cu6jfzmS; dkim-atps=neutral Received: from silver.osuosl.org (smtp3.osuosl.org [140.211.166.136]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 4C7Bp23bZbz9sTc for ; Sat, 10 Oct 2020 02:29:46 +1100 (AEDT) Received: from localhost (localhost [127.0.0.1]) by silver.osuosl.org (Postfix) with ESMTP id EE3672E2AC; Fri, 9 Oct 2020 15:29:42 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from silver.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id DJ3j2lKxd8f8; Fri, 9 Oct 2020 15:29:37 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by silver.osuosl.org (Postfix) with ESMTP id 98DAD2E2A6; Fri, 9 Oct 2020 15:29:37 +0000 (UTC) Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id 8BA85C0890; Fri, 9 Oct 2020 15:29:37 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@lists.linuxfoundation.org Received: from whitealder.osuosl.org (smtp1.osuosl.org [140.211.166.138]) by lists.linuxfoundation.org (Postfix) with ESMTP id 788ABC07FF for ; Fri, 9 Oct 2020 15:29:36 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by whitealder.osuosl.org (Postfix) with ESMTP id 7259487677 for ; Fri, 9 Oct 2020 15:29:36 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from whitealder.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 3V+nOq6XZtem for ; Fri, 9 Oct 2020 15:29:35 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [63.128.21.124]) by whitealder.osuosl.org (Postfix) with ESMTPS id 4473B876D7 for ; Fri, 9 Oct 2020 15:29:35 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1602257374; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=sokM9Ewqpe9Hqd1Dr1V1gVt4b9L6GrMILAJSp9I/a4Y=; b=cu6jfzmS4p1f4EyZOrwU6APIq/NZNksQ0uQbNEvP/0fq7UoamPPXKDnpwkFGKEWAji3SoP ZIpydXLKmQYB00YubVU2fc/Hh9/qUxwLbiEllsoLL/QvnbGwt4krAuAZoV8HOEyKlTdDEv Vn9sHqOmchhiO3F1bgMTciwlxJFphi8= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-592-PE5sXD4hNUiJfRXdVYSx8g-1; Fri, 09 Oct 2020 11:29:32 -0400 X-MC-Unique: PE5sXD4hNUiJfRXdVYSx8g-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id A3DCC18A8238 for ; Fri, 9 Oct 2020 15:29:31 +0000 (UTC) Received: from monae.redhat.com (ovpn-115-79.rdu2.redhat.com [10.10.115.79]) by smtp.corp.redhat.com (Postfix) with ESMTP id 3286873662 for ; Fri, 9 Oct 2020 15:29:31 +0000 (UTC) From: Mark Michelson To: dev@openvswitch.org Date: Fri, 9 Oct 2020 11:29:27 -0400 Message-Id: <20201009152928.1312612-3-mmichels@redhat.com> In-Reply-To: <20201009152928.1312612-1-mmichels@redhat.com> References: <20201009152928.1312612-1-mmichels@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=mmichels@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Subject: [ovs-dev] [RFC PATCH ovn 2/3] northd: refactor init_ipam_info_for_datapath X-BeenThere: ovs-dev@openvswitch.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: ovs-dev-bounces@openvswitch.org Sender: "dev" 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 --- northd/ovn-northd.c | 186 ++++++++++++++++++++++++++------------------ 1 file changed, 110 insertions(+), 76 deletions(-) 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)) {