[PATCH 1/7] ip-pool: Track IPv4 addresses in use

[PATCH 1/7] ip-pool: Track IPv4 addresses in use

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 6544 bytes --]

Add the ip-pool submodule that tracks IPv4 addresses in use on the
system for use when selecting the address for a new AP.
---
 Makefile.am   |   1 +
 src/ip-pool.c | 160 ++++++++++++++++++++++++++++++++++++++++++++++++++
 src/ip-pool.h |  32 ++++++++++
 3 files changed, 193 insertions(+)
 create mode 100644 src/ip-pool.c
 create mode 100644 src/ip-pool.h

diff --git a/Makefile.am b/Makefile.am
index 68035e46..083bc95a 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -243,6 +243,7 @@ src_iwd_SOURCES = src/main.c linux/nl80211.h src/iwd.h src/missing.h \
 					src/eap-wsc.c src/eap-wsc.h \
 					src/wscutil.h src/wscutil.c \
 					src/diagnostic.h src/diagnostic.c \
+					src/ip-pool.h src/ip-pool.c \
 					$(eap_sources) \
 					$(builtin_sources)
 
diff --git a/src/ip-pool.c b/src/ip-pool.c
new file mode 100644
index 00000000..1166383c
--- /dev/null
+++ b/src/ip-pool.c
@@ -0,0 +1,160 @@
+/*
+ *
+ *  Wireless daemon for Linux
+ *
+ *  Copyright (C) 2021  Intel Corporation. All rights reserved.
+ *
+ *  This library is free software; you can redistribute it and/or
+ *  modify it under the terms of the GNU Lesser General Public
+ *  License as published by the Free Software Foundation; either
+ *  version 2.1 of the License, or (at your option) any later version.
+ *
+ *  This library is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ *  Lesser General Public License for more details.
+ *
+ *  You should have received a copy of the GNU Lesser General Public
+ *  License along with this library; if not, write to the Free Software
+ *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ */
+
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#include <errno.h>
+#include <netinet/in.h>
+#include <arpa/inet.h>
+
+#include <ell/ell.h>
+
+#include "src/util.h"
+#include "src/iwd.h"
+#include "src/module.h"
+#include "src/netdev.h"
+#include "src/ip-pool.h"
+
+static struct l_queue *used_addr4_list;
+static struct l_netlink *rtnl;
+
+static bool ip_pool_addr4_match_ifindex(const void *a, const void *b)
+{
+	const struct ip_pool_addr4_record *addr = a;
+
+	return addr->ifindex == L_PTR_TO_UINT(b) && !addr->secondary;
+}
+
+const struct ip_pool_addr4_record *ip_pool_get_addr4(struct netdev *netdev)
+{
+	return l_queue_find(used_addr4_list, ip_pool_addr4_match_ifindex,
+				L_UINT_TO_PTR(netdev_get_ifindex(netdev)));
+}
+
+static bool ip_pool_addr4_match_free(void *data, void *user_data)
+{
+	const struct ip_pool_addr4_record *a = data;
+	const struct ip_pool_addr4_record *b = user_data;
+
+	if (a->ifindex != b->ifindex || a->addr != b->addr ||
+			a->prefix_len != b->prefix_len)
+		return false;
+
+	l_free(data);
+	return true;
+}
+
+static void ip_pool_addr_notify(uint16_t type, const void *data, uint32_t len,
+				void *user_data)
+{
+	const struct ifaddrmsg *ifa = data;
+	char *ip_str = NULL;
+	int r;
+	struct in_addr ip;
+
+	if (ifa->ifa_family != AF_INET || ifa->ifa_prefixlen < 1)
+		return;
+
+	len -= NLMSG_ALIGN(sizeof(struct ifaddrmsg));
+
+	l_rtnl_ifaddr4_extract(ifa, len, NULL, &ip_str, NULL);
+	if (!ip_str)
+		return;
+
+	r = inet_aton(ip_str, &ip);
+	l_free(ip_str);
+
+	if (r < 0)
+		return;
+
+	if (type == RTM_NEWADDR) {
+		struct ip_pool_addr4_record *addr;
+
+		addr = l_new(struct ip_pool_addr4_record, 1);
+		addr->addr = ntohl(ip.s_addr);
+		addr->prefix_len = ifa->ifa_prefixlen;
+		addr->ifindex = ifa->ifa_index;
+		addr->secondary = !!(ifa->ifa_flags & IFA_F_SECONDARY);
+		l_queue_push_tail(used_addr4_list, addr);
+	} else if (type == RTM_DELADDR) {
+		struct ip_pool_addr4_record addr;
+
+		addr.addr = ntohl(ip.s_addr);
+		addr.prefix_len = ifa->ifa_prefixlen;
+		addr.ifindex = ifa->ifa_index;
+
+		l_queue_foreach_remove(used_addr4_list,
+					ip_pool_addr4_match_free, &addr);
+	}
+}
+
+static void ip_pool_addr4_dump_cb(int error,
+					uint16_t type, const void *data,
+					uint32_t len, void *user_data)
+{
+	if (error) {
+		l_error("addr4_dump_cb: %s (%i)", strerror(-error), -error);
+		return;
+	}
+
+	ip_pool_addr_notify(type, data, len, user_data);
+}
+
+static int ip_pool_init(void)
+{
+	const struct l_settings *settings = iwd_get_config();
+	bool netconfig_enabled;
+
+	if (!l_settings_get_bool(settings, "General",
+				"EnableNetworkConfiguration",
+				&netconfig_enabled))
+		netconfig_enabled = false;
+
+	if (!netconfig_enabled)
+		return 0;
+
+	rtnl = iwd_get_rtnl();
+
+	if (!l_netlink_register(rtnl, RTNLGRP_IPV4_IFADDR,
+				ip_pool_addr_notify, NULL, NULL)) {
+		l_error("Failed to register for RTNL link notifications");
+		return -EIO;
+	}
+
+	if (!l_rtnl_ifaddr4_dump(rtnl, ip_pool_addr4_dump_cb, NULL, NULL)) {
+		l_error("Sending the IPv4 addr dump req failed");
+		return -EIO;
+	}
+
+	used_addr4_list = l_queue_new();
+	return 0;
+}
+
+static void ip_pool_exit(void)
+{
+	l_queue_destroy(used_addr4_list, l_free);
+	used_addr4_list = NULL;
+}
+
+IWD_MODULE(ip_pool, ip_pool_init, ip_pool_exit)
diff --git a/src/ip-pool.h b/src/ip-pool.h
new file mode 100644
index 00000000..6a220735
--- /dev/null
+++ b/src/ip-pool.h
@@ -0,0 +1,32 @@
+/*
+ *
+ *  Wireless daemon for Linux
+ *
+ *  Copyright (C) 2021  Intel Corporation. All rights reserved.
+ *
+ *  This library is free software; you can redistribute it and/or
+ *  modify it under the terms of the GNU Lesser General Public
+ *  License as published by the Free Software Foundation; either
+ *  version 2.1 of the License, or (at your option) any later version.
+ *
+ *  This library is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ *  Lesser General Public License for more details.
+ *
+ *  You should have received a copy of the GNU Lesser General Public
+ *  License along with this library; if not, write to the Free Software
+ *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ */
+
+struct netdev;
+
+struct ip_pool_addr4_record {
+	uint32_t addr;
+	uint8_t prefix_len;
+	uint32_t ifindex;
+	bool secondary;
+};
+
+const struct ip_pool_addr4_record *ip_pool_get_addr4(struct netdev *netdev);
-- 
2.27.0

[PATCH 2/7] ip-pool: Add subnet address selection logic

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 7792 bytes --]

Add the ip_pool_select_addr4 function to select a random subnet of requested
size from an address space defined by a string list (for use with the
AP profile [IPv4].Address and the global [IPv4].APAddressPool settings),
avoiding those subnets that conflict with subnets in use.  We take care
to give a similar weight to all subnets contained in the specified
ranges regardless of how many ranges contain each, basically so that
overlapping ranges don't affect the probabilities (debatable.)
---
 src/ip-pool.c | 207 ++++++++++++++++++++++++++++++++++++++++++++++++++
 src/ip-pool.h |   2 +
 2 files changed, 209 insertions(+)

diff --git a/src/ip-pool.c b/src/ip-pool.c
index 1166383c..88428eb6 100644
--- a/src/ip-pool.c
+++ b/src/ip-pool.c
@@ -36,9 +36,216 @@
 #include "src/netdev.h"
 #include "src/ip-pool.h"
 
+struct ip_pool_addr4_range {
+	uint32_t start;
+	uint32_t end;
+};
+
 static struct l_queue *used_addr4_list;
 static struct l_netlink *rtnl;
 
