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 <[email protected]> > ---
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 <[email protected]> > 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 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
