[ovs-dev] [PATCH ovn v6 1/3] northd: refactor and split some IPAM functions
Dumitru Ceara
dceara at redhat.com
Wed Jan 13 10:37:50 UTC 2021
On 1/12/21 7:39 PM, Mark Michelson wrote:
> This refactor focuses on three efforts:
> * Break a large function into smaller functions
> * Pass more specific data types to functions
> * Isolate these functions in their own code unit
>
> 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.
>
> Note that this is not a full refactor of IPAM. Dynamic address
> assignment is complicated and currently tightly coupled with datapath
> and port constructs. A refactor of that section of IPAM is difficult
> enough that it should have its own patch series separate from this one
> that focuses mostly on unit tests.
>
> Signed-off-by: Mark Michelson <mmichels at redhat.com>
> ---
Hi Mark,
I have a couple of minor comments inline.
Also, as Numan pointed out on a different patch set, it might be good to
follow the coding guidelines (at least) for new files and define
non-static functions before static functions, section "Functions" in:
https://github.com/ovn-org/ovn/blob/master/Documentation/internals/contributing/coding-style.rst
With these addressed:
Acked-by: Dumitru Ceara <dceara at redhat.com>
> northd/automake.mk | 5 +-
> northd/ipam.c | 335 ++++++++++++++++++++++++++++++++++++++++++++
> northd/ipam.h | 39 ++++++
> northd/ovn-northd.c | 291 ++++----------------------------------
> 4 files changed, 403 insertions(+), 267 deletions(-)
> create mode 100644 northd/ipam.c
> create mode 100644 northd/ipam.h
>
> diff --git a/northd/automake.mk b/northd/automake.mk
> index 69657e77e..3340178f5 100644
> --- a/northd/automake.mk
> +++ b/northd/automake.mk
> @@ -1,6 +1,9 @@
> # ovn-northd
> bin_PROGRAMS += northd/ovn-northd
> -northd_ovn_northd_SOURCES = northd/ovn-northd.c
> +northd_ovn_northd_SOURCES = \
> + northd/ovn-northd.c \
> + northd/ipam.c \
> + northd/ipam.h
> northd_ovn_northd_LDADD = \
> lib/libovn.la \
> $(OVSDB_LIBDIR)/libovsdb.la \
> diff --git a/northd/ipam.c b/northd/ipam.c
> new file mode 100644
> index 000000000..c393ba53c
> --- /dev/null
> +++ b/northd/ipam.c
> @@ -0,0 +1,335 @@
> +#include <config.h>
> +
> +#include <stddef.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <netinet/in.h>
> +
> +#include "ipam.h"
> +#include "ovn/lex.h"
> +
> +#include "smap.h"
> +#include "packets.h"
> +#include "bitmap.h"
> +#include "openvswitch/vlog.h"
> +
> +VLOG_DEFINE_THIS_MODULE(ipam)
> +
> +static void
> +init_ipam_ipv6_prefix(const char *ipv6_prefix, struct ipam_info *info)
> +{
> + info->ipv6_prefix_set = false;
> + info->ipv6_prefix = in6addr_any;
> +
> + if (!ipv6_prefix) {
> + return;
> + }
> +
> + /* XXX Since we only accept /64 addresses, why do we even bother
> + * with accepting and trying to analyze a user-provided mask?
> + */
> + 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, "%s: bad 'ipv6_prefix' %s: %s",
> + info->id, ipv6_prefix, error);
> + free(error);
> + } else {
> + 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, "%s: bad 'ipv6_prefix' %s: must be /64",
> + info->id, ipv6_prefix);
> + }
> + }
> + } 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, "%s: bad 'ipv6_prefix' %s", info->id,
> + ipv6_prefix);
> + }
> + }
> +
> + if (info->ipv6_prefix_set) {
> + /* Make sure nothing past first 64 bits are set */
> + struct in6_addr mask = ipv6_create_mask(64);
> + info->ipv6_prefix = ipv6_addr_bitand(&info->ipv6_prefix, &mask);
> + }
> +}
> +
> +static void
> +init_ipam_ipv4(const char *subnet_str, const char *exclude_ip_list,
> + struct ipam_info *info)
> +{
> + info->start_ipv4 = 0;
> + info->total_ipv4s = 0;
> + info->allocated_ipv4s = NULL;
> +
> + if (!subnet_str) {
> + return;
> + }
> +
> + ovs_be32 subnet, mask;
> + char *error = ip_parse_masked(subnet_str, &subnet, &mask);
> + if (error || mask == OVS_BE32_MAX || !ip_is_cidr(mask)) {
> + static struct vlog_rate_limit rl
> + = VLOG_RATE_LIMIT_INIT(5, 1);
> + VLOG_WARN_RL(&rl, "bad 'subnet' %s", subnet_str);
To have consistent warning logs this should be:
VLOG_WARN_RL(&rl, "%s: bad 'subnet' %s", info->id, subnet_str);
> + free(error);
> + return;
> + }
> +
> + 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(info->allocated_ipv4s, 0);
> +
> + if (!exclude_ip_list) {
> + return;
> + }
> +
> + struct lexer lexer;
> + lexer_init(&lexer, exclude_ip_list);
> + /* exclude_ip_list could be in the format -
> + * "10.0.0.4 10.0.0.10 10.0.0.20..10.0.0.50 10.0.0.100..10.0.0.110".
> + */
> + lexer_get(&lexer);
> + while (lexer.token.type != LEX_T_END) {
> + if (lexer.token.type != LEX_T_INTEGER) {
> + lexer_syntax_error(&lexer, "expecting address");
> + break;
> + }
> + uint32_t start = ntohl(lexer.token.value.ipv4);
> + lexer_get(&lexer);
> +
> + uint32_t end = start + 1;
> + if (lexer_match(&lexer, LEX_T_ELLIPSIS)) {
> + if (lexer.token.type != LEX_T_INTEGER) {
> + lexer_syntax_error(&lexer, "expecting address range");
> + break;
> + }
> + end = ntohl(lexer.token.value.ipv4) + 1;
> + lexer_get(&lexer);
> + }
> +
> + /* Clamp start...end to fit the subnet. */
> + start = MAX(info->start_ipv4, start);
> + end = MIN(info->start_ipv4 + info->total_ipv4s, end);
> + if (end > start) {
> + bitmap_set_multiple(info->allocated_ipv4s,
> + start - info->start_ipv4,
> + end - start, 1);
> + } else {
> + lexer_error(&lexer, "excluded addresses not in subnet");
> + }
> + }
> + 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);
> + */
The comment can be removed.
> + VLOG_WARN_RL(&rl, "logical switch: bad exclude_ips (%s)", lexer.error);
This could be:
VLOG_WARN_RL(&rl, "%s: bad exclude_ips (%s)", lexer.error, info->id);
Thanks,
Dumitru
More information about the dev
mailing list