+static int ip_pool_addr4_range_compare(const void *a, const void *b,
+					void *user_data)
+{
+	const struct ip_pool_addr4_range *range_a = a;
+	const struct ip_pool_addr4_range *range_b = b;
+
+	return range_a->start > range_b->start ? 1 : -1;
+}
+
+/*
+ * Append any address ranges within an input start/end range which contain
+ * at least one full subnet and don't intersect with any subnets already in
+ * use.  This may result in the input range being split into multiple ranges
+ * of different sizes or being skipped altogether.
+ * All inputs must be rounded to the subnet boundary and the @used queue
+ * sorted by the subnet start address.
+ */
+static void ip_pool_append_range(struct l_queue *to,
+					const struct ip_pool_addr4_range *range,
+					struct l_queue *used, const char *str)
+{
+	const struct l_queue_entry *entry = l_queue_get_entries(used);
+	const struct ip_pool_addr4_range *used_range =
+		entry ? entry->data : NULL;
+	uint32_t start = range->start;
+	bool print = true;
+
+	while (range->end > start) {
+		while (used_range && used_range->end <= start) {
+			entry = entry->next;
+			used_range = entry ? entry->data : NULL;
+		}
+
+		/* No more used ranges that intersect with @start/@range->end */
+		if (!used_range || range->end <= used_range->start) {
+			struct ip_pool_addr4_range *sub =
+				l_new(struct ip_pool_addr4_range, 1);
+
+			sub->start = start;
+			sub->end = range->end;
+			l_queue_push_tail(to, sub);
+			l_queue_insert(used, l_memdup(sub, sizeof(*sub)),
+					ip_pool_addr4_range_compare, NULL);
+			return;
+		}
+
+		if (print) {
+			l_debug("Address spec %s intersects with at least one "
+				"subnet already in use on the system or "
+				"specified in the settings", str);
+			print = false;
+		}
+
+		/* Now we know @used_range is non-NULL and intersects */
+		if (start < used_range->start) {
+			struct ip_pool_addr4_range *sub =
+				l_new(struct ip_pool_addr4_range, 1);
+
+			sub->start = start;
+			sub->end = used_range->start;
+			l_queue_push_tail(to, sub);
+			l_queue_insert(used, l_memdup(sub, sizeof(*sub)),
+					ip_pool_addr4_range_compare, NULL);
+		}
+
+		/* Skip to the start of the next subnet */
+		start = used_range->end;
+	}
+}
+
+/*
+ * Select a subnet and a host address from a defined space.
+ *
+ * Returns:  0 when an address was selected and written to *out_addr,
+ *          -EEXIST if all available subnet addresses are in use,
+ *          -EINVAL if there was a different error.
+ */
+int ip_pool_select_addr4(const char **addr_str_list, uint8_t prefix,
+				struct netdev *netdev, uint32_t *out_addr)
+{
+	uint32_t total = 0;
+	uint32_t selected;
+	unsigned int i;
+	uint32_t subnet_size = 1 << (32 - prefix);
+	uint32_t host_mask = subnet_size - 1;
+	uint32_t subnet_mask = ~host_mask;
+	uint32_t host_addr = 0;
+	struct l_queue *ranges = l_queue_new();
+	struct l_queue *used = l_queue_new();
+	struct in_addr ia;
+	const struct l_queue_entry *entry;
+	uint32_t ifindex = netdev ? netdev_get_ifindex(netdev) : 0;
+	int err = -EINVAL;
+
+	/* Build a sorted list of used/unavailable subnets */
+	for (entry = l_queue_get_entries((struct l_queue *) used_addr4_list);
+			entry; entry = entry->next) {
+		const struct ip_pool_addr4_record *rec = entry->data;
+		struct ip_pool_addr4_range *range;
+
+		/*
+		 * Don't add current subnets from the target interface to the used
+		 * subnet address list because we'll be replacing them (or adding
+		 * another secondary address) so there won't be a routing conflict.
+		 */
+		if (rec->ifindex == ifindex)
+			continue;
+
+		if (!rec->addr || !rec->prefix_len)
+			continue;
+
+		range = l_new(struct ip_pool_addr4_range, 1);
+		range->start = rec->addr & subnet_mask;
+		range->end = (range->start + (1 << (32 - rec->prefix_len)) +
+				subnet_size - 1) & subnet_mask;
+		l_queue_insert(used, range, ip_pool_addr4_range_compare, NULL);
+	}
+
+	/* Build the list of available subnets */
+
+	/* Check for the static IP syntax: Address=<IP> */
+	if (l_strv_length((char **) addr_str_list) == 1 &&
+			inet_pton(AF_INET, *addr_str_list, &ia) == 1) {
+		struct ip_pool_addr4_range range;
+
+		host_addr = ntohl(ia.s_addr);
+		range.start = host_addr & subnet_mask;
+		range.end = range.start + subnet_size;
+		ip_pool_append_range(ranges, &range, used, *addr_str_list);
+		goto check_avail;
+	}
+
+	for (i = 0; addr_str_list[i]; i++) {
+		struct ip_pool_addr4_range range;
+		uint32_t addr;
+		uint8_t addr_prefix;
+
+		if (!util_ip_prefix_tohl(addr_str_list[i], &addr_prefix, &addr,
+						NULL, NULL)) {
+			l_error("Can't parse %s as a subnet address",
+				addr_str_list[i]);
+			goto cleanup;
+		}
+
+		if (addr_prefix > prefix) {
+			l_debug("Address spec %s smaller than requested "
+				"subnet (prefix len %i)", addr_str_list[i],
+				prefix);
+			continue;
+		}
+
+		range.start = addr & subnet_mask;
+		range.end = range.start + (1 << (32 - addr_prefix));
+		ip_pool_append_range(ranges, &range, used, addr_str_list[i]);
+	}
+
+check_avail:
+	if (l_queue_isempty(ranges)) {
+		l_error("No IP subnets available for new Access Point after "
+			"eliminating those already in use on the system");
+		err = -EEXIST;
+		goto cleanup;
+	}
+
+	if (host_addr)
+		goto done;
+
+	/* Count available @prefix-sized subnets */
+	for (entry = l_queue_get_entries(ranges); entry; entry = entry->next) {
+		struct ip_pool_addr4_range *range = entry->data;
+
+		total += (range->end - range->start) >> (32 - prefix);
+	}
+
+	selected = l_getrandom_uint32() % total;
+
+	/* Find the @selected'th @prefix-sized subnet */
+	for (entry = l_queue_get_entries(ranges);; entry = entry->next) {
+		struct ip_pool_addr4_range *range = entry->data;
+		uint32_t count = (range->end - range->start) >> (32 - prefix);
+
+		if (selected < count) {
+			host_addr = range->start + (selected << (32 - prefix));
+			break;
+		}
+
+		selected -= count;
+	}
+
+	if ((host_addr & 0xff) == 0)
+		host_addr += 1;
+
+done:
+	err = 0;
+	*out_addr = host_addr;
+
+cleanup:
+	l_queue_destroy(ranges, l_free);
+	l_queue_destroy(used, l_free);
+	return err;
+}
+
 static bool ip_pool_addr4_match_ifindex(const void *a, const void *b)
 {
 	const struct ip_pool_addr4_record *addr = a;
diff --git a/src/ip-pool.h b/src/ip-pool.h
index 6a220735..8c75d03e 100644
--- a/src/ip-pool.h
+++ b/src/ip-pool.h
@@ -29,4 +29,6 @@ struct ip_pool_addr4_record {
 	bool secondary;
 };
 
+int ip_pool_select_addr4(const char **addr_str_list, uint8_t prefix,
+				struct netdev *netdev, uint32_t *out_addr);
 const struct ip_pool_addr4_record *ip_pool_get_addr4(struct netdev *netdev);
-- 
2.27.0

Re: [PATCH 2/7] ip-pool: Add subnet address selection logic

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1693 bytes --]

Hi Andrew,

On 5/25/21 7:55 AM, Andrew Zaborowski wrote:
> Add the ip_pool_select_addr4 function to select a random subnet of requested
> size from an address space defined by a string list (for use with the
> AP profile [IPv4].Address and the global [IPv4].APAddressPool settings),
> avoiding those subnets that conflict with subnets in use.  We take care
> to give a similar weight to all subnets contained in the specified
> ranges regardless of how many ranges contain each, basically so that
> overlapping ranges don't affect the probabilities (debatable.)
> ---
>   src/ip-pool.c | 207 ++++++++++++++++++++++++++++++++++++++++++++++++++
>   src/ip-pool.h |   2 +
>   2 files changed, 209 insertions(+)
> 

<snip>

> diff --git a/src/ip-pool.h b/src/ip-pool.h
> index 6a220735..8c75d03e 100644
> --- a/src/ip-pool.h
> +++ b/src/ip-pool.h
> @@ -29,4 +29,6 @@ struct ip_pool_addr4_record {
>   	bool secondary;
>   };
>   
> +int ip_pool_select_addr4(const char **addr_str_list, uint8_t prefix,
> +				struct netdev *netdev, uint32_t *out_addr);
>   const struct ip_pool_addr4_record *ip_pool_get_addr4(struct netdev *netdev);
> 

Similarly to previous patch, do we really need the netdev object here?

Also, what is 'prefix'?  It looks like it is used to convey the desired prefix 
length size.  So should it be named appropriately?  Maybe target_prefix_len?

Do you really need the netdev argument? It looks like it is used to skip adding 
that netdev's addresses to the 'used' list.  Why?  How likely is this to happen? 
  Can't we just leave those alone and assign a new subnet regardless? Or have AP 
wipe the addresses first?

Regards,
-Denis

Re: [PATCH 2/7] ip-pool: Add subnet address selection logic

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 522 bytes --]

Hi Andrew,

>> +int ip_pool_select_addr4(const char **addr_str_list, uint8_t prefix,
>> +                struct netdev *netdev, uint32_t *out_addr);
>>   const struct ip_pool_addr4_record *ip_pool_get_addr4(struct netdev *netdev);
>>
> 

Oh, and another thing.  Can this just return us an l_rtnl_address? The 
address/prefix is known and that is basically all you need for the 
l_rtnl_address object. That way it can be used directly in l_rtnl_ifaddr_add / 
delete functions.

Regards,
-Denis

Re: [PATCH 2/7] ip-pool: Add subnet address selection logic

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 1326 bytes --]

On Wed, 26 May 2021 at 21:56, Denis Kenzior <denkenz@gmail.com> wrote:
> On 5/25/21 7:55 AM, Andrew Zaborowski wrote:
> > +int ip_pool_select_addr4(const char **addr_str_list, uint8_t prefix,
> > +                             struct netdev *netdev, uint32_t *out_addr);
> >   const struct ip_pool_addr4_record *ip_pool_get_addr4(struct netdev *netdev);
> >
>
> Similarly to previous patch, do we really need the netdev object here?
>
> Also, what is 'prefix'?  It looks like it is used to convey the desired prefix
> length size.  So should it be named appropriately?  Maybe target_prefix_len?

Yep, I'll fix this.

>
> Do you really need the netdev argument? It looks like it is used to skip adding
> that netdev's addresses to the 'used' list.  Why?  How likely is this to happen?
>   Can't we just leave those alone and assign a new subnet regardless?

Sure, we can drop this functionality.  It sounded like a correct thing
to do but if there's a problem with it I can drop it.  Note we'll save
exactly 3 non-empty-non-comment lines of code.

> Or have AP
> wipe the addresses first?

Now that would be trading an integer comparison (the ifindex check..)
for a multi-step asynchronous operation with cancellation, error
checks, etc., I wouldn't do this to save that one argument.

Best regards

[PATCH 3/7] ap: Refactor DHCP settings loading

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 18288 bytes --]

Extend the [IPv4].Address setting's syntax to allow a new format: a list
of <IP>/<prefix_len> -form strings that define the address space from
which a subnet is selected.  Rewrite the DHCP settings loading with
other notable changes:

 * validate some of the settings more thoroughly,
 * name all netconfig-related ap_state members with the netconfig_
   prefix,
 * make sure we always call l_dhcp_server_set_netmask(),
 * allow netmasks other than 24-bit and change the default to 28 bits,
 * as requested avoid using the l_net_ ioctl-based functions although
   l_dhcp still uses them internally,
 * as requested avoid touching the ap_state members until the end of
   some functions so that on error they're basically a no-op (for
   readabaility).
---
 src/ap.c | 423 ++++++++++++++++++++++++++++++++++---------------------
 1 file changed, 262 insertions(+), 161 deletions(-)

diff --git a/src/ap.c b/src/ap.c
index fff7c664..aad460dc 100644
--- a/src/ap.c
+++ b/src/ap.c
@@ -52,6 +52,7 @@
 #include "src/frame-xchg.h"
 #include "src/wscutil.h"
 #include "src/eap-wsc.h"
+#include "src/ip-pool.h"
 #include "src/ap.h"
 #include "src/storage.h"
 #include "src/diagnostic.h"
@@ -88,15 +89,15 @@ struct ap_state {
 	uint16_t last_aid;
 	struct l_queue *sta_states;
 
-	struct l_dhcp_server *server;
+	struct l_dhcp_server *netconfig_dhcp;
+	char *netconfig_addr4_str;
+	uint8_t netconfig_prefix_len4;
 	uint32_t rtnl_add_cmd;
-	char *own_ip;
-	unsigned int ip_prefix;
 
 	bool started : 1;
 	bool gtk_set : 1;
-	bool cleanup_ip : 1;
-	bool use_ip_pool : 1;
+	bool netconfig_set_addr4 : 1;
+	bool netconfig_use_ip_pool : 1;
 };
 
 struct sta_state {
@@ -173,15 +174,14 @@ static bool ip_pool_create(const char *ip_prefix)
 	return true;
 }
 
-static char *ip_pool_get()
+static uint32_t ip_pool_get()
 {
 	uint32_t ip;
-	struct in_addr ia;
 	uint8_t next_subnet = (uint8_t)l_uintset_find_unused_min(pool.used);
 
 	/* This shouldn't happen */
 	if (next_subnet < pool.sub_start || next_subnet > pool.sub_end)
-		return NULL;
+		return 0;
 
 	l_uintset_put(pool.used, next_subnet);
 
@@ -189,8 +189,7 @@ static char *ip_pool_get()
 	ip &= 0xffff00ff;
 	ip |= (next_subnet << 8);
 
-	ia.s_addr = htonl(ip);
-	return l_strdup(inet_ntoa(ia));
+	return ip;
 }
 
 static bool ip_pool_put(const char *address)
@@ -321,23 +320,27 @@ static void ap_reset(struct ap_state *ap)
 	ap->started = false;
 
 	/* Delete IP if one was set by IWD */
-	if (ap->cleanup_ip)
+	if (ap->netconfig_set_addr4) {
 		l_rtnl_ifaddr4_delete(rtnl, netdev_get_ifindex(netdev),
-					ap->ip_prefix, ap->own_ip,
-					broadcast_from_ip(ap->own_ip),
-					NULL, NULL, NULL);
+				ap->netconfig_prefix_len4,
+				ap->netconfig_addr4_str,
+				broadcast_from_ip(ap->netconfig_addr4_str),
+				NULL, NULL, NULL);
+		ap->netconfig_set_addr4 = false;
+	}
 
-	if (ap->own_ip) {
+	if (ap->netconfig_use_ip_pool) {
 		/* Release IP from pool if used */
-		if (ap->use_ip_pool)
-			ip_pool_put(ap->own_ip);
-
-		l_free(ap->own_ip);
+		ip_pool_put(ap->netconfig_addr4_str);
+		ap->netconfig_use_ip_pool = false;
 	}
 
-	if (ap->server) {
-		l_dhcp_server_destroy(ap->server);
-		ap->server = NULL;
+	l_free(ap->netconfig_addr4_str);
+	ap->netconfig_addr4_str = NULL;
+
+	if (ap->netconfig_dhcp) {
+		l_dhcp_server_destroy(ap->netconfig_dhcp);
+		ap->netconfig_dhcp = NULL;
 	}
 }
 
@@ -2165,7 +2168,7 @@ static void ap_start_cb(struct l_genl_msg *msg, void *user_data)
 		goto failed;
 	}
 
-	if (ap->server && !l_dhcp_server_start(ap->server)) {
+	if (ap->netconfig_dhcp && !l_dhcp_server_start(ap->netconfig_dhcp)) {
 		l_error("DHCP server failed to start");
 		goto failed;
 	}
@@ -2492,182 +2495,268 @@ static void ap_mlme_notify(struct l_genl_msg *msg, void *user_data)
 	}
 }
 
-static bool dhcp_load_settings(struct ap_state *ap,
-				const struct l_settings *settings)
+#define AP_DEFAULT_IPV4_PREFIX_LEN 28
+
+static int ap_setup_netconfig4(struct ap_state *ap, const char **addr_str_list,
+				uint8_t prefix_len, const char *gateway_str,
+				const char **ip_range,
+				const char **dns_str_list,
+				unsigned int lease_time)
 {
-	struct l_dhcp_server *server = ap->server;
+	uint32_t ifindex = netdev_get_ifindex(ap->netdev);
+	const struct ip_pool_addr4_record *addr_rec =
+		ip_pool_get_addr4(ap->netdev);
+	uint32_t addr;
+	int ret;
 	struct in_addr ia;
+	struct l_dhcp_server *dhcp;
+	bool use_ip_pool = false;
+	bool r;
 
-	L_AUTO_FREE_VAR(char *, netmask) = l_settings_get_string(settings,
-							"IPv4", "Netmask");
-	L_AUTO_FREE_VAR(char *, gateway) = l_settings_get_string(settings,
-							"IPv4", "Gateway");
-	char **dns = l_settings_get_string_list(settings, "IPv4",
-							"DNSList", ',');
-	char **ip_range = l_settings_get_string_list(settings, "IPv4",
-							"IPRange", ',');
-	unsigned int lease_time;
-	bool ret = false;
+	dhcp = l_dhcp_server_new(ifindex);
+	if (!dhcp) {
+		l_error("Failed to create DHCP server on ifindex %u", ifindex);
+		return -EIO;
+	}
+
+	if (getenv("IWD_DHCP_DEBUG"))
+		l_dhcp_server_set_debug(dhcp, do_debug,
+					"[DHCPv4 SERV] ", NULL);
+
+	/*
+	 * The address pool specified for this AP (if any) has the priority,
+	 * next is the address currently set on the interface (if any) and
+	 * last is the global AP address pool (APRanges setting).
+	 */
+	if (addr_str_list) {
+		if (!prefix_len)
+			prefix_len = AP_DEFAULT_IPV4_PREFIX_LEN;
+
+		ret = ip_pool_select_addr4(addr_str_list, prefix_len,
+						ap->netdev, &addr);
+	} else if (addr_rec && addr_rec->addr &&
+			addr_rec->prefix_len > 0 && addr_rec->prefix_len < 31) {
+		addr = addr_rec->addr;
+
+		if (!prefix_len)
+			prefix_len = addr_rec->prefix_len;
+
+		ret = 0;
+	} else {
+		if (prefix_len) {
+			l_error("[IPv4].Netmask can't be used without an "
+				"address");
+			ret = -EINVAL;
+			goto error;
+		}
+
+		/* No config, no address set. Use IP pool */
+		addr = ip_pool_get();
+		if (addr) {
+			use_ip_pool = true;
+			prefix_len = pool.prefix;
+			ret = 0;
+		} else {
+			l_error("No more IP's in pool, cannot start AP on %u",
+					ifindex);
+			ret = -EEXIST;
+		}
+	}
 
-	if (!l_settings_get_uint(settings, "IPv4", "LeaseTime", &lease_time))
-		lease_time = 0;
+	if (ret)
+		goto error;
 
-	if (gateway && !l_dhcp_server_set_gateway(server, gateway)) {
-		l_error("[IPv4].Gateway value error");
+	ret = -EIO;
+
+	/*
+	 * l_dhcp_server_start() would retrieve the current IPv4 from
+	 * the interface but set it anyway in case there are multiple
+	 * addresses, saves one ioctl too.
+	 */
+	ia.s_addr = htonl(addr);
+
+	if (!l_dhcp_server_set_ip_address(dhcp, inet_ntoa(ia))) {
+		l_error("l_dhcp_server_set_ip_address failed");
 		goto error;
 	}
 
-	if (dns && !l_dhcp_server_set_dns(server, dns)) {
-		l_error("[IPv4].DNSList value error");
+	ia.s_addr = htonl(util_netmask_from_prefix(prefix_len));
+
+	if (!l_dhcp_server_set_netmask(dhcp, inet_ntoa(ia))) {
+		l_error("l_dhcp_server_set_netmask failed");
 		goto error;
 	}
 
-	if (netmask && !l_dhcp_server_set_netmask(server, netmask)) {
-		l_error("[IPv4].Netmask value error");
+	if (gateway_str && !l_dhcp_server_set_gateway(dhcp, gateway_str)) {
+		l_error("l_dhcp_server_set_gateway failed");
 		goto error;
 	}
 
 	if (ip_range) {
-		if (l_strv_length(ip_range) != 2) {
-			l_error("Two addresses expected in [IPv4].IPRange");
+		r = l_dhcp_server_set_ip_range(dhcp, ip_range[0], ip_range[1]);
+		if (!r) {
+			l_error("l_dhcp_server_set_ip_range failed");
 			goto error;
 		}
+	}
 
-		if (!l_dhcp_server_set_ip_range(server, ip_range[0],
-							ip_range[1])) {
-			l_error("Error setting IP range from [IPv4].IPRange");
+	if (dns_str_list) {
+		r = l_dhcp_server_set_dns(dhcp, (char **) dns_str_list);
+		if (!r) {
+			l_error("l_dhcp_server_set_dns failed");
 			goto error;
 		}
 	}
 
-	if (lease_time && !l_dhcp_server_set_lease_time(server, lease_time)) {
-		l_error("[IPv4].LeaseTime value error");
+	if (lease_time && !l_dhcp_server_set_lease_time(dhcp, lease_time)) {
+		l_error("l_dhcp_server_set_lease_time failed");
 		goto error;
 	}
 
-	if (netmask && inet_pton(AF_INET, netmask, &ia) > 0)
-		ap->ip_prefix = __builtin_popcountl(ia.s_addr);
-	else
-		ap->ip_prefix = 24;
-
-	ret = true;
+	ia.s_addr = htonl(addr);
+	ap->netconfig_addr4_str = l_strdup(inet_ntoa(ia));
+	ap->netconfig_prefix_len4 = prefix_len;
+	ap->netconfig_set_addr4 = !addr_rec || addr_rec->addr != addr ||
+		addr_rec->prefix_len > prefix_len;
+	ap->netconfig_use_ip_pool = use_ip_pool;
+	ap->netconfig_dhcp = dhcp;
+	return 0;
 
 error:
-	l_strv_free(dns);
-	l_strv_free(ip_range);
+	l_dhcp_server_destroy(dhcp);
 	return ret;
 }
 
-/*
- * This will determine the IP being used for DHCP. The IP will be automatically
- * set to ap->own_ip.
- *
- * The address to set (or keep) is determined in this order:
- * 1. Address defined in provisioning file
- * 2. Address already set on interface
- * 3. Address in IP pool.
- *
- * Returns:  0 if an IP was successfully selected and needs to be set
- *          -EALREADY if an IP was already set on the interface or
- *            IP configuration was not enabled,
- *          -EEXIST if the IP pool ran out of IP's
- *          -EINVAL if there was an error.
- */
-static int ap_setup_dhcp(struct ap_state *ap, const struct l_settings *settings)
+static int ap_load_ipv4(struct ap_state *ap, const struct l_settings *config)
 {
-	uint32_t ifindex = netdev_get_ifindex(ap->netdev);
-	struct in_addr ia;
-	uint32_t address = 0;
-	L_AUTO_FREE_VAR(char *, addr) = NULL;
 	int ret = -EINVAL;
+	char **addr_str_list = NULL;
+	uint32_t static_addr = 0;
+	uint8_t prefix_len = 0;
+	char *gateway_str = NULL;
+	char **ip_range = NULL;
+	char **dns_str_list = NULL;
+	unsigned int lease_time = 0;
+	struct in_addr ia;
+
+	if (!l_settings_has_group(config, "IPv4") || !pool.used)
+		return 0;
 
-	/* get the current address if there is one */
-	if (l_net_get_address(ifindex, &ia) && ia.s_addr != 0)
-		address = ia.s_addr;
+	if (l_settings_has_key(config, "IPv4", "Address")) {
+		addr_str_list = l_settings_get_string_list(config, "IPv4",
+								"Address", ',');
+		if (!addr_str_list || !*addr_str_list) {
+			l_error("Can't parse the profile [IPv4].Address "
+				"setting as a string list");
+			goto done;
+		}
 
-	addr = l_settings_get_string(settings, "IPv4", "Address");
-	if (addr) {
-		if (inet_pton(AF_INET, addr, &ia) < 0)
-			return -EINVAL;
+		/* Check for the static IP syntax: Address=<IP> */
+		if (l_strv_length(addr_str_list) == 1 &&
+				inet_pton(AF_INET, *addr_str_list, &ia) == 1)
+			static_addr = ntohl(ia.s_addr);
+	}
 
-		/* Is a matching address already set on interface? */
-		if (ia.s_addr == address)
-			ret = -EALREADY;
-		else
-			ret = 0;
-	} else if (address) {
-		/* No address in config, but interface has one set */
-		addr = l_strdup(inet_ntoa(ia));
-		ret = -EALREADY;
-	} else if (pool.used) {
-		/* No config file, no address set. Use IP pool */
-		addr = ip_pool_get();
-		if (!addr) {
-			l_error("No more IP's in pool, cannot start AP on %u",
-					ifindex);
-			return -EEXIST;
+	if (l_settings_has_key(config, "IPv4", "Netmask")) {
+		L_AUTO_FREE_VAR(char *, netmask_str) =
+			l_settings_get_string(config, "IPv4", "Netmask");
+
+		if (inet_pton(AF_INET, netmask_str, &ia) != 1) {
+			l_error("Can't parse the profile [IPv4].Netmask "
+				"setting");
+			goto done;
 		}
 
-		ap->use_ip_pool = true;
-		ap->ip_prefix = pool.prefix;
-		ret = 0;
-	} else
-		return -EALREADY;
+		prefix_len = __builtin_popcount(ia.s_addr);
 
-	ap->server = l_dhcp_server_new(ifindex);
-	if (!ap->server) {
-		l_error("Failed to create DHCP server on %u", ifindex);
-		return -EINVAL;
+		if (ntohl(ia.s_addr) != util_netmask_from_prefix(prefix_len)) {
+			l_error("Invalid profile [IPv4].Netmask value");
+			goto done;
+		}
 	}
 
-	if (getenv("IWD_DHCP_DEBUG"))
-		l_dhcp_server_set_debug(ap->server, do_debug,
-							"[DHCPv4 SERV] ", NULL);
+	if (l_settings_has_key(config, "IPv4", "Gateway")) {
+		gateway_str = l_settings_get_string(config, "IPv4", "Gateway");
+		if (!gateway_str) {
+			l_error("Invalid profile [IPv4].Gateway value");
+			goto done;
+		}
+	}
 
-	/* Set the remaining DHCP options in config file */
-	if (!dhcp_load_settings(ap, settings))
-		return -EINVAL;
+	if (l_settings_get_value(config, "IPv4", "IPRange")) {
+		int i;
+		uint32_t netmask;
+		uint8_t tmp_len = prefix_len ?: AP_DEFAULT_IPV4_PREFIX_LEN;
 
-	if (!l_dhcp_server_set_ip_address(ap->server, addr))
-		return -EINVAL;
+		ip_range = l_settings_get_string_list(config, "IPv4",
+							"IPRange", ',');
 
-	ap->own_ip = l_steal_ptr(addr);
-	return ret;
-}
+		if (!static_addr) {
+			l_error("[IPv4].IPRange only makes sense in an AP "
+				"profile if a static local address has also "
+				"been specified");
+			goto done;
+		}
 
-static int ap_load_dhcp(struct ap_state *ap, const struct l_settings *config,
-			bool *wait_dhcp)
-{
-	uint32_t ifindex = netdev_get_ifindex(ap->netdev);
-	int err = -EINVAL;
+		if (!ip_range || l_strv_length(ip_range) != 2) {
+			l_error("Can't parse the profile [IPv4].IPRange "
+				"setting as two address strings");
+			goto done;
+		}
 
-	if (!l_settings_has_group(config, "IPv4"))
-		return 0;
+		netmask = util_netmask_from_prefix(tmp_len);
 
-	err = ap_setup_dhcp(ap, config);
-	if (err == 0) {
-		/* Address change required */
-		ap->rtnl_add_cmd = l_rtnl_ifaddr4_add(rtnl, ifindex,
-					ap->ip_prefix, ap->own_ip,
-					broadcast_from_ip(ap->own_ip),
-					ap_ifaddr4_added_cb, ap, NULL);
+		for (i = 0; i < 2; i++) {
+			struct in_addr range_addr;
 
-		if (!ap->rtnl_add_cmd) {
-			l_error("Failed to add IPv4 address");
-			return -EIO;
+			if (inet_aton(ip_range[i], &range_addr) != 1) {
+				l_error("Can't parse address in "
+					"[IPv4].IPRange[%i]", i + 1);
+				goto done;
+			}
+
+			if ((static_addr ^ ntohl(range_addr.s_addr)) &
+					netmask) {
+				ia.s_addr = htonl(static_addr);
+				l_error("[IPv4].IPRange[%i] is not in the "
+					"%s/%i subnet", i + 1, inet_ntoa(ia),
+					tmp_len);
+				goto done;
+			}
 		}
+	}
 
-		ap->cleanup_ip = true;
+	if (l_settings_has_key(config, "IPv4", "DNSList")) {
+		dns_str_list = l_settings_get_string_list(config, "IPv4",
+								"DNSList", ',');
+		if (!dns_str_list || !*dns_str_list) {
+			l_error("Can't parse the profile [IPv4].DNSList "
+				"setting as a string list");
+			goto done;
+		}
+	}
 
-		*wait_dhcp = true;
-		err = 0;
-	/* Selected address already set, continue normally */
-	} else if (err == -EALREADY) {
-		*wait_dhcp = false;
-		err = 0;
+	if (l_settings_has_key(config, "IPv4", "LeaseTime")) {
+		if (!l_settings_get_uint(config, "IPv4", "LeaseTime",
+						&lease_time) ||
+				lease_time < 1) {
+			l_error("Error parsing [IPv4].LeaseTime as a positive "
+				"integer");
+			goto done;
+		}
 	}
 
-	return err;
+	ret = ap_setup_netconfig4(ap, (const char **) addr_str_list, prefix_len,
+					gateway_str, (const char **) ip_range,
+					(const char **) dns_str_list,
+					lease_time);
+
+done:
+	l_strv_free(addr_str_list);
+	l_free(gateway_str);
+	l_strv_free(ip_range);
+	l_strv_free(dns_str_list);
+	return ret;
 }
 
 static bool ap_load_psk(struct ap_state *ap, const struct l_settings *config)
@@ -2722,7 +2811,7 @@ static bool ap_load_psk(struct ap_state *ap, const struct l_settings *config)
 }
 
 static int ap_load_config(struct ap_state *ap, const struct l_settings *config,
-				bool *out_wait_dhcp, bool *out_cck_rates)
+				bool *out_cck_rates)
 {
 	size_t len;
 	L_AUTO_FREE_VAR(char *, strval) = NULL;
@@ -2748,11 +2837,12 @@ static int ap_load_config(struct ap_state *ap, const struct l_settings *config,
 		return -EINVAL;
 
 	/*
-	 * This loads DHCP settings either from the l_settings object or uses
-	 * the defaults. wait_on_address will be set true if an address change
-	 * is required.
+	 * This looks@the network configuration settings in @config and
+	 * relevant global settings and if it determines that netconfig is to
+	 * be enabled for the AP, it both creates the DHCP server object and
+	 * processes IP settings, applying the defaults where needed.
 	 */
-	err = ap_load_dhcp(ap, config, out_wait_dhcp);
+	err = ap_load_ipv4(ap, config);
 	if (err)
 		return err;
 
@@ -2868,15 +2958,15 @@ struct ap_state *ap_start(struct netdev *netdev, struct l_settings *config,
 	struct ap_state *ap;
 	struct wiphy *wiphy = netdev_get_wiphy(netdev);
 	uint64_t wdev_id = netdev_get_wdev_id(netdev);
-	int err = -EINVAL;
-	bool wait_on_address = false;
+	int err;
 	bool cck_rates = true;
 
-	if (err_out)
-		*err_out = err;
+	if (L_WARN_ON(!config)) {
+		if (err_out)
+			*err_out = -EINVAL;
 
-	if (L_WARN_ON(!config))
 		return NULL;
+	}
 
 	ap = l_new(struct ap_state, 1);
 	ap->nl80211 = l_genl_family_new(iwd_get_genl(), NL80211_GENL_NAME);
@@ -2884,7 +2974,7 @@ struct ap_state *ap_start(struct netdev *netdev, struct l_settings *config,
 	ap->ops = ops;
 	ap->user_data = user_data;
 
-	err = ap_load_config(ap, config, &wait_on_address, &cck_rates);
+	err = ap_load_config(ap, config, &cck_rates);
 	if (err)
 		goto error;
 
@@ -2950,9 +3040,20 @@ struct ap_state *ap_start(struct netdev *netdev, struct l_settings *config,
 	if (!ap->mlme_watch)
 		l_error("Registering for MLME notification failed");
 
-	if (wait_on_address) {
-		if (err_out)
-			*err_out = 0;
+	if (ap->netconfig_set_addr4) {
+		const char *broadcast_str =
+			broadcast_from_ip(ap->netconfig_addr4_str);
+
+		ap->rtnl_add_cmd = l_rtnl_ifaddr4_add(rtnl,
+						netdev_get_ifindex(netdev),
+						ap->netconfig_prefix_len4,
+						ap->netconfig_addr4_str,
+						broadcast_str,
+						ap_ifaddr4_added_cb, ap, NULL);
+		if (!ap->rtnl_add_cmd) {
+			l_error("Failed to add the IPv4 address");
+			goto error;
+		}
 
 		return ap;
 	}
-- 
2.27.0

[PATCH 4/7] ap: Refactor global address pool loading

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 7749 bytes --]

Deprecate the global [General].APRanges setting in favour of
[IPv4].APAddressPool with an extended (but backwards-compatible) syntax.
Drop the existing address pool creation code.

The new APAddressPool setting has the same syntax as the profile-local
[IPv4].Address setting and the subnet selection code will fall back
to the global setting if it's missing, this way we use common code to
handle both settings.
---
 src/ap.c | 180 +++++++++++++------------------------------------------
 1 file changed, 42 insertions(+), 138 deletions(-)

diff --git a/src/ap.c b/src/ap.c
index aad460dc..52587d9d 100644
--- a/src/ap.c
+++ b/src/ap.c
@@ -97,7 +97,6 @@ struct ap_state {
 	bool started : 1;
 	bool gtk_set : 1;
 	bool netconfig_set_addr4 : 1;
-	bool netconfig_use_ip_pool : 1;
 };
 
 struct sta_state {
@@ -128,97 +127,11 @@ struct ap_wsc_pbc_probe_record {
 	uint64_t timestamp;
 };
 
-struct ap_ip_pool {
-	uint32_t start;
-	uint32_t end;
-	uint8_t prefix;
-
-	/* Fist/last valid subnet */
-	uint8_t sub_start;
-	uint8_t sub_end;
-
-	struct l_uintset *used;
-};
-
-struct ap_ip_pool pool;
+static bool netconfig_enabled;
+static char **global_addr4_strs;
 static uint32_t netdev_watch;
 static struct l_netlink *rtnl;
 
-/*
- * Creates pool of IPs which AP intefaces can use. Each call to ip_pool_get
- * will advance the subnet +1 so there are no IP conflicts between AP
- * interfaces
- */
-static bool ip_pool_create(const char *ip_prefix)
-{
-	if (!util_ip_prefix_tohl(ip_prefix, &pool.prefix, &pool.start,
-					&pool.end, NULL))
-		return false;
-
-	if (pool.prefix > 24) {
-		l_error("APRanges prefix must 24 or less (%u used)",
-				pool.prefix);
-		memset(&pool, 0, sizeof(pool));
-		return false;
-	}
-
-	/*
-	 * Find the number of subnets we can use, this will dictate the number
-	 * of AP interfaces that can be created (when using DHCP)
-	 */
-	pool.sub_start = (pool.start & 0x0000ff00) >> 8;
-	pool.sub_end = (pool.end & 0x0000ff00) >> 8;
-
-	pool.used = l_uintset_new_from_range(pool.sub_start, pool.sub_end);
-
-	return true;
-}
-
-static uint32_t ip_pool_get()
-{
-	uint32_t ip;
-	uint8_t next_subnet = (uint8_t)l_uintset_find_unused_min(pool.used);
-
-	/* This shouldn't happen */
-	if (next_subnet < pool.sub_start || next_subnet > pool.sub_end)
-		return 0;
-
-	l_uintset_put(pool.used, next_subnet);
-
-	ip = pool.start;
-	ip &= 0xffff00ff;
-	ip |= (next_subnet << 8);
-
-	return ip;
-}
-
-static bool ip_pool_put(const char *address)
-{
-	struct in_addr ia;
-	uint32_t ip;
-	uint8_t subnet;
-
-	if (inet_aton(address, &ia) < 0)
-		return false;
-
-	ip = ntohl(ia.s_addr);
-
-	subnet = (ip & 0x0000ff00) >> 8;
-
-	if (subnet < pool.sub_start || subnet > pool.sub_end)
-		return false;
-
-	return l_uintset_take(pool.used, subnet);
-}
-
-static void ip_pool_destroy()
-{
-	if (pool.used)
-		l_uintset_free(pool.used);
-
-	memset(&pool, 0, sizeof(pool));
-}
-
 static const char *broadcast_from_ip(const char *ip)
 {
 	struct in_addr ia;
@@ -329,12 +242,6 @@ static void ap_reset(struct ap_state *ap)
 		ap->netconfig_set_addr4 = false;
 	}
 
-	if (ap->netconfig_use_ip_pool) {
-		/* Release IP from pool if used */
-		ip_pool_put(ap->netconfig_addr4_str);
-		ap->netconfig_use_ip_pool = false;
-	}
-
 	l_free(ap->netconfig_addr4_str);
 	ap->netconfig_addr4_str = NULL;
 
@@ -2510,7 +2417,6 @@ static int ap_setup_netconfig4(struct ap_state *ap, const char **addr_str_list,
 	int ret;
 	struct in_addr ia;
 	struct l_dhcp_server *dhcp;
-	bool use_ip_pool = false;
 	bool r;
 
 	dhcp = l_dhcp_server_new(ifindex);
@@ -2543,24 +2449,11 @@ static int ap_setup_netconfig4(struct ap_state *ap, const char **addr_str_list,
 
 		ret = 0;
 	} else {
-		if (prefix_len) {
-			l_error("[IPv4].Netmask can't be used without an "
-				"address");
-			ret = -EINVAL;
-			goto error;
-		}
+		if (!prefix_len)
+			prefix_len = AP_DEFAULT_IPV4_PREFIX_LEN;
 
-		/* No config, no address set. Use IP pool */
-		addr = ip_pool_get();
-		if (addr) {
-			use_ip_pool = true;
-			prefix_len = pool.prefix;
-			ret = 0;
-		} else {
-			l_error("No more IP's in pool, cannot start AP on %u",
-					ifindex);
-			ret = -EEXIST;
-		}
+		ret = ip_pool_select_addr4((const char **) global_addr4_strs,
+						prefix_len, ap->netdev, &addr);
 	}
 
 	if (ret)
@@ -2618,7 +2511,6 @@ static int ap_setup_netconfig4(struct ap_state *ap, const char **addr_str_list,
 	ap->netconfig_prefix_len4 = prefix_len;
 	ap->netconfig_set_addr4 = !addr_rec || addr_rec->addr != addr ||
 		addr_rec->prefix_len > prefix_len;
-	ap->netconfig_use_ip_pool = use_ip_pool;
 	ap->netconfig_dhcp = dhcp;
 	return 0;
 
@@ -2639,7 +2531,7 @@ static int ap_load_ipv4(struct ap_state *ap, const struct l_settings *config)
 	unsigned int lease_time = 0;
 	struct in_addr ia;
 
-	if (!l_settings_has_group(config, "IPv4") || !pool.used)
+	if (!l_settings_has_group(config, "IPv4") || !netconfig_enabled)
 		return 0;
 
 	if (l_settings_has_key(config, "IPv4", "Address")) {
@@ -3628,7 +3520,6 @@ static void ap_netdev_watch(struct netdev *netdev,
 static int ap_init(void)
 {
 	const struct l_settings *settings = iwd_get_config();
-	bool dhcp_enable;
 
 	netdev_watch = netdev_watch_add(ap_netdev_watch, NULL, NULL);
 
@@ -3639,31 +3530,44 @@ static int ap_init(void)
 			ap_diagnostic_interface_destroy, false);
 
 	/*
-	 * Reusing [General].EnableNetworkConfiguration as a switch to enable
-	 * DHCP server. If no value is found or it is false do not create a
-	 * DHCP server.
+	 * Enable network configuration and DHCP only if
+	 * [General].EnableNetworkConfiguration is true.
 	 */
 	if (!l_settings_get_bool(settings, "General",
-				"EnableNetworkConfiguration", &dhcp_enable))
-		dhcp_enable = false;
-
-	if (dhcp_enable) {
-		L_AUTO_FREE_VAR(char *, ip_prefix);
-
-		ip_prefix = l_settings_get_string(settings, "General",
-							"APRanges");
-		/*
-		 * In this case its assumed the user only cares about station
-		 * netconfig so we let ap_init pass but DHCP will not be
-		 * enabled.
-		 */
-		if (!ip_prefix) {
-			l_warn("[General].APRanges must be set for DHCP");
-			return 0;
+					"EnableNetworkConfiguration",
+					&netconfig_enabled))
+		netconfig_enabled = false;
+
+	if (netconfig_enabled) {
+		if (l_settings_get_value(settings, "IPv4", "APAddressPool")) {
+			global_addr4_strs = l_settings_get_string_list(settings,
+								"IPv4",
+								"APAddressPool",
+								',');
+			if (!global_addr4_strs || !*global_addr4_strs) {
+				l_error("Can't parse the [IPv4].APAddressPool "
+					"setting as a string list");
+				l_strv_free(global_addr4_strs);
+				global_addr4_strs = NULL;
+			}
+		} else if (l_settings_get_value(settings,
+						"General", "APRanges")) {
+			global_addr4_strs = l_settings_get_string_list(settings,
+								"General",
+								"APRanges",
+								',');
+			if (!global_addr4_strs || !*global_addr4_strs) {
+				l_error("Can't parse the [General].APRanges "
+					"setting as a string list");
+				l_strv_free(global_addr4_strs);
+				global_addr4_strs = NULL;
+			}
 		}
 
-		if (!ip_pool_create(ip_prefix))
-			return -EINVAL;
+		/* Fall back to 192.168.0.0/16 */
+		if (!global_addr4_strs)
+			global_addr4_strs =
+				l_strv_append(NULL, "192.168.0.0/16");
 	}
 
 	rtnl = iwd_get_rtnl();
@@ -3676,7 +3580,7 @@ static void ap_exit(void)
 	netdev_watch_remove(netdev_watch);
 	l_dbus_unregister_interface(dbus_get_bus(), IWD_AP_INTERFACE);
 
-	ip_pool_destroy();
+	l_strv_free(global_addr4_strs);
 }
 
 IWD_MODULE(ap, ap_init, ap_exit)
-- 
2.27.0

[PATCH 5/7] ap: Send a specific error message on async AP start failure

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 3543 bytes --]

We generate the DBus error reply type from the errno only when
ap_start() was failing synchronously, now also send the errno through
the callbacks so that we can also return a specific DBus reply when
failing asynchronously.  Thea AP autotest relies on receiving the
AlreadyExists DBus error.
---
 src/ap.c | 32 ++++++++++++++++++--------------
 src/ap.h |  4 ++++
 2 files changed, 22 insertions(+), 14 deletions(-)

diff --git a/src/ap.c b/src/ap.c
index 52587d9d..639c9101 100644
--- a/src/ap.c
+++ b/src/ap.c
@@ -2054,9 +2054,11 @@ static void do_debug(const char *str, void *user_data)
 	l_info("%s%s", prefix, str);
 }
 
-static void ap_start_failed(struct ap_state *ap)
+static void ap_start_failed(struct ap_state *ap, int err)
 {
-	ap->ops->handle_event(AP_EVENT_START_FAILED, NULL, ap->user_data);
+	struct ap_event_start_failed_data data = { err };
+
+	ap->ops->handle_event(AP_EVENT_START_FAILED, &data, ap->user_data);
 	ap_reset(ap);
 	l_genl_family_free(ap->nl80211);
 
@@ -2071,22 +2073,18 @@ static void ap_start_cb(struct l_genl_msg *msg, void *user_data)
 
 	if (l_genl_msg_get_error(msg) < 0) {
 		l_error("START_AP failed: %i", l_genl_msg_get_error(msg));
-
-		goto failed;
+		ap_start_failed(ap, l_genl_msg_get_error(msg));
+		return;
 	}
 
 	if (ap->netconfig_dhcp && !l_dhcp_server_start(ap->netconfig_dhcp)) {
 		l_error("DHCP server failed to start");
-		goto failed;
+		ap_start_failed(ap, -EINVAL);
+		return;
 	}
 
 	ap->started = true;
 	ap->ops->handle_event(AP_EVENT_STARTED, NULL, ap->user_data);
-
-	return;
-
-failed:
-	ap_start_failed(ap);
 }
 
 static struct l_genl_msg *ap_build_cmd_start_ap(struct ap_state *ap)
@@ -2201,12 +2199,12 @@ static void ap_ifaddr4_added_cb(int error, uint16_t type, const void *data,
 
 	if (error) {
 		l_error("Failed to set IP address");
-		ap_start_failed(ap);
+		ap_start_failed(ap, error);
 		return;
 	}
 
 	if (!ap_start_send(ap))
-		ap_start_failed(ap);
+		ap_start_failed(ap, -EIO);
 }
 
 static bool ap_parse_new_station_ies(const void *data, uint16_t len,
@@ -2378,10 +2376,12 @@ static void ap_mlme_notify(struct l_genl_msg *msg, void *user_data)
 	switch (l_genl_msg_get_command(msg)) {
 	case NL80211_CMD_STOP_AP:
 		if (ap->start_stop_cmd_id) {
+			struct ap_event_start_failed_data data = { -ECANCELED };
+
 			l_genl_family_cancel(ap->nl80211,
 						ap->start_stop_cmd_id);
 			ap->start_stop_cmd_id = 0;
-			ap->ops->handle_event(AP_EVENT_START_FAILED, NULL,
+			ap->ops->handle_event(AP_EVENT_START_FAILED, &data,
 						ap->user_data);
 		} else if (ap->started) {
 			ap->started = false;
@@ -3145,13 +3145,17 @@ static void ap_if_event_func(enum ap_event_type type, const void *event_data,
 
 	switch (type) {
 	case AP_EVENT_START_FAILED:
+	{
+		const struct ap_event_start_failed_data *data = event_data;
+
 		if (L_WARN_ON(!ap_if->pending))
 			break;
 
-		reply = dbus_error_failed(ap_if->pending);
+		reply = dbus_error_from_errno(data->error, ap_if->pending);
 		dbus_pending_reply(&ap_if->pending, reply);
 		ap_if->ap = NULL;
 		break;
+	}
 
 	case AP_EVENT_STARTED:
 		if (L_WARN_ON(!ap_if->pending))
diff --git a/src/ap.h b/src/ap.h
index b0100f34..9a03d3ae 100644
--- a/src/ap.h
+++ b/src/ap.h
@@ -35,6 +35,10 @@ enum ap_event_type {
 	AP_EVENT_PBC_MODE_EXIT,
 };
 
+struct ap_event_start_failed_data {
+	int error;
+};
+
 struct ap_event_station_added_data {
 	const uint8_t *mac;
 	const uint8_t *assoc_ies;
-- 
2.27.0

[PATCH 6/7] autotests: Update APRanges usage in testAP

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 2087 bytes --]

[General].APRange is now [IPv4].APAddressPool and the netmask is changed
from 23 to 27 bits to make the test correctly assert that only two
default-sized subnets are allowed by IWD simultaneously (default has
changed from 24 to 28 bits)
---
 autotests/testAP/dhcp/main.conf |  4 +++-
 autotests/testAP/dhcp_test.py   | 12 +++++++++---
 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/autotests/testAP/dhcp/main.conf b/autotests/testAP/dhcp/main.conf
index 667de23c..62ae782d 100644
--- a/autotests/testAP/dhcp/main.conf
+++ b/autotests/testAP/dhcp/main.conf
@@ -3,4 +3,6 @@ DisableMacAddressRandomization=true
 
 [General]
 EnableNetworkConfiguration=true
-APRanges=192.168.80.0/23
+
+[IPv4]
+APAddressPool=192.168.80.0/27
diff --git a/autotests/testAP/dhcp_test.py b/autotests/testAP/dhcp_test.py
index bf52fb28..e69c4465 100644
--- a/autotests/testAP/dhcp_test.py
+++ b/autotests/testAP/dhcp_test.py
@@ -58,8 +58,14 @@ class Test(unittest.TestCase):
             testutil.test_iface_operstate(dev2.name)
             testutil.test_ifaces_connected(dev1.name, dev2.name, group=False)
 
-            testutil.test_ip_address_match(dev1.name, "192.168.80.1")
-            testutil.test_ip_address_match(dev2.name, "192.168.80.2")
+            try:
+                testutil.test_ip_address_match(dev1.name, "192.168.80.1")
+                testutil.test_ip_address_match(dev2.name, "192.168.80.2")
+                ip = "192.168.80.1"
+            except:
+                testutil.test_ip_address_match(dev1.name, "192.168.80.17")
+                testutil.test_ip_address_match(dev2.name, "192.168.80.18")
+                ip = "192.168.80.17"
 
             wd.unregister_psk_agent(psk_agent)
 
@@ -76,7 +82,7 @@ class Test(unittest.TestCase):
             # got initially.
             dev4.start_ap('TestAP4', 'Password4')
 
-            testutil.test_ip_address_match(dev4.name, "192.168.80.1")
+            testutil.test_ip_address_match(dev4.name, ip)
 
         finally:
             dev1.stop_ap()
-- 
2.27.0

[PATCH 7/7] doc: Update AP settings in iwd.ap(5) and iwd.config(5)

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 4922 bytes --]

---
 src/iwd.ap.rst     | 31 ++++++++++++++++++++++++-------
 src/iwd.config.rst | 29 ++++++++++++++++++++++++++---
 2 files changed, 50 insertions(+), 10 deletions(-)

diff --git a/src/iwd.ap.rst b/src/iwd.ap.rst
index 51622509..5fa23179 100644
--- a/src/iwd.ap.rst
+++ b/src/iwd.ap.rst
@@ -104,14 +104,31 @@ is desired, the group header line must still be present:
    :widths: 20 80
 
    * - Address
-     - Local IP address
+     - Local IP address or a comma-separated list of prefix-notation addresses
 
        Optional local address pool for the access point and the DHCP server.
-       If provided this addresss will be set on the AP interface and any other
-       DHCP server options will be derived from it, unless they are overridden
-       by other settings below.  If *Address* is not provided and no IP
-       address is set on the interface prior to calling `StartProfile`,  the IP
-       pool defined by the global ``[General].APRanges`` setting will be used.
+       If a single address is provided this address will be set on the AP
+       interface and any other DHCP server options will be derived from it
+       if not overridden by other settings below.
+
+       If a list of addresses and prefix lengths is specified (in the
+       `<IP>/<prefix-len>` format), a single subnet address will be selected
+       from the available space each time this profile is started.  The subnet
+       size is based on the ``[IPv4].Netmask`` setting.
+
+       If *Address* is not provided and no IP address is set on the
+       interface prior to calling `StartProfile` the value of the main.conf
+       ``[IPv4].APAddressPool`` setting will be inherited, which in turn
+       defaults to 192.168.0.0/16.
+
+       For example, if ``[IPv4].Netmask`` is set to 255.255.255.0 and this
+       setting, or the global *APAddressPool* fallback, is set to
+       ``192.168.0.0/16, 10.0.0.0/22``, IWD will select one of the 256 subnets
+       with addresses in the 192.168.<0-255>.0/24 range or one of the 4 subnets
+       with addresses in the 10.0.<0-3>.0/24 range, allowing 270 possible
+       subnets.  Defining an address pool larger than the desired subnet gives
+       IWD a chance to avoid conflicts if other interfaces on the system use
+       dynamically assigned addresses.
 
    * - Gateway
      - IP Address of gateway
@@ -122,7 +139,7 @@ is desired, the group header line must still be present:
    * - Netmask
      - Local netmask of the AP
 
-       This will be generated from ``[IPv4].Address`` if not provided.
+       Defaults to a 28-bit netmask if not provided.
 
    * - DNSList
      - List of DNS servers as a comma-separated IP address list
diff --git a/src/iwd.config.rst b/src/iwd.config.rst
index 60f5b5db..78ec284e 100644
--- a/src/iwd.config.rst
+++ b/src/iwd.config.rst
@@ -67,8 +67,8 @@ The group ``[General]`` contains general settings.
        obtain the dynamic addresses from the network through the built-in
        DHCP client.
 
-       This also enables DHCP server when in AP mode when either
-       [General].APRanges is set or an AP profile is being used.
+       This also enables network configuration and the DHCP server when in AP
+       mode and the AP profile being activated does not override it.
 
        The network configuration feature is disabled by default.  See
        ``[Network]`` settings for additional settings related to network
@@ -182,7 +182,7 @@ The group ``[General]`` contains general settings.
        then setting ``DisableANQP`` to ``false`` is recommended.
 
 Network
----------
+-------
 
 The group ``[Network]`` contains network configuration related settings.
 
@@ -319,6 +319,29 @@ No modification from defaults is normally required.
        prevent **iwd** from roaming properly, but can be useful for networks
        operating under extremely low rssi levels where roaming isn't possible.
 
+IPv4
+----
+
+The group ``[IPv4]`` contains settings related to IPv4 network configuration.
+
+.. list-table::
+   :header-rows: 0
+   :stub-columns: 0
+   :widths: 20 80
+   :align: left
+
+   * - APAddressPool
+     - Values: comma-separated list of prefix-notation IP strings
+
+       Defines the space of IPs used for the Access Point-mode subnet addresses
+       and the DHCP server.  Defaults to 192.168.0.0/16.  The prefix length
+       decides the size of the pool from which an address is selected but the
+       actual subnet size (netmask) is based on the AP profile being activated
+       and defaults to 28 bits.  The AP profile's ``[IPv4].Address`` setting
+       overrides the global value set here.  Setting a too small address space
+       will limit the number of access points that can be running
+       simultaneously on different interfaces.
+
 SEE ALSO
 ========
 
-- 
2.27.0

Re: [PATCH 1/7] ip-pool: Track IPv4 addresses in use

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 2436 bytes --]

Hi Andrew,

On 5/25/21 7:55 AM, Andrew Zaborowski wrote:
> Add the ip-pool submodule that tracks IPv4 addresses in use on the
> system for use when selecting the address for a new AP.
> ---
>   Makefile.am   |   1 +
>   src/ip-pool.c | 160 ++++++++++++++++++++++++++++++++++++++++++++++++++
>   src/ip-pool.h |  32 ++++++++++
>   3 files changed, 193 insertions(+)
>   create mode 100644 src/ip-pool.c
>   create mode 100644 src/ip-pool.h
> 

<snip>

So I'm completely fine with the implementation now...

> diff --git a/src/ip-pool.h b/src/ip-pool.h
> new file mode 100644
> index 00000000..6a220735
> --- /dev/null
> +++ b/src/ip-pool.h
> @@ -0,0 +1,32 @@
> +/*
> + *
> + *  Wireless daemon for Linux
> + *
> + *  Copyright (C) 2021  Intel Corporation. All rights reserved.
> + *
> + *  This library is free software; you can redistribute it and/or
> + *  modify it under the terms of the GNU Lesser General Public
> + *  License as published by the Free Software Foundation; either
> + *  version 2.1 of the License, or (at your option) any later version.
> + *
> + *  This library is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + *  Lesser General Public License for more details.
> + *
> + *  You should have received a copy of the GNU Lesser General Public
> + *  License along with this library; if not, write to the Free Software
> + *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> + *
> + */
> +
> +struct netdev;

Is there a strong reason for making this tighly-coupled to netdev?  I mean in 
theory ip-pool can be reused by netconfig for example, which also tracks IPv4/v6 
addresses (for some unknown reason, but one thing at a time...)

> +
> +struct ip_pool_addr4_record {
> +	uint32_t addr;
> +	uint8_t prefix_len;
> +	uint32_t ifindex;
> +	bool secondary;
> +};

Is there a compelling reason to use a dedicated structure (leaking 
implementation details) instead of returning a newly allocated l_rtnl_address 
instead?

> +
> +const struct ip_pool_addr4_record *ip_pool_get_addr4(struct netdev *netdev);
> 

This function is used exactly once, when the profile is loaded.  So it isn't 
exactly on the hot-path where raw access to the store is needed.

Regards,
-Denis

Re: [PATCH 1/7] ip-pool: Track IPv4 addresses in use

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 1269 bytes --]

Hi Denis,

On Wed, 26 May 2021 at 21:43, Denis Kenzior <denkenz@gmail.com> wrote:
> On 5/25/21 7:55 AM, Andrew Zaborowski wrote:
> > +struct netdev;
>
> Is there a strong reason for making this tighly-coupled to netdev?

Other than convenience, no.

> I mean in
> theory ip-pool can be reused by netconfig for example, which also tracks IPv4/v6
> addresses (for some unknown reason, but one thing at a time...)

That shouldn't be a problem but ok, I'll drop the netdev usage.

>
> > +
> > +struct ip_pool_addr4_record {
> > +     uint32_t addr;
> > +     uint8_t prefix_len;
> > +     uint32_t ifindex;
> > +     bool secondary;
> > +};
>
> Is there a compelling reason to use a dedicated structure (leaking
> implementation details) instead of returning a newly allocated l_rtnl_address
> instead?

Again, convenience, readability, better fit for the use case.  But
like I said I can move to using l_rtnl_address, I didn't do it this
time because it sounded like I convinced you against it.  I'll switch
to l_rtnl_address though.

Using l_rtnl_address here is like using a class for a simple integer
or boolean value (Java style), I wouldn't call directly accessing the
actual value "leaking implementation details."

Best regards

Re: [PATCH 1/7] ip-pool: Track IPv4 addresses in use

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 2052 bytes --]

Hi Andrew,

>> I mean in
>> theory ip-pool can be reused by netconfig for example, which also tracks IPv4/v6
>> addresses (for some unknown reason, but one thing at a time...)
> 
> That shouldn't be a problem but ok, I'll drop the netdev usage.
> 

netconfig depends on netdev right now, but it really shouldn't.  The only reason 
is that it needs the device name, which really belongs elsewhere since netconfig 
will end up being used by ead, which will not even come with netconfig module at 
all.

So the looser you make this coupling, the easier it will be for us to reuse this 
stuff in other projects.  Start thinking along those lines please.

>>
>>> +
>>> +struct ip_pool_addr4_record {
>>> +     uint32_t addr;
>>> +     uint8_t prefix_len;
>>> +     uint32_t ifindex;
>>> +     bool secondary;
>>> +};
>>
>> Is there a compelling reason to use a dedicated structure (leaking
>> implementation details) instead of returning a newly allocated l_rtnl_address
>> instead?
> 
> Again, convenience, readability, better fit for the use case.  But

readability? Yeah I don't buy that one ;)  You were doing stuff like storing the 
ip address in host order for whatever reason.  And you expect someone reading 
ap.c to remember / know that?

> like I said I can move to using l_rtnl_address, I didn't do it this
> time because it sounded like I convinced you against it.  I'll switch
> to l_rtnl_address though.

I'm fine if you use a custom type inside ip-pool for ease of implementation, but 
you should make the public API 'nice'.

> 
> Using l_rtnl_address here is like using a class for a simple integer
> or boolean value (Java style), I wouldn't call directly accessing the
> actual value "leaking implementation details."

You're returning a const struct foo * back, which basically screams 'I'm sending 
internal details out'.  Also see my comment about host/network ordering.

If you think l_rtnl_address isn't suitable for whatever reason, then lets fix 
that instead.

Regards,
-Denis

Re: [PATCH 1/7] ip-pool: Track IPv4 addresses in use

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 371 bytes --]

> netconfig depends on netdev right now, but it really shouldn't.  The only reason 
> is that it needs the device name, which really belongs elsewhere since netconfig 
> will end up being used by ead, which will not even come with netconfig module at 
> all.
> 

Whoops, s/netconfig/netdev

So that last line should read ... netdev module ...

Regards,
-Denis

Re: [PATCH 1/7] ip-pool: Track IPv4 addresses in use

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 1065 bytes --]

On Wed, 26 May 2021 at 23:56, Denis Kenzior <denkenz@gmail.com> wrote:
> > Again, convenience, readability, better fit for the use case.  But
>
> readability? Yeah I don't buy that one ;)

If you can write
  ret_val = ip_pool_select_addr4(...);

  do_some_maths(ret_val->addr)

instead of:

  struct in_addr ia;
  char addr[INET_ADDRSTRLEN];

  ret_val = ip_pool_select_addr4(...);

  if (!l_rtnl_address_get_address(ret_val, addr) || inet_aton(addr, &ia) < 0) {
        l_rtnl_address_free(ret_val);
        goto error;
  }

  do_some_maths(ntohl(ia.s_addr));

I call that readable vs. unreadable ;-)

>  You were doing stuff like storing the
> ip address in host order for whatever reason.  And you expect someone reading
> ap.c to remember / know that?

Whenever you're going to store the IP as an uint32_t (rather than e.g.
in_addr) it's probably because you want to do some arithmetics and
you'll want the host byte order.

Besides you can generally assume that a plain integer variable is in
host byte order.

Best regards

Re: [PATCH 1/7] ip-pool: Track IPv4 addresses in use

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1675 bytes --]

Hi Andrew,

On 5/26/21 5:36 PM, Andrew Zaborowski wrote:
> On Wed, 26 May 2021 at 23:56, Denis Kenzior <denkenz@gmail.com> wrote:
>>> Again, convenience, readability, better fit for the use case.  But
>>
>> readability? Yeah I don't buy that one ;)
> 
> If you can write
>    ret_val = ip_pool_select_addr4(...);
> 
>    do_some_maths(ret_val->addr)
> 

What maths are you performing and why?

The whole point of the API is to have the caller request an address/prefix combo 
that can be set directly on the interface and the dhcp_server object.  If you're 
doing any maths afterward, your API has failed, no?

> instead of:
> 
>    struct in_addr ia;
>    char addr[INET_ADDRSTRLEN];
> 
>    ret_val = ip_pool_select_addr4(...);
> 
>    if (!l_rtnl_address_get_address(ret_val, addr) || inet_aton(addr, &ia) < 0) {
>          l_rtnl_address_free(ret_val);
>          goto error;
>    }
> 
>    do_some_maths(ntohl(ia.s_addr));
> 
> I call that readable vs. unreadable ;-)

And I would question the entire design?

> 
>>   You were doing stuff like storing the
>> ip address in host order for whatever reason.  And you expect someone reading
>> ap.c to remember / know that?
> 
> Whenever you're going to store the IP as an uint32_t (rather than e.g.
> in_addr) it's probably because you want to do some arithmetics and
> you'll want the host byte order.
> 
> Besides you can generally assume that a plain integer variable is in
> host byte order.

For a uint32 that is called 'addr' coming from a module called ip_pool?  I would 
assume the opposite.  Hence, for a public API, just don't do it this way.

Regards,
-Denis

Re: [PATCH 1/7] ip-pool: Track IPv4 addresses in use

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 877 bytes --]

On Thu, 27 May 2021 at 00:55, Denis Kenzior <denkenz@gmail.com> wrote:
> On 5/26/21 5:36 PM, Andrew Zaborowski wrote:
> > On Wed, 26 May 2021 at 23:56, Denis Kenzior <denkenz@gmail.com> wrote:
> >>> Again, convenience, readability, better fit for the use case.  But
> >>
> >> readability? Yeah I don't buy that one ;)
> >
> > If you can write
> >    ret_val = ip_pool_select_addr4(...);
> >
> >    do_some_maths(ret_val->addr)
> >
>
> What maths are you performing and why?

Outside of ip_pool_select_addr4() itself (which will suffer internally
too), there's some broadcast_from_ip() for example.

But fair enough that should go away if we switch to
l_rtnl_ifaddr_add() (But again this is a change out of the scope of
this patchset that I've been rewriting since before the big ap.h
change to use an l_settings and other unrelated changes)

Best regards

Re: [PATCH 1/7] ip-pool: Track IPv4 addresses in use

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1417 bytes --]

Hi Andrew,

On 5/26/21 6:26 PM, Andrew Zaborowski wrote:
> On Thu, 27 May 2021 at 00:55, Denis Kenzior <denkenz@gmail.com> wrote:
>> On 5/26/21 5:36 PM, Andrew Zaborowski wrote:
>>> On Wed, 26 May 2021 at 23:56, Denis Kenzior <denkenz@gmail.com> wrote:
>>>>> Again, convenience, readability, better fit for the use case.  But
>>>>
>>>> readability? Yeah I don't buy that one ;)
>>>
>>> If you can write
>>>     ret_val = ip_pool_select_addr4(...);
>>>
>>>     do_some_maths(ret_val->addr)
>>>
>>
>> What maths are you performing and why?
> 
> Outside of ip_pool_select_addr4() itself (which will suffer internally
> too), there's some broadcast_from_ip() for example.

Just leave that.  Your argument of 'l_rtnl_address doesn't store the ifindex' is 
fair.  I would have solved this differently, but in the end implementation 
details are just that.

What I care about is the API, get that right and you can fix the rest later.

> 
> But fair enough that should go away if we switch to
> l_rtnl_ifaddr_add() (But again this is a change out of the scope of

That is what I thought.

> this patchset that I've been rewriting since before the big ap.h
> change to use an l_settings and other unrelated changes)

And I get that.  I feel your pain.  But then, we're adding a new module and I do 
want the APIs to be at least somewhat sorted from the very beginning.

Regards,
-Denis