[PATCH 1/2] doc: Add net.connman.iwd.IPv{4,6}Configuration API doc

[PATCH 1/2] doc: Add net.connman.iwd.IPv{4,6}Configuration API doc

[Andrew Zaborowski]

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

---
 doc/ip-configuration-api.txt | 47 ++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)
 create mode 100644 doc/ip-configuration-api.txt

diff --git a/doc/ip-configuration-api.txt b/doc/ip-configuration-api.txt
new file mode 100644
index 00000000..58a522de
--- /dev/null
+++ b/doc/ip-configuration-api.txt
@@ -0,0 +1,47 @@
+IP Configuration hierarchy
+===============================
+
+Service		net.connman.iwd
+Interface	net.connman.iwd.IPv4Configuration [Experimental]
+Interface	net.connman.iwd.IPv6Configuration [Experimental]
+Object path	/net/connman/iwd/{phy0,phy1,...}/{1,2,...}
+Object path	/net/connman/iwd/{phy0,phy1,...}/p2p_peers/{aa_bb_cc_dd_ee_ff}
+
+The interfaces net.connman.iwd.IPv4Configuration and
+net.connman.iwd.IPv6Configuration currently have the same sets of methods,
+signals and properties.  In station mode, when network configuration is
+enabled there may be one or both interfaces present on a device object in
+connected state depending on if IPv4 and IPv6 addresses have both been
+configured.  In P2P mode only net.connman.iwd.IPv4Configuration is used.
+
+Properties	string Method [readonly]
+
+			Indicates whether the local address was set
+			statically (value "static") or obtained automatically
+			such as through DHCP (value "auto").  Even when the
+			address was obtained from the remote end some
+			configuration bits, such as DNS addresses, may have
+			been overridden locally.
+
+		string Address [readonly]
+
+			Holds the local IP address.
+
+		byte PrefixLength [readonly]
+
+			Holds the prefix-length of the local subnet.  For
+			IPv4 this maps to the netmask.
+
+		string Gateway [readonly, optional]
+
+			Holds the gateway address for the subnet if one
+			exists.
+
+		array(string) DomainNameServers [readonly, optional]
+
+			Holds the list of domain name server addresses
+			configured if any.
+
+		array(string) DomainNames [readonly, optional]
+
+			Holds the network's local domain names if any exist.
-- 
2.30.2

[PATCH 2/2] netconfig: Add IP configuration properties on Station and P2P

[Andrew Zaborowski]

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

Add the IPv{4,6}Configuration D-Bus interfaces on the station and P2P
objects that expose current netconfig values.  Access Point and P2P-GO
are not handled yet, and some of the IP config properties may not
automatically generate PropertiesChanged on DHCP lease renewals but
their values are unlikely to change.
---
 src/dbus.h      |   2 +
 src/netconfig.c | 640 ++++++++++++++++++++++++++++++++++++++++--------
 src/netconfig.h |   2 +-
 src/p2p.c       |   4 +-
 src/station.c   |   8 +-
 5 files changed, 551 insertions(+), 105 deletions(-)

diff --git a/src/dbus.h b/src/dbus.h
index 7703b37f..a345f7ac 100644
--- a/src/dbus.h
+++ b/src/dbus.h
@@ -43,6 +43,8 @@
 #define IWD_STATION_DIAGNOSTIC_INTERFACE "net.connman.iwd.StationDiagnostic"
 #define IWD_AP_DIAGNOSTIC_INTERFACE "net.connman.iwd.AccessPointDiagnostic"
 #define IWD_STATION_DEBUG_INTERFACE "net.connman.iwd.StationDebug"
+#define IWD_IPV4_CONFIG_INTERFACE "net.connman.iwd.IPv4Configuration"
+#define IWD_IPV6_CONFIG_INTERFACE "net.connman.iwd.IPv6Configuration"
 
 #define IWD_BASE_PATH "/net/connman/iwd"
 #define IWD_AGENT_MANAGER_PATH IWD_BASE_PATH
diff --git a/src/netconfig.c b/src/netconfig.c
index c748630b..503bb41f 100644
--- a/src/netconfig.c
+++ b/src/netconfig.c
@@ -49,6 +49,7 @@
 #include "src/resolve.h"
 #include "src/util.h"
 #include "src/ie.h"
+#include "src/dbus.h"
 #include "src/netconfig.h"
 
 struct netconfig {
@@ -58,14 +59,18 @@ struct netconfig {
 	uint8_t rtm_protocol;
 	uint8_t rtm_v6_protocol;
 	struct l_rtnl_address *v4_address;
+	struct l_rtnl_address *v6_address;
 	char **dns4_overrides;
 	char **dns6_overrides;
 	struct ie_fils_ip_addr_response_info *fils_override;
+	char *v4_gateway_str;
+	char *v6_gateway_str;
 
 	const struct l_settings *active_settings;
 
 	netconfig_notify_func_t notify;
 	void *user_data;
+	bool connected;
 
 	struct resolve *resolve;
 
@@ -74,6 +79,12 @@ struct netconfig {
 	uint32_t addr4_add_cmd_id;
 	uint32_t addr6_add_cmd_id;
 	uint32_t route4_add_gateway_cmd_id;
+
+	char *dbus_path;
+	struct interface_data {
+		bool is_ipv4;
+		struct netconfig *netconfig;
+	} ipv4_data, ipv6_data;
 };
 
 static struct l_netlink *rtnl;
@@ -132,6 +143,7 @@ static void netconfig_free(void *data)
 	l_dhcp_client_destroy(netconfig->dhcp_client);
 	l_dhcp6_client_destroy(netconfig->dhcp6_client);
 
+	l_free(netconfig->dbus_path);
 	l_free(netconfig);
 }
 
@@ -152,20 +164,6 @@ static struct netconfig *netconfig_find(uint32_t ifindex)
 	return NULL;
 }
 
-#define APPEND_STRDUPV(dest, index, src)			\
-	do {							\
-		char **p;					\
-		for (p = src; p && *p; p++)			\
-			dest[index++] = l_strdup(*p);		\
-	} while (0)						\
-
-#define APPENDV(dest, index, src)				\
-	do {							\
-		char **p;					\
-		for (p = src; p && *p; p++)			\
-			dest[index++] = *p;			\
-	} while (0)						\
-
 static inline char *netconfig_ipv4_to_string(uint32_t addr)
 {
 	struct in_addr in_addr = { .s_addr = addr };
@@ -196,77 +194,108 @@ static inline char *netconfig_ipv6_to_string(const uint8_t *addr)
 	return addr_str;
 }
 
-static void netconfig_set_neighbor_entry_cb(int error,
-						uint16_t type, const void *data,
-						uint32_t len, void *user_data)
-{
-	if (error)
-		l_error("l_rtnl_neighbor_set_hwaddr failed: %s (%i)",
-			strerror(-error), error);
-}
-
-static int netconfig_set_dns(struct netconfig *netconfig)
+static char **netconfig_get_dns_list(struct netconfig *netconfig, int af,
+					const uint8_t **out_dns_mac)
 {
-	char **dns6_list = NULL;
-	char **dns4_list = NULL;
-	unsigned int n_entries = 0;
-	char **dns_list;
-	const uint8_t *fils_dns4_mac = NULL;
-	const uint8_t *fils_dns6_mac = NULL;
 	const struct ie_fils_ip_addr_response_info *fils =
 		netconfig->fils_override;
 
-	if (!netconfig->dns4_overrides &&
-			netconfig->rtm_protocol == RTPROT_DHCP) {
+	if (af == AF_INET) {
 		const struct l_dhcp_lease *lease;
 
+		if (netconfig->dns4_overrides)
+			return l_strv_copy(netconfig->dns4_overrides);
+
+		if (netconfig->rtm_protocol != RTPROT_DHCP)
+			return NULL;
+
 		if (fils && fils->ipv4_dns) {
-			dns4_list = l_new(char *, 2);
-			dns4_list[0] = netconfig_ipv4_to_string(fils->ipv4_dns);
+			char **dns_list = l_new(char *, 2);
 
 			if (!l_memeqzero(fils->ipv4_dns_mac, 6) &&
+					out_dns_mac &&
 					util_ip_subnet_match(
 							fils->ipv4_prefix_len,
 							&fils->ipv4_addr,
 							&fils->ipv4_dns))
-				fils_dns4_mac = fils->ipv4_dns_mac;
-		} else if ((lease = l_dhcp_client_get_lease(
-						netconfig->dhcp_client)))
-			dns4_list = l_dhcp_lease_get_dns(lease);
-	}
+				*out_dns_mac = fils->ipv4_dns_mac;
+
+			dns_list[0] = netconfig_ipv4_to_string(fils->ipv4_dns);
+			return dns_list;
+		}
 
-	if (!netconfig->dns6_overrides &&
-			netconfig->rtm_v6_protocol == RTPROT_DHCP) {
+		if (!(lease = l_dhcp_client_get_lease(netconfig->dhcp_client)))
+			return NULL;
+
+		return l_dhcp_lease_get_dns(lease);
+	} else {
 		const struct l_dhcp6_lease *lease;
 
-		if (fils && !l_memeqzero(fils->ipv6_dns,
-						16)) {
-			dns6_list = l_new(char *, 2);
-			dns6_list[0] = netconfig_ipv6_to_string(fils->ipv6_dns);
+		if (netconfig->dns6_overrides)
+			return l_strv_copy(netconfig->dns6_overrides);
+
+		if (netconfig->rtm_v6_protocol != RTPROT_DHCP)
+			return NULL;
+
+		if (fils && !l_memeqzero(fils->ipv6_dns, 16)) {
+			char **dns_list = l_new(char *, 2);
 
 			if (!l_memeqzero(fils->ipv6_dns_mac, 6) &&
+					out_dns_mac &&
 					util_ip_subnet_match(
 							fils->ipv6_prefix_len,
 							fils->ipv6_addr,
 							fils->ipv6_dns))
-				fils_dns6_mac = fils->ipv6_dns_mac;
-		} else if ((lease = l_dhcp6_client_get_lease(
+				*out_dns_mac = fils->ipv6_dns_mac;
+
+			dns_list[0] = netconfig_ipv6_to_string(fils->ipv6_dns);
+			return dns_list;
+		}
+
+		if (!(lease = l_dhcp6_client_get_lease(
 						netconfig->dhcp6_client)))
-			dns6_list = l_dhcp6_lease_get_dns(lease);
+			return NULL;
+
+		return l_dhcp6_lease_get_dns(lease);
 	}
+}
+
+static void netconfig_set_neighbor_entry_cb(int error,
+						uint16_t type, const void *data,
+						uint32_t len, void *user_data)
+{
+	if (error)
+		l_error("l_rtnl_neighbor_set_hwaddr failed: %s (%i)",
+			strerror(-error), error);
+}
+
+static int netconfig_set_dns(struct netconfig *netconfig)
+{
+	const uint8_t *fils_dns4_mac = NULL;
+	const uint8_t *fils_dns6_mac = NULL;
+	char **dns4_list = netconfig_get_dns_list(netconfig, AF_INET,
+							&fils_dns4_mac);
+	char **dns6_list = netconfig_get_dns_list(netconfig, AF_INET6,
+							&fils_dns6_mac);
+	unsigned int n_entries4 = l_strv_length(dns4_list);
+	unsigned int n_entries6 = l_strv_length(dns6_list);
+	char **dns_list;
+	const struct ie_fils_ip_addr_response_info *fils =
+		netconfig->fils_override;
 
-	n_entries += l_strv_length(netconfig->dns4_overrides);
-	n_entries += l_strv_length(netconfig->dns6_overrides);
-	n_entries += l_strv_length(dns4_list);
-	n_entries += l_strv_length(dns6_list);
+	if (!dns4_list && !dns6_list)
+		return 0;
 
-	dns_list = l_new(char *, n_entries + 1);
-	n_entries = 0;
+	dns_list = l_realloc(dns4_list,
+				sizeof(char *) * (n_entries4 + n_entries6 + 1));
+
+	if (dns6_list) {
+		memcpy(dns_list + n_entries4, dns6_list,
+			sizeof(char *) * (n_entries6 + 1));
+		/* Contents now belong to dns_list, so no l_strfreev */
+		l_free(dns6_list);
+	}
 
-	APPEND_STRDUPV(dns_list, n_entries, netconfig->dns4_overrides);
-	APPENDV(dns_list, n_entries, dns4_list);
-	APPEND_STRDUPV(dns_list, n_entries, netconfig->dns6_overrides);
-	APPENDV(dns_list, n_entries, dns6_list);
 	resolve_set_dns(netconfig->resolve, dns_list);
 
 	if (fils_dns4_mac && !l_rtnl_neighbor_set_hwaddr(rtnl,
@@ -284,9 +313,6 @@ static int netconfig_set_dns(struct netconfig *netconfig)
 		l_debug("l_rtnl_neighbor_set_hwaddr failed");
 
 	l_strv_free(dns_list);
-	/* Contents belonged to dns_list, so not l_strfreev */
-	l_free(dns4_list);
-	l_free(dns6_list);
 	return 0;
 }
 
@@ -402,6 +428,8 @@ static char *netconfig_ipv4_get_gateway(struct netconfig *netconfig,
 {
 	const struct l_dhcp_lease *lease;
 	char *gateway;
+	const struct ie_fils_ip_addr_response_info *fils =
+		netconfig->fils_override;
 
 	switch (netconfig->rtm_protocol) {
 	case RTPROT_STATIC:
@@ -415,15 +443,12 @@ static char *netconfig_ipv4_get_gateway(struct netconfig *netconfig,
 		return gateway;
 
 	case RTPROT_DHCP:
-		if (netconfig->fils_override &&
-				netconfig->fils_override->ipv4_gateway) {
-			gateway = netconfig_ipv4_to_string(
-					netconfig->fils_override->ipv4_gateway);
+		if (fils && fils->ipv4_gateway) {
+			gateway = netconfig_ipv4_to_string(fils->ipv4_gateway);
 
-			if (gateway && !l_memeqzero(netconfig->fils_override->
-							ipv4_gateway_mac, 6))
-				*out_mac = netconfig->fils_override->
-					ipv4_gateway_mac;
+			if (gateway && out_mac &&
+					!l_memeqzero(fils->ipv4_gateway_mac, 6))
+				*out_mac = fils->ipv4_gateway_mac;
 
 			return gateway;
 		}
@@ -477,10 +502,12 @@ no_prefix_len:
 
 static struct l_rtnl_route *netconfig_get_static6_gateway(
 						struct netconfig *netconfig,
+						char **out_str,
 						const uint8_t **out_mac)
 {
 	L_AUTO_FREE_VAR(char *, gateway);
 	struct l_rtnl_route *ret;
+	const uint8_t *mac = NULL;
 
 	gateway = l_settings_get_string(netconfig->active_settings,
 						"IPv6", "Gateway");
@@ -492,7 +519,7 @@ static struct l_rtnl_route *netconfig_get_static6_gateway(
 					netconfig->fils_override->ipv6_gateway);
 
 		if (!l_memeqzero(netconfig->fils_override->ipv6_gateway_mac, 6))
-			*out_mac = netconfig->fils_override->ipv6_gateway_mac;
+			mac = netconfig->fils_override->ipv6_gateway_mac;
 	} else if (!gateway)
 		return NULL;
 
@@ -506,6 +533,8 @@ static struct l_rtnl_route *netconfig_get_static6_gateway(
 
 	l_rtnl_route_set_priority(ret, ROUTE_PRIORITY_OFFSET);
 	l_rtnl_route_set_protocol(ret, RTPROT_STATIC);
+	*out_str = l_steal_ptr(gateway);
+	*out_mac = mac;
 
 	return ret;
 }
@@ -703,6 +732,43 @@ static void netconfig_route_generic_cb(int error, uint16_t type,
 	}
 }
 
+static void netconfig_connected(struct netconfig *netconfig)
+{
+	struct l_dbus *dbus = dbus_get_bus();
+
+	netconfig->connected = true;
+
+	if (netconfig->notify) {
+		netconfig->notify(NETCONFIG_EVENT_CONNECTED,
+					netconfig->user_data);
+		netconfig->notify = NULL;
+	}
+
+	if (netconfig->dbus_path &&
+			unlikely(!l_dbus_object_add_interface(dbus,
+						netconfig->dbus_path,
+						IWD_IPV4_CONFIG_INTERFACE,
+						&netconfig->ipv4_data)))
+		l_info("Unable to add %s at %s",
+			IWD_IPV4_CONFIG_INTERFACE, netconfig->dbus_path);
+
+	if (netconfig->dbus_path &&
+			unlikely(!l_dbus_object_add_interface(dbus,
+						netconfig->dbus_path,
+						IWD_IPV6_CONFIG_INTERFACE,
+						&netconfig->ipv6_data)))
+		l_info("Unable to add %s at %s",
+			IWD_IPV6_CONFIG_INTERFACE, netconfig->dbus_path);
+
+	if (netconfig->dbus_path &&
+			!l_dbus_object_add_interface(dbus, netconfig->dbus_path,
+						L_DBUS_INTERFACE_PROPERTIES,
+						netconfig))
+		/* Properties may already exist on the object, not an error */
+		l_debug("Unable to add %s at %s",
+			L_DBUS_INTERFACE_PROPERTIES, netconfig->dbus_path);
+}
+
 static void netconfig_route_add_cmd_cb(int error, uint16_t type,
 						const void *data, uint32_t len,
 						void *user_data)
@@ -717,11 +783,7 @@ static void netconfig_route_add_cmd_cb(int error, uint16_t type,
 		return;
 	}
 
-	if (!netconfig->notify)
-		return;
-
-	netconfig->notify(NETCONFIG_EVENT_CONNECTED, netconfig->user_data);
-	netconfig->notify = NULL;
+	netconfig_connected(netconfig);
 }
 
 static bool netconfig_ipv4_routes_install(struct netconfig *netconfig)
@@ -760,12 +822,7 @@ static bool netconfig_ipv4_routes_install(struct netconfig *netconfig)
 				netconfig->rtm_protocol == RTPROT_STATIC ?
 				"setting file" : "DHCPv4 lease");
 
-		if (netconfig->notify) {
-			netconfig->notify(NETCONFIG_EVENT_CONNECTED,
-						netconfig->user_data);
-			netconfig->notify = NULL;
-		}
-
+		netconfig_connected(netconfig);
 		return true;
 	}
 
@@ -832,7 +889,7 @@ static void netconfig_ipv6_ifaddr_add_cmd_cb(int error, uint16_t type,
 {
 	struct netconfig *netconfig = user_data;
 	struct l_rtnl_route *gateway;
-	const uint8_t *gateway_mac = NULL;
+	const uint8_t *gateway_mac;
 
 	netconfig->addr6_add_cmd_id = 0;
 
@@ -842,7 +899,9 @@ static void netconfig_ipv6_ifaddr_add_cmd_cb(int error, uint16_t type,
 		return;
 	}
 
-	gateway = netconfig_get_static6_gateway(netconfig, &gateway_mac);
+	gateway = netconfig_get_static6_gateway(netconfig,
+						&netconfig->v6_gateway_str,
+						&gateway_mac);
 	if (gateway) {
 		L_WARN_ON(!l_rtnl_route_add(rtnl, netconfig->ifindex,
 						gateway,
@@ -883,11 +942,44 @@ static void netconfig_ifaddr_del_cmd_cb(int error, uint16_t type,
 				"Error %d: %s", error, strerror(-error));
 }
 
+static bool netconfig_address_cmp_address(const struct l_rtnl_address *a,
+						const struct l_rtnl_address *b)
+{
+	char str_a[INET6_ADDRSTRLEN];
+	char str_b[INET6_ADDRSTRLEN];
+
+	if (a == b)
+		return true;
+
+	if (!a || !b)
+		return false;
+
+	if (!l_rtnl_address_get_address(a, str_a) ||
+			!l_rtnl_address_get_address(b, str_b))
+		return false;
+
+	return !strcmp(str_a, str_b);
+}
+
+static bool netconfig_address_cmp_prefix_len(const struct l_rtnl_address *a,
+						const struct l_rtnl_address *b)
+{
+	if (a == b)
+		return true;
+
+	if (!a || !b)
+		return false;
+
+	return l_rtnl_address_get_prefix_length(a) ==
+		l_rtnl_address_get_prefix_length(b);
+}
+
 static void netconfig_ipv4_dhcp_event_handler(struct l_dhcp_client *client,
 						enum l_dhcp_client_event event,
 						void *userdata)
 {
 	struct netconfig *netconfig = userdata;
+	struct l_dbus *dbus = dbus_get_bus();
 
 	l_debug("DHCPv4 event %d", event);
 
@@ -897,10 +989,47 @@ static void netconfig_ipv4_dhcp_event_handler(struct l_dhcp_client *client,
 					netconfig->v4_address,
 					netconfig_ifaddr_del_cmd_cb,
 					netconfig, NULL));
-		l_rtnl_address_free(netconfig->v4_address);
 		/* Fall through. */
 	case L_DHCP_CLIENT_EVENT_LEASE_OBTAINED:
-		netconfig->v4_address = netconfig_get_dhcp4_address(netconfig);
+	{
+		char *gateway_str;
+		struct l_rtnl_address *address;
+
+		gateway_str = netconfig_ipv4_get_gateway(netconfig, NULL);
+		if (l_streq0(netconfig->v4_gateway_str, gateway_str))
+			l_free(gateway_str);
+		else {
+			l_free(netconfig->v4_gateway_str);
+			netconfig->v4_gateway_str = gateway_str;
+
+			if (netconfig->dbus_path && netconfig->connected)
+				l_dbus_property_changed(dbus,
+						netconfig->dbus_path,
+						IWD_IPV4_CONFIG_INTERFACE,
+						"Gateway");
+		}
+
+		address = netconfig_get_dhcp4_address(netconfig);
+
+		if (netconfig->dbus_path && netconfig->connected &&
+				!netconfig_address_cmp_prefix_len(
+							netconfig->v4_address,
+							address))
+			l_dbus_property_changed(dbus, netconfig->dbus_path,
+						IWD_IPV4_CONFIG_INTERFACE,
+						"Netmask");
+
+		if (netconfig->dbus_path && netconfig->connected &&
+				netconfig_address_cmp_address(
+							netconfig->v4_address,
+							address))
+			l_dbus_property_changed(dbus, netconfig->dbus_path,
+						IWD_IPV4_CONFIG_INTERFACE,
+						"Address");
+
+		l_rtnl_address_free(netconfig->v4_address);
+		netconfig->v4_address = address;
+
 		if (!netconfig->v4_address) {
 			l_error("netconfig: Failed to obtain IP addresses from "
 							"DHCPv4 lease.");
@@ -913,6 +1042,7 @@ static void netconfig_ipv4_dhcp_event_handler(struct l_dhcp_client *client,
 					netconfig_ipv4_ifaddr_add_cmd_cb,
 					netconfig, NULL)));
 		break;
+	}
 	case L_DHCP_CLIENT_EVENT_LEASE_RENEWED:
 		break;
 	case L_DHCP_CLIENT_EVENT_LEASE_EXPIRED:
@@ -923,6 +1053,25 @@ static void netconfig_ipv4_dhcp_event_handler(struct l_dhcp_client *client,
 		l_rtnl_address_free(netconfig->v4_address);
 		netconfig->v4_address = NULL;
 
+		if (netconfig->dbus_path && netconfig->connected) {
+			l_dbus_property_changed(dbus, netconfig->dbus_path,
+						IWD_IPV4_CONFIG_INTERFACE,
+						"Address");
+			l_dbus_property_changed(dbus, netconfig->dbus_path,
+						IWD_IPV4_CONFIG_INTERFACE,
+						"Netmask");
+		}
+
+		if (netconfig->v4_gateway_str) {
+			if (netconfig->dbus_path && netconfig->connected)
+				l_dbus_property_changed(dbus,
+						netconfig->dbus_path,
+						IWD_IPV4_CONFIG_INTERFACE,
+						"Gateway");
+
+			l_free(l_steal_ptr(netconfig->v4_gateway_str));
+		}
+
 		/* Fall through. */
 	case L_DHCP_CLIENT_EVENT_NO_LEASE:
 		/*
@@ -945,18 +1094,87 @@ static void netconfig_dhcp6_event_handler(struct l_dhcp6_client *client,
 						void *userdata)
 {
 	struct netconfig *netconfig = userdata;
+	struct l_dbus *dbus = dbus_get_bus();
 
 	switch (event) {
 	case L_DHCP6_CLIENT_EVENT_IP_CHANGED:
 	case L_DHCP6_CLIENT_EVENT_LEASE_OBTAINED:
 	case L_DHCP6_CLIENT_EVENT_LEASE_RENEWED:
+	{
+		__auto_type lease =
+			l_dhcp6_client_get_lease(netconfig->dhcp6_client);
+		L_AUTO_FREE_VAR(char *, addr_str) =
+			l_dhcp6_lease_get_address(lease);
+		struct l_rtnl_address *address;
+		__auto_type icmp6 =
+			l_dhcp6_client_get_icmp6(netconfig->dhcp6_client);
+		__auto_type router = l_icmp6_client_get_router(icmp6);
+		char *gateway_str = l_icmp6_router_get_address(router);
+
+		if (l_streq0(netconfig->v6_gateway_str, gateway_str))
+			l_free(gateway_str);
+		else {
+			l_free(netconfig->v6_gateway_str);
+			netconfig->v6_gateway_str = gateway_str;
+
+			if (netconfig->dbus_path && netconfig->connected)
+				l_dbus_property_changed(dbus,
+						netconfig->dbus_path,
+						IWD_IPV6_CONFIG_INTERFACE,
+						"Gateway");
+		}
+
+		address = l_rtnl_address_new(addr_str,
+					l_dhcp6_lease_get_prefix_length(lease));
+
+		if (netconfig->dbus_path && netconfig->connected &&
+				!netconfig_address_cmp_prefix_len(
+							netconfig->v6_address,
+							address))
+			l_dbus_property_changed(dbus, netconfig->dbus_path,
+						IWD_IPV6_CONFIG_INTERFACE,
+						"Netmask");
+
+		if (netconfig->dbus_path && netconfig->connected &&
+				netconfig_address_cmp_address(
+							netconfig->v6_address,
+							address))
+			l_dbus_property_changed(dbus, netconfig->dbus_path,
+						IWD_IPV6_CONFIG_INTERFACE,
+						"Address");
+
+		l_rtnl_address_free(netconfig->v6_address);
+		netconfig->v4_address = address;
+
 		netconfig_set_dns(netconfig);
 		netconfig_set_domains(netconfig);
 		break;
+	}
 	case L_DHCP6_CLIENT_EVENT_LEASE_EXPIRED:
 		l_debug("Lease for interface %u expired", netconfig->ifindex);
 		netconfig_set_dns(netconfig);
 		netconfig_set_domains(netconfig);
+		l_rtnl_address_free(netconfig->v6_address);
+		netconfig->v6_address = NULL;
+
+		if (netconfig->dbus_path && netconfig->connected) {
+			l_dbus_property_changed(dbus, netconfig->dbus_path,
+						IWD_IPV6_CONFIG_INTERFACE,
+						"Address");
+			l_dbus_property_changed(dbus, netconfig->dbus_path,
+						IWD_IPV6_CONFIG_INTERFACE,
+						"Netmask");
+		}
+
+		if (netconfig->v6_gateway_str) {
+			if (netconfig->dbus_path && netconfig->connected)
+				l_dbus_property_changed(dbus,
+						netconfig->dbus_path,
+						IWD_IPV6_CONFIG_INTERFACE,
+						"Gateway");
+
+			l_free(l_steal_ptr(netconfig->v6_gateway_str));
+		}
 
 		/* Fall through */
 	case L_DHCP6_CLIENT_EVENT_NO_LEASE:
@@ -969,6 +1187,8 @@ static void netconfig_dhcp6_event_handler(struct l_dhcp6_client *client,
 
 static void netconfig_remove_v4_address(struct netconfig *netconfig)
 {
+	struct l_dbus *dbus = dbus_get_bus();
+
 	if (!netconfig->v4_address)
 		return;
 
@@ -978,6 +1198,15 @@ static void netconfig_remove_v4_address(struct netconfig *netconfig)
 					netconfig, NULL));
 	l_rtnl_address_free(netconfig->v4_address);
 	netconfig->v4_address = NULL;
+
+	if (netconfig->dbus_path && netconfig->connected) {
+		l_dbus_property_changed(dbus, netconfig->dbus_path,
+					IWD_IPV4_CONFIG_INTERFACE,
+					"Address");
+		l_dbus_property_changed(dbus, netconfig->dbus_path,
+					IWD_IPV4_CONFIG_INTERFACE,
+					"Netmask");
+	}
 }
 
 static void netconfig_reset_v4(struct netconfig *netconfig)
@@ -993,6 +1222,9 @@ static void netconfig_reset_v4(struct netconfig *netconfig)
 
 		l_acd_destroy(netconfig->acd);
 		netconfig->acd = NULL;
+
+		l_free(netconfig->v4_gateway_str);
+		netconfig->v4_gateway_str = NULL;
 	}
 }
 
@@ -1102,7 +1334,6 @@ static bool netconfig_ipv4_select_and_install(struct netconfig *netconfig)
 static bool netconfig_ipv6_select_and_install(struct netconfig *netconfig)
 {
 	struct netdev *netdev = netdev_find(netconfig->ifindex);
-	struct l_rtnl_address *address = NULL;
 
 	if (netconfig->rtm_v6_protocol == RTPROT_UNSPEC) {
 		l_debug("IPV6 configuration disabled");
@@ -1111,10 +1342,7 @@ static bool netconfig_ipv6_select_and_install(struct netconfig *netconfig)
 
 	sysfs_write_ipv6_setting(netdev_get_name(netdev), "disable_ipv6", "0");
 
-	if (netconfig->rtm_v6_protocol == RTPROT_STATIC)
-		address = netconfig_get_static6_address(
-						netconfig->active_settings);
-	else if (netconfig->rtm_v6_protocol == RTPROT_DHCP &&
+	if (netconfig->rtm_v6_protocol == RTPROT_DHCP &&
 			netconfig->fils_override &&
 			!l_memeqzero(netconfig->fils_override->ipv6_addr, 16)) {
 		uint8_t prefix_len = netconfig->fils_override->ipv6_prefix_len;
@@ -1124,11 +1352,12 @@ static bool netconfig_ipv6_select_and_install(struct netconfig *netconfig)
 		if (unlikely(!addr_str))
 			return false;
 
-		if (L_WARN_ON(unlikely(!(address = l_rtnl_address_new(addr_str,
-								prefix_len)))))
+		netconfig->v6_address = l_rtnl_address_new(addr_str,
+								prefix_len);
+		if (L_WARN_ON(unlikely(!netconfig->v6_address)))
 			return false;
 
-		l_rtnl_address_set_noprefixroute(address, true);
+		l_rtnl_address_set_noprefixroute(netconfig->v6_address, true);
 
 		/*
 		 * TODO: If netconfig->fils_override->ipv6_lifetime is set,
@@ -1138,12 +1367,12 @@ static bool netconfig_ipv6_select_and_install(struct netconfig *netconfig)
 		 */
 	}
 
-	if (address) {
+	if (netconfig->v6_address) {
 		L_WARN_ON(!(netconfig->addr6_add_cmd_id =
-			l_rtnl_ifaddr_add(rtnl, netconfig->ifindex, address,
+			l_rtnl_ifaddr_add(rtnl, netconfig->ifindex,
+					netconfig->v6_address,
 					netconfig_ipv6_ifaddr_add_cmd_cb,
 					netconfig, NULL)));
-		l_rtnl_address_free(address);
 		return true;
 	}
 
@@ -1258,7 +1487,6 @@ bool netconfig_load_settings(struct netconfig *netconfig,
 
 	if (l_settings_has_key(active_settings, "IPv6", "Address")) {
 		v6_address = netconfig_get_static6_address(active_settings);
-		l_rtnl_address_free(v6_address);
 
 		if (unlikely(!v6_address)) {
 			l_error("netconfig: Can't parse IPv6 address");
@@ -1276,11 +1504,14 @@ bool netconfig_load_settings(struct netconfig *netconfig,
 
 	if (!v6_enabled)
 		netconfig->rtm_v6_protocol = RTPROT_UNSPEC;
-	else if (v6_address)
+	else if (v6_address) {
+		netconfig->v6_address = l_steal_ptr(v6_address);
 		netconfig->rtm_v6_protocol = RTPROT_STATIC;
-	else
+	} else
 		netconfig->rtm_v6_protocol = RTPROT_DHCP;
 
+	l_rtnl_address_free(v6_address);
+
 	if (send_hostname)
 		l_dhcp_client_set_hostname(netconfig->dhcp_client, hostname);
 
@@ -1339,6 +1570,16 @@ bool netconfig_reconfigure(struct netconfig *netconfig)
 bool netconfig_reset(struct netconfig *netconfig)
 {
 	struct netdev *netdev = netdev_find(netconfig->ifindex);
+	struct l_dbus *dbus = dbus_get_bus();
+
+	if (netconfig->dbus_path && netconfig->connected) {
+		l_dbus_object_remove_interface(dbus, netconfig->dbus_path,
+						IWD_IPV4_CONFIG_INTERFACE);
+		l_dbus_object_remove_interface(dbus, netconfig->dbus_path,
+						IWD_IPV6_CONFIG_INTERFACE);
+	}
+
+	netconfig->connected = false;
 
 	if (netconfig->route4_add_gateway_cmd_id) {
 		l_netlink_cancel(rtnl, netconfig->route4_add_gateway_cmd_id);
@@ -1361,6 +1602,9 @@ bool netconfig_reset(struct netconfig *netconfig)
 	netconfig_reset_v4(netconfig);
 
 	if (netconfig->rtm_v6_protocol) {
+		l_rtnl_address_free(netconfig->v6_address);
+		netconfig->v6_address = NULL;
+
 		l_strfreev(netconfig->dns6_overrides);
 		netconfig->dns6_overrides = NULL;
 
@@ -1369,6 +1613,9 @@ bool netconfig_reset(struct netconfig *netconfig)
 
 		sysfs_write_ipv6_setting(netdev_get_name(netdev),
 						"disable_ipv6", "1");
+
+		l_free(netconfig->v6_gateway_str);
+		netconfig->v6_gateway_str = NULL;
 	}
 
 	l_free(l_steal_ptr(netconfig->fils_override));
@@ -1419,7 +1666,7 @@ void netconfig_handle_fils_ip_resp(struct netconfig *netconfig,
 	netconfig->fils_override = l_memdup(info, sizeof(*info));
 }
 
-struct netconfig *netconfig_new(uint32_t ifindex)
+struct netconfig *netconfig_new(uint32_t ifindex, const char *dbus_path)
 {
 	struct netdev *netdev = netdev_find(ifindex);
 	struct netconfig *netconfig;
@@ -1436,6 +1683,7 @@ struct netconfig *netconfig_new(uint32_t ifindex)
 
 	netconfig = l_new(struct netconfig, 1);
 	netconfig->ifindex = ifindex;
+	netconfig->dbus_path = l_strdup(dbus_path);
 	netconfig->resolve = resolve_new(ifindex);
 
 	netconfig->dhcp_client = l_dhcp_client_new(ifindex);
@@ -1468,6 +1716,19 @@ struct netconfig *netconfig_new(uint32_t ifindex)
 	sysfs_write_ipv6_setting(netdev_get_name(netdev), "accept_ra", "0");
 	sysfs_write_ipv6_setting(netdev_get_name(netdev), "disable_ipv6", "1");
 
+	/*
+	 * Use struct interface_data as interface user_data so that the
+	 * property getters can check which interface they're being called
+	 * for.  For the org.freedesktop.DBus.Properties.{Get,GetAll} calls
+	 * we can read the interface name from the first argument but the
+	 * getters are also called when the InterfaceAdded signal messages
+	 * is being built.
+	 */
+	netconfig->ipv4_data.is_ipv4 = true;
+	netconfig->ipv4_data.netconfig = netconfig;
+	netconfig->ipv6_data.is_ipv4 = false;
+	netconfig->ipv6_data.netconfig = netconfig;
+
 	return netconfig;
 }
 
@@ -1485,6 +1746,174 @@ void netconfig_destroy(struct netconfig *netconfig)
 	netconfig_free(netconfig);
 }
 
+static bool netconfig_property_get_method(struct l_dbus *dbus,
+					struct l_dbus_message *message,
+					struct l_dbus_message_builder *builder,
+					void *user_data)
+{
+	const struct interface_data *data = user_data;
+	uint8_t protocol;
+
+	protocol = data->is_ipv4 ?  data->netconfig->rtm_protocol :
+		data->netconfig->rtm_v6_protocol;
+
+	l_dbus_message_builder_append_basic(builder, 's',
+						protocol == RTPROT_DHCP ?
+						"auto" : "static");
+	return true;
+}
+
+static bool netconfig_property_get_address(struct l_dbus *dbus,
+					struct l_dbus_message *message,
+					struct l_dbus_message_builder *builder,
+					void *user_data)
+{
+	const struct interface_data *data = user_data;
+	const struct l_rtnl_address *address;
+	char ip_str[INET6_ADDRSTRLEN];
+
+	address = data->is_ipv4 ? data->netconfig->v4_address :
+		data->netconfig->v6_address;
+
+	if (!address || !l_rtnl_address_get_address(address, ip_str))
+		return false;
+
+	l_dbus_message_builder_append_basic(builder, 's', ip_str);
+	return true;
+}
+
+static bool netconfig_property_get_prefix(struct l_dbus *dbus,
+					struct l_dbus_message *message,
+					struct l_dbus_message_builder *builder,
+					void *user_data)
+{
+	const struct interface_data *data = user_data;
+	const struct l_rtnl_address *address;
+	uint8_t prefix_len;
+
+	address = data->is_ipv4 ?
+		data->netconfig->v4_address : data->netconfig->v6_address;
+
+	if (!address || !(prefix_len =
+				l_rtnl_address_get_prefix_length(address)))
+		return false;
+
+	l_dbus_message_builder_append_basic(builder, 'y', &prefix_len);
+	return true;
+}
+
+static bool netconfig_property_get_gateway(struct l_dbus *dbus,
+					struct l_dbus_message *message,
+					struct l_dbus_message_builder *builder,
+					void *user_data)
+{
+	const struct interface_data *data = user_data;
+	const char *gateway_str = data->is_ipv4 ?
+		data->netconfig->v4_gateway_str :
+		data->netconfig->v6_gateway_str;
+
+	if (!gateway_str)
+		return false;
+
+	l_dbus_message_builder_append_basic(builder, 's', gateway_str);
+	return true;
+}
+
+static bool netconfig_property_get_dnses(struct l_dbus *dbus,
+					struct l_dbus_message *message,
+					struct l_dbus_message_builder *builder,
+					void *user_data)
+{
+	const struct interface_data *data = user_data;
+	char **dns_str_list = netconfig_get_dns_list(data->netconfig,
+					data->is_ipv4 ? AF_INET : AF_INET6,
+					NULL);
+	char **i;
+
+	l_dbus_message_builder_enter_array(builder, "s");
+
+	for (i = dns_str_list; i && *i; i++)
+		l_dbus_message_builder_append_basic(builder, 's', *i);
+
+	l_dbus_message_builder_leave_array(builder);
+	l_strv_free(dns_str_list);
+	return true;
+}
+
+static bool netconfig_property_get_domains(struct l_dbus *dbus,
+					struct l_dbus_message *message,
+					struct l_dbus_message_builder *builder,
+					void *user_data)
+{
+	const struct interface_data *data = user_data;
+
+	if (data->is_ipv4) {
+		L_AUTO_FREE_VAR(char *, domain) =
+			l_settings_get_string(data->netconfig->active_settings,
+						"IPv4", "DomainName");
+
+		if (!domain) {
+			const struct l_dhcp_lease *lease;
+
+			if (data->netconfig->rtm_protocol != RTPROT_DHCP)
+				return false;
+
+			lease = l_dhcp_client_get_lease(
+						data->netconfig->dhcp_client);
+			if (!lease)
+				return false;
+
+			if (!(domain = l_dhcp_lease_get_domain_name(lease)))
+				return false;
+		}
+
+		l_dbus_message_builder_enter_array(builder, "s");
+		l_dbus_message_builder_append_basic(builder, 's', domain);
+		l_dbus_message_builder_leave_array(builder);
+	} else {
+		const struct l_dhcp6_lease *lease;
+		char **domains;
+		char **i;
+
+		if (data->netconfig->rtm_v6_protocol != RTPROT_DHCP)
+			return false;
+
+		lease = l_dhcp6_client_get_lease(data->netconfig->dhcp6_client);
+		if (!lease)
+			return false;
+
+		domains = l_dhcp6_lease_get_domains(lease);
+		if (!domains)
+			return false;
+
+		l_dbus_message_builder_enter_array(builder, "s");
+
+		for (i = domains; *i; i++)
+			l_dbus_message_builder_append_basic(builder, 's', *i);
+
+		l_dbus_message_builder_leave_array(builder);
+		l_strv_free(domains);
+	}
+
+	return true;
+}
+
+static void netconfig_setup_interface(struct l_dbus_interface *interface)
+{
+	l_dbus_interface_property(interface, "Method", 0, "s",
+					netconfig_property_get_method, NULL);
+	l_dbus_interface_property(interface, "Address", 0, "s",
+					netconfig_property_get_address, NULL);
+	l_dbus_interface_property(interface, "PrefixLength", 0, "y",
+					netconfig_property_get_prefix, NULL);
+	l_dbus_interface_property(interface, "Gateway", 0, "s",
+					netconfig_property_get_gateway, NULL);
+	l_dbus_interface_property(interface, "DomainNameServers", 0, "as",
+					netconfig_property_get_dnses, NULL);
+	l_dbus_interface_property(interface, "DomainNames", 0, "as",
+					netconfig_property_get_domains, NULL);
+}
+
 static int netconfig_init(void)
 {
 	uint32_t r;
@@ -1536,6 +1965,14 @@ static int netconfig_init(void)
 
 	netconfig_list = l_queue_new();
 
+	L_WARN_ON(unlikely(!l_dbus_register_interface(dbus_get_bus(),
+						IWD_IPV4_CONFIG_INTERFACE,
+						netconfig_setup_interface,
+						NULL, false)));
+	L_WARN_ON(unlikely(!l_dbus_register_interface(dbus_get_bus(),
+						IWD_IPV6_CONFIG_INTERFACE,
+						netconfig_setup_interface,
+						NULL, false)));
 	return 0;
 
 error:
@@ -1552,6 +1989,9 @@ static void netconfig_exit(void)
 	rtnl = NULL;
 
 	l_queue_destroy(netconfig_list, netconfig_free);
+
+	l_dbus_unregister_interface(dbus_get_bus(), IWD_IPV4_CONFIG_INTERFACE);
+	l_dbus_unregister_interface(dbus_get_bus(), IWD_IPV6_CONFIG_INTERFACE);
 }
 
 IWD_MODULE(netconfig, netconfig_init, netconfig_exit)
diff --git a/src/netconfig.h b/src/netconfig.h
index fa46c7c8..5d54d9cc 100644
--- a/src/netconfig.h
+++ b/src/netconfig.h
@@ -45,5 +45,5 @@ bool netconfig_get_fils_ip_req(struct netconfig *netconfig,
 void netconfig_handle_fils_ip_resp(struct netconfig *netconfig,
 			const struct ie_fils_ip_addr_response_info *info);
 
-struct netconfig *netconfig_new(uint32_t ifindex);
+struct netconfig *netconfig_new(uint32_t ifindex, const char *dbus_path);
 void netconfig_destroy(struct netconfig *netconfig);
diff --git a/src/p2p.c b/src/p2p.c
index 3328271b..b68e4f7c 100644
--- a/src/p2p.c
+++ b/src/p2p.c
@@ -1320,7 +1320,9 @@ static void p2p_start_client_netconfig(struct p2p_device *dev)
 	struct l_settings *settings;
 
 	if (!dev->conn_netconfig) {
-		dev->conn_netconfig = netconfig_new(ifindex);
+		const char *peer_path = p2p_peer_get_path(dev->conn_peer);
+
+		dev->conn_netconfig = netconfig_new(ifindex, peer_path);
 		if (!dev->conn_netconfig) {
 			p2p_connect_failed(dev);
 			return;
diff --git a/src/station.c b/src/station.c
index ac741f5a..a6b5922f 100644
--- a/src/station.c
+++ b/src/station.c
@@ -3577,6 +3577,7 @@ static struct station *station_create(struct netdev *netdev)
 	struct station *station;
 	struct l_dbus *dbus = dbus_get_bus();
 	bool autoconnect = true;
+	const char *path;
 
 	station = l_new(struct station, 1);
 	watchlist_init(&station->state_watches, NULL);
@@ -3594,11 +3595,12 @@ static struct station *station_create(struct netdev *netdev)
 
 	l_queue_push_head(station_list, station);
 
-	l_dbus_object_add_interface(dbus, netdev_get_path(netdev),
-					IWD_STATION_INTERFACE, station);
+	path = netdev_get_path(netdev);
+	l_dbus_object_add_interface(dbus, path, IWD_STATION_INTERFACE, station);
 
 	if (netconfig_enabled)
-		station->netconfig = netconfig_new(netdev_get_ifindex(netdev));
+		station->netconfig = netconfig_new(netdev_get_ifindex(netdev),
+							path);
 
 	station->anqp_pending = l_queue_new();
 
-- 
2.30.2

Re: [PATCH 2/2] netconfig: Add IP configuration properties on Station and P2P

[Denis Kenzior]

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

Hi Andrew,

On 9/8/21 4:44 PM, Andrew Zaborowski wrote:
> Add the IPv{4,6}Configuration D-Bus interfaces on the station and P2P
> objects that expose current netconfig values.  Access Point and P2P-GO
> are not handled yet, and some of the IP config properties may not
> automatically generate PropertiesChanged on DHCP lease renewals but
> their values are unlikely to change.
> ---
>   src/dbus.h      |   2 +
>   src/netconfig.c | 640 ++++++++++++++++++++++++++++++++++++++++--------
>   src/netconfig.h |   2 +-
>   src/p2p.c       |   4 +-
>   src/station.c   |   8 +-
>   5 files changed, 551 insertions(+), 105 deletions(-)
> 

<snip>

> @@ -152,20 +164,6 @@ static struct netconfig *netconfig_find(uint32_t ifindex)
>   	return NULL;
>   }
>   
> -#define APPEND_STRDUPV(dest, index, src)			\
> -	do {							\
> -		char **p;					\
> -		for (p = src; p && *p; p++)			\
> -			dest[index++] = l_strdup(*p);		\
> -	} while (0)						\
> -
> -#define APPENDV(dest, index, src)				\
> -	do {							\
> -		char **p;					\
> -		for (p = src; p && *p; p++)			\
> -			dest[index++] = *p;			\
> -	} while (0)						\
> -
>   static inline char *netconfig_ipv4_to_string(uint32_t addr)
>   {
>   	struct in_addr in_addr = { .s_addr = addr };
> @@ -196,77 +194,108 @@ static inline char *netconfig_ipv6_to_string(const uint8_t *addr)
>   	return addr_str;
>   }
>   
> -static void netconfig_set_neighbor_entry_cb(int error,
> -						uint16_t type, const void *data,
> -						uint32_t len, void *user_data)
> -{
> -	if (error)
> -		l_error("l_rtnl_neighbor_set_hwaddr failed: %s (%i)",
> -			strerror(-error), error);
> -}
> -
> -static int netconfig_set_dns(struct netconfig *netconfig)
> +static char **netconfig_get_dns_list(struct netconfig *netconfig, int af,
> +					const uint8_t **out_dns_mac)
>   {
> -	char **dns6_list = NULL;
> -	char **dns4_list = NULL;
> -	unsigned int n_entries = 0;
> -	char **dns_list;
> -	const uint8_t *fils_dns4_mac = NULL;
> -	const uint8_t *fils_dns6_mac = NULL;
>   	const struct ie_fils_ip_addr_response_info *fils =
>   		netconfig->fils_override;
>   
> -	if (!netconfig->dns4_overrides &&
> -			netconfig->rtm_protocol == RTPROT_DHCP) {
> +	if (af == AF_INET) {
>   		const struct l_dhcp_lease *lease;
>   
> +		if (netconfig->dns4_overrides)
> +			return l_strv_copy(netconfig->dns4_overrides);
> +
> +		if (netconfig->rtm_protocol != RTPROT_DHCP)
> +			return NULL;
> +
>   		if (fils && fils->ipv4_dns) {
> -			dns4_list = l_new(char *, 2);
> -			dns4_list[0] = netconfig_ipv4_to_string(fils->ipv4_dns);
> +			char **dns_list = l_new(char *, 2);
>   
>   			if (!l_memeqzero(fils->ipv4_dns_mac, 6) &&
> +					out_dns_mac &&
>   					util_ip_subnet_match(
>   							fils->ipv4_prefix_len,
>   							&fils->ipv4_addr,
>   							&fils->ipv4_dns))
> -				fils_dns4_mac = fils->ipv4_dns_mac;
> -		} else if ((lease = l_dhcp_client_get_lease(
> -						netconfig->dhcp_client)))
> -			dns4_list = l_dhcp_lease_get_dns(lease);
> -	}
> +				*out_dns_mac = fils->ipv4_dns_mac;
> +
> +			dns_list[0] = netconfig_ipv4_to_string(fils->ipv4_dns);
> +			return dns_list;
> +		}
>   
> -	if (!netconfig->dns6_overrides &&
> -			netconfig->rtm_v6_protocol == RTPROT_DHCP) {
> +		if (!(lease = l_dhcp_client_get_lease(netconfig->dhcp_client)))
> +			return NULL;
> +
> +		return l_dhcp_lease_get_dns(lease);
> +	} else {
>   		const struct l_dhcp6_lease *lease;
>   
> -		if (fils && !l_memeqzero(fils->ipv6_dns,
> -						16)) {
> -			dns6_list = l_new(char *, 2);
> -			dns6_list[0] = netconfig_ipv6_to_string(fils->ipv6_dns);
> +		if (netconfig->dns6_overrides)
> +			return l_strv_copy(netconfig->dns6_overrides);
> +
> +		if (netconfig->rtm_v6_protocol != RTPROT_DHCP)
> +			return NULL;
> +
> +		if (fils && !l_memeqzero(fils->ipv6_dns, 16)) {
> +			char **dns_list = l_new(char *, 2);
>   
>   			if (!l_memeqzero(fils->ipv6_dns_mac, 6) &&
> +					out_dns_mac &&
>   					util_ip_subnet_match(
>   							fils->ipv6_prefix_len,
>   							fils->ipv6_addr,
>   							fils->ipv6_dns))
> -				fils_dns6_mac = fils->ipv6_dns_mac;
> -		} else if ((lease = l_dhcp6_client_get_lease(
> +				*out_dns_mac = fils->ipv6_dns_mac;
> +
> +			dns_list[0] = netconfig_ipv6_to_string(fils->ipv6_dns);
> +			return dns_list;
> +		}
> +
> +		if (!(lease = l_dhcp6_client_get_lease(
>   						netconfig->dhcp6_client)))
> -			dns6_list = l_dhcp6_lease_get_dns(lease);
> +			return NULL;
> +
> +		return l_dhcp6_lease_get_dns(lease);
>   	}
> +}
> +
> +static void netconfig_set_neighbor_entry_cb(int error,
> +						uint16_t type, const void *data,
> +						uint32_t len, void *user_data)
> +{
> +	if (error)
> +		l_error("l_rtnl_neighbor_set_hwaddr failed: %s (%i)",
> +			strerror(-error), error);
> +}
> +
> +static int netconfig_set_dns(struct netconfig *netconfig)
> +{
> +	const uint8_t *fils_dns4_mac = NULL;
> +	const uint8_t *fils_dns6_mac = NULL;
> +	char **dns4_list = netconfig_get_dns_list(netconfig, AF_INET,
> +							&fils_dns4_mac);
> +	char **dns6_list = netconfig_get_dns_list(netconfig, AF_INET6,
> +							&fils_dns6_mac);
> +	unsigned int n_entries4 = l_strv_length(dns4_list);
> +	unsigned int n_entries6 = l_strv_length(dns6_list);
> +	char **dns_list;
> +	const struct ie_fils_ip_addr_response_info *fils =
> +		netconfig->fils_override;
>   
> -	n_entries += l_strv_length(netconfig->dns4_overrides);
> -	n_entries += l_strv_length(netconfig->dns6_overrides);
> -	n_entries += l_strv_length(dns4_list);
> -	n_entries += l_strv_length(dns6_list);
> +	if (!dns4_list && !dns6_list)
> +		return 0;
>   
> -	dns_list = l_new(char *, n_entries + 1);
> -	n_entries = 0;
> +	dns_list = l_realloc(dns4_list,
> +				sizeof(char *) * (n_entries4 + n_entries6 + 1));
> +
> +	if (dns6_list) {
> +		memcpy(dns_list + n_entries4, dns6_list,
> +			sizeof(char *) * (n_entries6 + 1));
> +		/* Contents now belong to dns_list, so no l_strfreev */
> +		l_free(dns6_list);
> +	}
>   
> -	APPEND_STRDUPV(dns_list, n_entries, netconfig->dns4_overrides);
> -	APPENDV(dns_list, n_entries, dns4_list);
> -	APPEND_STRDUPV(dns_list, n_entries, netconfig->dns6_overrides);
> -	APPENDV(dns_list, n_entries, dns6_list);
>   	resolve_set_dns(netconfig->resolve, dns_list);
>   
>   	if (fils_dns4_mac && !l_rtnl_neighbor_set_hwaddr(rtnl,
> @@ -284,9 +313,6 @@ static int netconfig_set_dns(struct netconfig *netconfig)
>   		l_debug("l_rtnl_neighbor_set_hwaddr failed");
>   
>   	l_strv_free(dns_list);
> -	/* Contents belonged to dns_list, so not l_strfreev */
> -	l_free(dns4_list);
> -	l_free(dns6_list);
>   	return 0;
>   }
>   

I applied this part separately in commit 8b573fe3984d ("netconfig: Refactor 
netconfig_set_dns")

<snip>

> @@ -703,6 +732,43 @@ static void netconfig_route_generic_cb(int error, uint16_t type,
>   	}
>   }
>   
> +static void netconfig_connected(struct netconfig *netconfig)
> +{
> +	struct l_dbus *dbus = dbus_get_bus();
> +
> +	netconfig->connected = true;
> +
> +	if (netconfig->notify) {
> +		netconfig->notify(NETCONFIG_EVENT_CONNECTED,
> +					netconfig->user_data);
> +		netconfig->notify = NULL;
> +	}
> +
> +	if (netconfig->dbus_path &&
> +			unlikely(!l_dbus_object_add_interface(dbus,
> +						netconfig->dbus_path,
> +						IWD_IPV4_CONFIG_INTERFACE,
> +						&netconfig->ipv4_data)))
> +		l_info("Unable to add %s at %s",
> +			IWD_IPV4_CONFIG_INTERFACE, netconfig->dbus_path);
> +
> +	if (netconfig->dbus_path &&
> +			unlikely(!l_dbus_object_add_interface(dbus,
> +						netconfig->dbus_path,
> +						IWD_IPV6_CONFIG_INTERFACE,
> +						&netconfig->ipv6_data)))
> +		l_info("Unable to add %s at %s",
> +			IWD_IPV6_CONFIG_INTERFACE, netconfig->dbus_path);
> +
> +	if (netconfig->dbus_path &&
> +			!l_dbus_object_add_interface(dbus, netconfig->dbus_path,
> +						L_DBUS_INTERFACE_PROPERTIES,
> +						netconfig))
> +		/* Properties may already exist on the object, not an error */
> +		l_debug("Unable to add %s at %s",
> +			L_DBUS_INTERFACE_PROPERTIES, netconfig->dbus_path);

So the connected event is fired once we have IPv4 or IPv6 configuration, right?

Why would we populate both IPv6 and IPv4 interfaces here though?  Why don't you 
just make IPv6 interface available when IPv6 is configured, and similarly for 
IPv4?  Would make a lot of the code simpler and require less 
l_dbus_property_changed() calls.

> +}
> +
>   static void netconfig_route_add_cmd_cb(int error, uint16_t type,
>   						const void *data, uint32_t len,
>   						void *user_data)

<snip>

> @@ -897,10 +989,47 @@ static void netconfig_ipv4_dhcp_event_handler(struct l_dhcp_client *client,
>   					netconfig->v4_address,
>   					netconfig_ifaddr_del_cmd_cb,
>   					netconfig, NULL));
> -		l_rtnl_address_free(netconfig->v4_address);
>   		/* Fall through. */
>   	case L_DHCP_CLIENT_EVENT_LEASE_OBTAINED:
> -		netconfig->v4_address = netconfig_get_dhcp4_address(netconfig);
> +	{
> +		char *gateway_str;
> +		struct l_rtnl_address *address;
> +
> +		gateway_str = netconfig_ipv4_get_gateway(netconfig, NULL);
> +		if (l_streq0(netconfig->v4_gateway_str, gateway_str))
> +			l_free(gateway_str);
> +		else {
> +			l_free(netconfig->v4_gateway_str);
> +			netconfig->v4_gateway_str = gateway_str;
> +
> +			if (netconfig->dbus_path && netconfig->connected)
> +				l_dbus_property_changed(dbus,
> +						netconfig->dbus_path,
> +						IWD_IPV4_CONFIG_INTERFACE,
> +						"Gateway");
> +		}

So in theory LEASE_OBTAINED should only ever be signaled once.  Why not register 
the IPv4 interface now and avoid all this logic?  If we lose the lease for some 
reason, then we should get a LEASE_LOST or LEASE_EXPIRED event first.

> +
> +		address = netconfig_get_dhcp4_address(netconfig);
> +
> +		if (netconfig->dbus_path && netconfig->connected &&
> +				!netconfig_address_cmp_prefix_len(
> +							netconfig->v4_address,
> +							address))
> +			l_dbus_property_changed(dbus, netconfig->dbus_path,
> +						IWD_IPV4_CONFIG_INTERFACE,
> +						"Netmask");
> +
> +		if (netconfig->dbus_path && netconfig->connected &&
> +				netconfig_address_cmp_address(
> +							netconfig->v4_address,
> +							address))
> +			l_dbus_property_changed(dbus, netconfig->dbus_path,
> +						IWD_IPV4_CONFIG_INTERFACE,
> +						"Address");
> +
> +		l_rtnl_address_free(netconfig->v4_address);
> +		netconfig->v4_address = address;
> +
>   		if (!netconfig->v4_address) {
>   			l_error("netconfig: Failed to obtain IP addresses from "
>   							"DHCPv4 lease.");
> @@ -913,6 +1042,7 @@ static void netconfig_ipv4_dhcp_event_handler(struct l_dhcp_client *client,
>   					netconfig_ipv4_ifaddr_add_cmd_cb,
>   					netconfig, NULL)));
>   		break;
> +	}
>   	case L_DHCP_CLIENT_EVENT_LEASE_RENEWED:
>   		break;
>   	case L_DHCP_CLIENT_EVENT_LEASE_EXPIRED:
> @@ -923,6 +1053,25 @@ static void netconfig_ipv4_dhcp_event_handler(struct l_dhcp_client *client,
>   		l_rtnl_address_free(netconfig->v4_address);
>   		netconfig->v4_address = NULL;
>   
> +		if (netconfig->dbus_path && netconfig->connected) {
> +			l_dbus_property_changed(dbus, netconfig->dbus_path,
> +						IWD_IPV4_CONFIG_INTERFACE,
> +						"Address");
> +			l_dbus_property_changed(dbus, netconfig->dbus_path,
> +						IWD_IPV4_CONFIG_INTERFACE,
> +						"Netmask");
> +		}
> +
> +		if (netconfig->v4_gateway_str) {
> +			if (netconfig->dbus_path && netconfig->connected)
> +				l_dbus_property_changed(dbus,
> +						netconfig->dbus_path,
> +						IWD_IPV4_CONFIG_INTERFACE,
> +						"Gateway");
> +
> +			l_free(l_steal_ptr(netconfig->v4_gateway_str));
> +		}

Why not simply remove the IPv4 interface?

> +
>   		/* Fall through. */
>   	case L_DHCP_CLIENT_EVENT_NO_LEASE:
>   		/*
> @@ -945,18 +1094,87 @@ static void netconfig_dhcp6_event_handler(struct l_dhcp6_client *client,
>   						void *userdata)
>   {
>   	struct netconfig *netconfig = userdata;
> +	struct l_dbus *dbus = dbus_get_bus();
>   
>   	switch (event) {
>   	case L_DHCP6_CLIENT_EVENT_IP_CHANGED:
>   	case L_DHCP6_CLIENT_EVENT_LEASE_OBTAINED:
>   	case L_DHCP6_CLIENT_EVENT_LEASE_RENEWED:
> +	{
> +		__auto_type lease =
> +			l_dhcp6_client_get_lease(netconfig->dhcp6_client);
> +		L_AUTO_FREE_VAR(char *, addr_str) =
> +			l_dhcp6_lease_get_address(lease);
> +		struct l_rtnl_address *address;
> +		__auto_type icmp6 =
> +			l_dhcp6_client_get_icmp6(netconfig->dhcp6_client);
> +		__auto_type router = l_icmp6_client_get_router(icmp6);
> +		char *gateway_str = l_icmp6_router_get_address(router);
> +
> +		if (l_streq0(netconfig->v6_gateway_str, gateway_str))
> +			l_free(gateway_str);
> +		else {
> +			l_free(netconfig->v6_gateway_str);
> +			netconfig->v6_gateway_str = gateway_str;
> +
> +			if (netconfig->dbus_path && netconfig->connected)
> +				l_dbus_property_changed(dbus,
> +						netconfig->dbus_path,
> +						IWD_IPV6_CONFIG_INTERFACE,
> +						"Gateway");
> +		}
> +
> +		address = l_rtnl_address_new(addr_str,
> +					l_dhcp6_lease_get_prefix_length(lease));
> +
> +		if (netconfig->dbus_path && netconfig->connected &&
> +				!netconfig_address_cmp_prefix_len(
> +							netconfig->v6_address,
> +							address))
> +			l_dbus_property_changed(dbus, netconfig->dbus_path,
> +						IWD_IPV6_CONFIG_INTERFACE,
> +						"Netmask");
> +
> +		if (netconfig->dbus_path && netconfig->connected &&
> +				netconfig_address_cmp_address(
> +							netconfig->v6_address,
> +							address))
> +			l_dbus_property_changed(dbus, netconfig->dbus_path,
> +						IWD_IPV6_CONFIG_INTERFACE,
> +						"Address");
> +
> +		l_rtnl_address_free(netconfig->v6_address);
> +		netconfig->v4_address = address;

v6 address?

Also similar comments as above?  Why not just register the IPv6 interface here 
instead?

Also, I'm really not sure whether just Netmask/Address/Gateway are the only 
things that can change.  DNS, NTP, domain, etc information might also change. 
So a more generic old lease / new lease comparison might be needed.  Or always 
send a PropertyChanged for all attributes and have the caller figure it out?

<snip>

> +static bool netconfig_property_get_domains(struct l_dbus *dbus,
> +					struct l_dbus_message *message,
> +					struct l_dbus_message_builder *builder,
> +					void *user_data)
> +{
> +	const struct interface_data *data = user_data;
> +
> +	if (data->is_ipv4) {
> +		L_AUTO_FREE_VAR(char *, domain) =
> +			l_settings_get_string(data->netconfig->active_settings,
> +						"IPv4", "DomainName");
> +
> +		if (!domain) {
> +			const struct l_dhcp_lease *lease;
> +
> +			if (data->netconfig->rtm_protocol != RTPROT_DHCP)
> +				return false;
> +
> +			lease = l_dhcp_client_get_lease(
> +						data->netconfig->dhcp_client);
> +			if (!lease)
> +				return false;
> +
> +			if (!(domain = l_dhcp_lease_get_domain_name(lease)))
> +				return false;
> +		}
> +
> +		l_dbus_message_builder_enter_array(builder, "s");
> +		l_dbus_message_builder_append_basic(builder, 's', domain);
> +		l_dbus_message_builder_leave_array(builder);
> +	} else {
> +		const struct l_dhcp6_lease *lease;
> +		char **domains;
> +		char **i;
> +
> +		if (data->netconfig->rtm_v6_protocol != RTPROT_DHCP)
> +			return false;
> +
> +		lease = l_dhcp6_client_get_lease(data->netconfig->dhcp6_client);
> +		if (!lease)
> +			return false;
> +
> +		domains = l_dhcp6_lease_get_domains(lease);
> +		if (!domains)
> +			return false;
> +
> +		l_dbus_message_builder_enter_array(builder, "s");
> +
> +		for (i = domains; *i; i++)
> +			l_dbus_message_builder_append_basic(builder, 's', *i);
> +
> +		l_dbus_message_builder_leave_array(builder);
> +		l_strv_free(domains);
> +	}
> +
> +	return true;

Might as well make this two separate functions

> +}
> +

Regards,
-Denis

Re: [PATCH 2/2] netconfig: Add IP configuration properties on Station and P2P

[Denis Kenzior]

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

Hi Andrew,

On 9/8/21 4:44 PM, Andrew Zaborowski wrote:
> Add the IPv{4,6}Configuration D-Bus interfaces on the station and P2P
> objects that expose current netconfig values.  Access Point and P2P-GO
> are not handled yet, and some of the IP config properties may not
> automatically generate PropertiesChanged on DHCP lease renewals but
> their values are unlikely to change.
> ---
>   src/dbus.h      |   2 +
>   src/netconfig.c | 640 ++++++++++++++++++++++++++++++++++++++++--------
>   src/netconfig.h |   2 +-
>   src/p2p.c       |   4 +-
>   src/station.c   |   8 +-
>   5 files changed, 551 insertions(+), 105 deletions(-)
> 

<snip>

> @@ -152,20 +164,6 @@ static struct netconfig *netconfig_find(uint32_t ifindex)
>   	return NULL;
>   }
>   
> -#define APPEND_STRDUPV(dest, index, src)			\
> -	do {							\
> -		char **p;					\
> -		for (p = src; p && *p; p++)			\
> -			dest[index++] = l_strdup(*p);		\
> -	} while (0)						\
> -
> -#define APPENDV(dest, index, src)				\
> -	do {							\
> -		char **p;					\
> -		for (p = src; p && *p; p++)			\
> -			dest[index++] = *p;			\
> -	} while (0)						\
> -
>   static inline char *netconfig_ipv4_to_string(uint32_t addr)
>   {
>   	struct in_addr in_addr = { .s_addr = addr };
> @@ -196,77 +194,108 @@ static inline char *netconfig_ipv6_to_string(const uint8_t *addr)
>   	return addr_str;
>   }
>   
> -static void netconfig_set_neighbor_entry_cb(int error,
> -						uint16_t type, const void *data,
> -						uint32_t len, void *user_data)
> -{
> -	if (error)
> -		l_error("l_rtnl_neighbor_set_hwaddr failed: %s (%i)",
> -			strerror(-error), error);
> -}
> -
> -static int netconfig_set_dns(struct netconfig *netconfig)
> +static char **netconfig_get_dns_list(struct netconfig *netconfig, int af,
> +					const uint8_t **out_dns_mac)
>   {
> -	char **dns6_list = NULL;
> -	char **dns4_list = NULL;
> -	unsigned int n_entries = 0;
> -	char **dns_list;
> -	const uint8_t *fils_dns4_mac = NULL;
> -	const uint8_t *fils_dns6_mac = NULL;
>   	const struct ie_fils_ip_addr_response_info *fils =
>   		netconfig->fils_override;
>   
> -	if (!netconfig->dns4_overrides &&
> -			netconfig->rtm_protocol == RTPROT_DHCP) {
> +	if (af == AF_INET) {
>   		const struct l_dhcp_lease *lease;
>   
> +		if (netconfig->dns4_overrides)
> +			return l_strv_copy(netconfig->dns4_overrides);
> +
> +		if (netconfig->rtm_protocol != RTPROT_DHCP)
> +			return NULL;
> +
>   		if (fils && fils->ipv4_dns) {
> -			dns4_list = l_new(char *, 2);
> -			dns4_list[0] = netconfig_ipv4_to_string(fils->ipv4_dns);
> +			char **dns_list = l_new(char *, 2);
>   
>   			if (!l_memeqzero(fils->ipv4_dns_mac, 6) &&
> +					out_dns_mac &&
>   					util_ip_subnet_match(
>   							fils->ipv4_prefix_len,
>   							&fils->ipv4_addr,
>   							&fils->ipv4_dns))
> -				fils_dns4_mac = fils->ipv4_dns_mac;
> -		} else if ((lease = l_dhcp_client_get_lease(
> -						netconfig->dhcp_client)))
> -			dns4_list = l_dhcp_lease_get_dns(lease);
> -	}
> +				*out_dns_mac = fils->ipv4_dns_mac;
> +
> +			dns_list[0] = netconfig_ipv4_to_string(fils->ipv4_dns);
> +			return dns_list;
> +		}
>   
> -	if (!netconfig->dns6_overrides &&
> -			netconfig->rtm_v6_protocol == RTPROT_DHCP) {
> +		if (!(lease = l_dhcp_client_get_lease(netconfig->dhcp_client)))
> +			return NULL;
> +
> +		return l_dhcp_lease_get_dns(lease);
> +	} else {
>   		const struct l_dhcp6_lease *lease;
>   
> -		if (fils && !l_memeqzero(fils->ipv6_dns,
> -						16)) {
> -			dns6_list = l_new(char *, 2);
> -			dns6_list[0] = netconfig_ipv6_to_string(fils->ipv6_dns);
> +		if (netconfig->dns6_overrides)
> +			return l_strv_copy(netconfig->dns6_overrides);
> +
> +		if (netconfig->rtm_v6_protocol != RTPROT_DHCP)
> +			return NULL;
> +
> +		if (fils && !l_memeqzero(fils->ipv6_dns, 16)) {
> +			char **dns_list = l_new(char *, 2);
>   
>   			if (!l_memeqzero(fils->ipv6_dns_mac, 6) &&
> +					out_dns_mac &&
>   					util_ip_subnet_match(
>   							fils->ipv6_prefix_len,
>   							fils->ipv6_addr,
>   							fils->ipv6_dns))
> -				fils_dns6_mac = fils->ipv6_dns_mac;
> -		} else if ((lease = l_dhcp6_client_get_lease(
> +				*out_dns_mac = fils->ipv6_dns_mac;
> +
> +			dns_list[0] = netconfig_ipv6_to_string(fils->ipv6_dns);
> +			return dns_list;
> +		}
> +
> +		if (!(lease = l_dhcp6_client_get_lease(
>   						netconfig->dhcp6_client)))
> -			dns6_list = l_dhcp6_lease_get_dns(lease);
> +			return NULL;
> +
> +		return l_dhcp6_lease_get_dns(lease);
>   	}
> +}
> +
> +static void netconfig_set_neighbor_entry_cb(int error,
> +						uint16_t type, const void *data,
> +						uint32_t len, void *user_data)
> +{
> +	if (error)
> +		l_error("l_rtnl_neighbor_set_hwaddr failed: %s (%i)",
> +			strerror(-error), error);
> +}
> +
> +static int netconfig_set_dns(struct netconfig *netconfig)
> +{
> +	const uint8_t *fils_dns4_mac = NULL;
> +	const uint8_t *fils_dns6_mac = NULL;
> +	char **dns4_list = netconfig_get_dns_list(netconfig, AF_INET,
> +							&fils_dns4_mac);
> +	char **dns6_list = netconfig_get_dns_list(netconfig, AF_INET6,
> +							&fils_dns6_mac);
> +	unsigned int n_entries4 = l_strv_length(dns4_list);
> +	unsigned int n_entries6 = l_strv_length(dns6_list);
> +	char **dns_list;
> +	const struct ie_fils_ip_addr_response_info *fils =
> +		netconfig->fils_override;
>   
> -	n_entries += l_strv_length(netconfig->dns4_overrides);
> -	n_entries += l_strv_length(netconfig->dns6_overrides);
> -	n_entries += l_strv_length(dns4_list);
> -	n_entries += l_strv_length(dns6_list);
> +	if (!dns4_list && !dns6_list)
> +		return 0;
>   
> -	dns_list = l_new(char *, n_entries + 1);
> -	n_entries = 0;
> +	dns_list = l_realloc(dns4_list,
> +				sizeof(char *) * (n_entries4 + n_entries6 + 1));
> +
> +	if (dns6_list) {
> +		memcpy(dns_list + n_entries4, dns6_list,
> +			sizeof(char *) * (n_entries6 + 1));
> +		/* Contents now belong to dns_list, so no l_strfreev */
> +		l_free(dns6_list);
> +	}
>   
> -	APPEND_STRDUPV(dns_list, n_entries, netconfig->dns4_overrides);
> -	APPENDV(dns_list, n_entries, dns4_list);
> -	APPEND_STRDUPV(dns_list, n_entries, netconfig->dns6_overrides);
> -	APPENDV(dns_list, n_entries, dns6_list);
>   	resolve_set_dns(netconfig->resolve, dns_list);
>   
>   	if (fils_dns4_mac && !l_rtnl_neighbor_set_hwaddr(rtnl,
> @@ -284,9 +313,6 @@ static int netconfig_set_dns(struct netconfig *netconfig)
>   		l_debug("l_rtnl_neighbor_set_hwaddr failed");
>   
>   	l_strv_free(dns_list);
> -	/* Contents belonged to dns_list, so not l_strfreev */
> -	l_free(dns4_list);
> -	l_free(dns6_list);
>   	return 0;
>   }
>   

I applied this part separately in commit 8b573fe3984d ("netconfig: Refactor 
netconfig_set_dns")

<snip>

> @@ -703,6 +732,43 @@ static void netconfig_route_generic_cb(int error, uint16_t type,
>   	}
>   }
>   
> +static void netconfig_connected(struct netconfig *netconfig)
> +{
> +	struct l_dbus *dbus = dbus_get_bus();
> +
> +	netconfig->connected = true;
> +
> +	if (netconfig->notify) {
> +		netconfig->notify(NETCONFIG_EVENT_CONNECTED,
> +					netconfig->user_data);
> +		netconfig->notify = NULL;
> +	}
> +
> +	if (netconfig->dbus_path &&
> +			unlikely(!l_dbus_object_add_interface(dbus,
> +						netconfig->dbus_path,
> +						IWD_IPV4_CONFIG_INTERFACE,
> +						&netconfig->ipv4_data)))
> +		l_info("Unable to add %s at %s",
> +			IWD_IPV4_CONFIG_INTERFACE, netconfig->dbus_path);
> +
> +	if (netconfig->dbus_path &&
> +			unlikely(!l_dbus_object_add_interface(dbus,
> +						netconfig->dbus_path,
> +						IWD_IPV6_CONFIG_INTERFACE,
> +						&netconfig->ipv6_data)))
> +		l_info("Unable to add %s at %s",
> +			IWD_IPV6_CONFIG_INTERFACE, netconfig->dbus_path);
> +
> +	if (netconfig->dbus_path &&
> +			!l_dbus_object_add_interface(dbus, netconfig->dbus_path,
> +						L_DBUS_INTERFACE_PROPERTIES,
> +						netconfig))
> +		/* Properties may already exist on the object, not an error */
> +		l_debug("Unable to add %s at %s",
> +			L_DBUS_INTERFACE_PROPERTIES, netconfig->dbus_path);

So the connected event is fired once we have IPv4 or IPv6 configuration, right?

Why would we populate both IPv6 and IPv4 interfaces here though?  Why don't you 
just make IPv6 interface available when IPv6 is configured, and similarly for 
IPv4?  Would make a lot of the code simpler and require less 
l_dbus_property_changed() calls.

> +}
> +
>   static void netconfig_route_add_cmd_cb(int error, uint16_t type,
>   						const void *data, uint32_t len,
>   						void *user_data)

<snip>

> @@ -897,10 +989,47 @@ static void netconfig_ipv4_dhcp_event_handler(struct l_dhcp_client *client,
>   					netconfig->v4_address,
>   					netconfig_ifaddr_del_cmd_cb,
>   					netconfig, NULL));
> -		l_rtnl_address_free(netconfig->v4_address);
>   		/* Fall through. */
>   	case L_DHCP_CLIENT_EVENT_LEASE_OBTAINED:
> -		netconfig->v4_address = netconfig_get_dhcp4_address(netconfig);
> +	{
> +		char *gateway_str;
> +		struct l_rtnl_address *address;
> +
> +		gateway_str = netconfig_ipv4_get_gateway(netconfig, NULL);
> +		if (l_streq0(netconfig->v4_gateway_str, gateway_str))
> +			l_free(gateway_str);
> +		else {
> +			l_free(netconfig->v4_gateway_str);
> +			netconfig->v4_gateway_str = gateway_str;
> +
> +			if (netconfig->dbus_path && netconfig->connected)
> +				l_dbus_property_changed(dbus,
> +						netconfig->dbus_path,
> +						IWD_IPV4_CONFIG_INTERFACE,
> +						"Gateway");
> +		}

So in theory LEASE_OBTAINED should only ever be signaled once.  Why not register 
the IPv4 interface now and avoid all this logic?  If we lose the lease for some 
reason, then we should get a LEASE_LOST or LEASE_EXPIRED event first.

> +
> +		address = netconfig_get_dhcp4_address(netconfig);
> +
> +		if (netconfig->dbus_path && netconfig->connected &&
> +				!netconfig_address_cmp_prefix_len(
> +							netconfig->v4_address,
> +							address))
> +			l_dbus_property_changed(dbus, netconfig->dbus_path,
> +						IWD_IPV4_CONFIG_INTERFACE,
> +						"Netmask");
> +
> +		if (netconfig->dbus_path && netconfig->connected &&
> +				netconfig_address_cmp_address(
> +							netconfig->v4_address,
> +							address))
> +			l_dbus_property_changed(dbus, netconfig->dbus_path,
> +						IWD_IPV4_CONFIG_INTERFACE,
> +						"Address");
> +
> +		l_rtnl_address_free(netconfig->v4_address);
> +		netconfig->v4_address = address;
> +
>   		if (!netconfig->v4_address) {
>   			l_error("netconfig: Failed to obtain IP addresses from "
>   							"DHCPv4 lease.");
> @@ -913,6 +1042,7 @@ static void netconfig_ipv4_dhcp_event_handler(struct l_dhcp_client *client,
>   					netconfig_ipv4_ifaddr_add_cmd_cb,
>   					netconfig, NULL)));
>   		break;
> +	}
>   	case L_DHCP_CLIENT_EVENT_LEASE_RENEWED:
>   		break;
>   	case L_DHCP_CLIENT_EVENT_LEASE_EXPIRED:
> @@ -923,6 +1053,25 @@ static void netconfig_ipv4_dhcp_event_handler(struct l_dhcp_client *client,
>   		l_rtnl_address_free(netconfig->v4_address);
>   		netconfig->v4_address = NULL;
>   
> +		if (netconfig->dbus_path && netconfig->connected) {
> +			l_dbus_property_changed(dbus, netconfig->dbus_path,
> +						IWD_IPV4_CONFIG_INTERFACE,
> +						"Address");
> +			l_dbus_property_changed(dbus, netconfig->dbus_path,
> +						IWD_IPV4_CONFIG_INTERFACE,
> +						"Netmask");
> +		}
> +
> +		if (netconfig->v4_gateway_str) {
> +			if (netconfig->dbus_path && netconfig->connected)
> +				l_dbus_property_changed(dbus,
> +						netconfig->dbus_path,
> +						IWD_IPV4_CONFIG_INTERFACE,
> +						"Gateway");
> +
> +			l_free(l_steal_ptr(netconfig->v4_gateway_str));
> +		}

Why not simply remove the IPv4 interface?

> +
>   		/* Fall through. */
>   	case L_DHCP_CLIENT_EVENT_NO_LEASE:
>   		/*
> @@ -945,18 +1094,87 @@ static void netconfig_dhcp6_event_handler(struct l_dhcp6_client *client,
>   						void *userdata)
>   {
>   	struct netconfig *netconfig = userdata;
> +	struct l_dbus *dbus = dbus_get_bus();
>   
>   	switch (event) {
>   	case L_DHCP6_CLIENT_EVENT_IP_CHANGED:
>   	case L_DHCP6_CLIENT_EVENT_LEASE_OBTAINED:
>   	case L_DHCP6_CLIENT_EVENT_LEASE_RENEWED:
> +	{
> +		__auto_type lease =
> +			l_dhcp6_client_get_lease(netconfig->dhcp6_client);
> +		L_AUTO_FREE_VAR(char *, addr_str) =
> +			l_dhcp6_lease_get_address(lease);
> +		struct l_rtnl_address *address;
> +		__auto_type icmp6 =
> +			l_dhcp6_client_get_icmp6(netconfig->dhcp6_client);
> +		__auto_type router = l_icmp6_client_get_router(icmp6);
> +		char *gateway_str = l_icmp6_router_get_address(router);
> +
> +		if (l_streq0(netconfig->v6_gateway_str, gateway_str))
> +			l_free(gateway_str);
> +		else {
> +			l_free(netconfig->v6_gateway_str);
> +			netconfig->v6_gateway_str = gateway_str;
> +
> +			if (netconfig->dbus_path && netconfig->connected)
> +				l_dbus_property_changed(dbus,
> +						netconfig->dbus_path,
> +						IWD_IPV6_CONFIG_INTERFACE,
> +						"Gateway");
> +		}
> +
> +		address = l_rtnl_address_new(addr_str,
> +					l_dhcp6_lease_get_prefix_length(lease));
> +
> +		if (netconfig->dbus_path && netconfig->connected &&
> +				!netconfig_address_cmp_prefix_len(
> +							netconfig->v6_address,
> +							address))
> +			l_dbus_property_changed(dbus, netconfig->dbus_path,
> +						IWD_IPV6_CONFIG_INTERFACE,
> +						"Netmask");
> +
> +		if (netconfig->dbus_path && netconfig->connected &&
> +				netconfig_address_cmp_address(
> +							netconfig->v6_address,
> +							address))
> +			l_dbus_property_changed(dbus, netconfig->dbus_path,
> +						IWD_IPV6_CONFIG_INTERFACE,
> +						"Address");
> +
> +		l_rtnl_address_free(netconfig->v6_address);
> +		netconfig->v4_address = address;

v6 address?

Also similar comments as above?  Why not just register the IPv6 interface here 
instead?

Also, I'm really not sure whether just Netmask/Address/Gateway are the only 
things that can change.  DNS, NTP, domain, etc information might also change. 
So a more generic old lease / new lease comparison might be needed.  Or always 
send a PropertyChanged for all attributes and have the caller figure it out?

<snip>

> +static bool netconfig_property_get_domains(struct l_dbus *dbus,
> +					struct l_dbus_message *message,
> +					struct l_dbus_message_builder *builder,
> +					void *user_data)
> +{
> +	const struct interface_data *data = user_data;
> +
> +	if (data->is_ipv4) {
> +		L_AUTO_FREE_VAR(char *, domain) =
> +			l_settings_get_string(data->netconfig->active_settings,
> +						"IPv4", "DomainName");
> +
> +		if (!domain) {
> +			const struct l_dhcp_lease *lease;
> +
> +			if (data->netconfig->rtm_protocol != RTPROT_DHCP)
> +				return false;
> +
> +			lease = l_dhcp_client_get_lease(
> +						data->netconfig->dhcp_client);
> +			if (!lease)
> +				return false;
> +
> +			if (!(domain = l_dhcp_lease_get_domain_name(lease)))
> +				return false;
> +		}
> +
> +		l_dbus_message_builder_enter_array(builder, "s");
> +		l_dbus_message_builder_append_basic(builder, 's', domain);
> +		l_dbus_message_builder_leave_array(builder);
> +	} else {
> +		const struct l_dhcp6_lease *lease;
> +		char **domains;
> +		char **i;
> +
> +		if (data->netconfig->rtm_v6_protocol != RTPROT_DHCP)
> +			return false;
> +
> +		lease = l_dhcp6_client_get_lease(data->netconfig->dhcp6_client);
> +		if (!lease)
> +			return false;
> +
> +		domains = l_dhcp6_lease_get_domains(lease);
> +		if (!domains)
> +			return false;
> +
> +		l_dbus_message_builder_enter_array(builder, "s");
> +
> +		for (i = domains; *i; i++)
> +			l_dbus_message_builder_append_basic(builder, 's', *i);
> +
> +		l_dbus_message_builder_leave_array(builder);
> +		l_strv_free(domains);
> +	}
> +
> +	return true;

Might as well make this two separate functions

> +}
> +

Regards,
-Denis

Re: [PATCH 2/2] netconfig: Add IP configuration properties on Station and P2P

[Andrew Zaborowski]

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

Hi Denis,

On Wed, 15 Sept 2021 at 00:08, Denis Kenzior <denkenz@gmail.com> wrote:
> On 9/8/21 4:44 PM, Andrew Zaborowski wrote:
> > @@ -703,6 +732,43 @@ static void netconfig_route_generic_cb(int error, uint16_t type,
> >       }
> >   }
> >
> > +static void netconfig_connected(struct netconfig *netconfig)
> > +{
> > +     struct l_dbus *dbus = dbus_get_bus();
> > +
> > +     netconfig->connected = true;
> > +
> > +     if (netconfig->notify) {
> > +             netconfig->notify(NETCONFIG_EVENT_CONNECTED,
> > +                                     netconfig->user_data);
> > +             netconfig->notify = NULL;
> > +     }
> > +
> > +     if (netconfig->dbus_path &&
> > +                     unlikely(!l_dbus_object_add_interface(dbus,
> > +                                             netconfig->dbus_path,
> > +                                             IWD_IPV4_CONFIG_INTERFACE,
> > +                                             &netconfig->ipv4_data)))
> > +             l_info("Unable to add %s at %s",
> > +                     IWD_IPV4_CONFIG_INTERFACE, netconfig->dbus_path);
> > +
> > +     if (netconfig->dbus_path &&
> > +                     unlikely(!l_dbus_object_add_interface(dbus,
> > +                                             netconfig->dbus_path,
> > +                                             IWD_IPV6_CONFIG_INTERFACE,
> > +                                             &netconfig->ipv6_data)))
> > +             l_info("Unable to add %s at %s",
> > +                     IWD_IPV6_CONFIG_INTERFACE, netconfig->dbus_path);
> > +
> > +     if (netconfig->dbus_path &&
> > +                     !l_dbus_object_add_interface(dbus, netconfig->dbus_path,
> > +                                             L_DBUS_INTERFACE_PROPERTIES,
> > +                                             netconfig))
> > +             /* Properties may already exist on the object, not an error */
> > +             l_debug("Unable to add %s at %s",
> > +                     L_DBUS_INTERFACE_PROPERTIES, netconfig->dbus_path);
>
> So the connected event is fired once we have IPv4 or IPv6 configuration, right?
>
> Why would we populate both IPv6 and IPv4 interfaces here though?  Why don't you
> just make IPv6 interface available when IPv6 is configured, and similarly for
> IPv4?  Would make a lot of the code simpler and require less
> l_dbus_property_changed() calls.

Yep, I was considering that, the semantics of the two interfaces are
going to be a little different but that should be fine for NM as the
potential user of the API.

I added a netconfig.connected, I'll need to split this into a
.connected4 and .connected6 or similar, to track whether we've added
the interfaces yet.

>
> > +}
> > +
> >   static void netconfig_route_add_cmd_cb(int error, uint16_t type,
> >                                               const void *data, uint32_t len,
> >                                               void *user_data)
>
> <snip>
>
> > @@ -897,10 +989,47 @@ static void netconfig_ipv4_dhcp_event_handler(struct l_dhcp_client *client,
> >                                       netconfig->v4_address,
> >                                       netconfig_ifaddr_del_cmd_cb,
> >                                       netconfig, NULL));
> > -             l_rtnl_address_free(netconfig->v4_address);
> >               /* Fall through. */
> >       case L_DHCP_CLIENT_EVENT_LEASE_OBTAINED:
> > -             netconfig->v4_address = netconfig_get_dhcp4_address(netconfig);
> > +     {
> > +             char *gateway_str;
> > +             struct l_rtnl_address *address;
> > +
> > +             gateway_str = netconfig_ipv4_get_gateway(netconfig, NULL);
> > +             if (l_streq0(netconfig->v4_gateway_str, gateway_str))
> > +                     l_free(gateway_str);
> > +             else {
> > +                     l_free(netconfig->v4_gateway_str);
> > +                     netconfig->v4_gateway_str = gateway_str;
> > +
> > +                     if (netconfig->dbus_path && netconfig->connected)
> > +                             l_dbus_property_changed(dbus,
> > +                                             netconfig->dbus_path,
> > +                                             IWD_IPV4_CONFIG_INTERFACE,
> > +                                             "Gateway");
> > +             }
>
> So in theory LEASE_OBTAINED should only ever be signaled once.  Why not register
> the IPv4 interface now and avoid all this logic?  If we lose the lease for some
> reason, then we should get a LEASE_LOST or LEASE_EXPIRED event first.

Yep, that'd be in line with making the interfaces be present only
during the time that we have a lease (if method is DHCP.)

>
> > +
> > +             address = netconfig_get_dhcp4_address(netconfig);
> > +
> > +             if (netconfig->dbus_path && netconfig->connected &&
> > +                             !netconfig_address_cmp_prefix_len(
> > +                                                     netconfig->v4_address,
> > +                                                     address))
> > +                     l_dbus_property_changed(dbus, netconfig->dbus_path,
> > +                                             IWD_IPV4_CONFIG_INTERFACE,
> > +                                             "Netmask");
> > +
> > +             if (netconfig->dbus_path && netconfig->connected &&
> > +                             netconfig_address_cmp_address(
> > +                                                     netconfig->v4_address,
> > +                                                     address))
> > +                     l_dbus_property_changed(dbus, netconfig->dbus_path,
> > +                                             IWD_IPV4_CONFIG_INTERFACE,
> > +                                             "Address");
> > +
> > +             l_rtnl_address_free(netconfig->v4_address);
> > +             netconfig->v4_address = address;
> > +
> >               if (!netconfig->v4_address) {
> >                       l_error("netconfig: Failed to obtain IP addresses from "
> >                                                       "DHCPv4 lease.");
> > @@ -913,6 +1042,7 @@ static void netconfig_ipv4_dhcp_event_handler(struct l_dhcp_client *client,
> >                                       netconfig_ipv4_ifaddr_add_cmd_cb,
> >                                       netconfig, NULL)));
> >               break;
> > +     }
> >       case L_DHCP_CLIENT_EVENT_LEASE_RENEWED:
> >               break;
> >       case L_DHCP_CLIENT_EVENT_LEASE_EXPIRED:
> > @@ -923,6 +1053,25 @@ static void netconfig_ipv4_dhcp_event_handler(struct l_dhcp_client *client,
> >               l_rtnl_address_free(netconfig->v4_address);
> >               netconfig->v4_address = NULL;
> >
> > +             if (netconfig->dbus_path && netconfig->connected) {
> > +                     l_dbus_property_changed(dbus, netconfig->dbus_path,
> > +                                             IWD_IPV4_CONFIG_INTERFACE,
> > +                                             "Address");
> > +                     l_dbus_property_changed(dbus, netconfig->dbus_path,
> > +                                             IWD_IPV4_CONFIG_INTERFACE,
> > +                                             "Netmask");
> > +             }
> > +
> > +             if (netconfig->v4_gateway_str) {
> > +                     if (netconfig->dbus_path && netconfig->connected)
> > +                             l_dbus_property_changed(dbus,
> > +                                             netconfig->dbus_path,
> > +                                             IWD_IPV4_CONFIG_INTERFACE,
> > +                                             "Gateway");
> > +
> > +                     l_free(l_steal_ptr(netconfig->v4_gateway_str));
> > +             }
>
> Why not simply remove the IPv4 interface?

Yep, same here.

>
> > +
> >               /* Fall through. */
> >       case L_DHCP_CLIENT_EVENT_NO_LEASE:
> >               /*
> > @@ -945,18 +1094,87 @@ static void netconfig_dhcp6_event_handler(struct l_dhcp6_client *client,
> >                                               void *userdata)
> >   {
> >       struct netconfig *netconfig = userdata;
> > +     struct l_dbus *dbus = dbus_get_bus();
> >
> >       switch (event) {
> >       case L_DHCP6_CLIENT_EVENT_IP_CHANGED:
> >       case L_DHCP6_CLIENT_EVENT_LEASE_OBTAINED:
> >       case L_DHCP6_CLIENT_EVENT_LEASE_RENEWED:
> > +     {
> > +             __auto_type lease =
> > +                     l_dhcp6_client_get_lease(netconfig->dhcp6_client);
> > +             L_AUTO_FREE_VAR(char *, addr_str) =
> > +                     l_dhcp6_lease_get_address(lease);
> > +             struct l_rtnl_address *address;
> > +             __auto_type icmp6 =
> > +                     l_dhcp6_client_get_icmp6(netconfig->dhcp6_client);
> > +             __auto_type router = l_icmp6_client_get_router(icmp6);
> > +             char *gateway_str = l_icmp6_router_get_address(router);
> > +
> > +             if (l_streq0(netconfig->v6_gateway_str, gateway_str))
> > +                     l_free(gateway_str);
> > +             else {
> > +                     l_free(netconfig->v6_gateway_str);
> > +                     netconfig->v6_gateway_str = gateway_str;
> > +
> > +                     if (netconfig->dbus_path && netconfig->connected)
> > +                             l_dbus_property_changed(dbus,
> > +                                             netconfig->dbus_path,
> > +                                             IWD_IPV6_CONFIG_INTERFACE,
> > +                                             "Gateway");
> > +             }
> > +
> > +             address = l_rtnl_address_new(addr_str,
> > +                                     l_dhcp6_lease_get_prefix_length(lease));
> > +
> > +             if (netconfig->dbus_path && netconfig->connected &&
> > +                             !netconfig_address_cmp_prefix_len(
> > +                                                     netconfig->v6_address,
> > +                                                     address))
> > +                     l_dbus_property_changed(dbus, netconfig->dbus_path,
> > +                                             IWD_IPV6_CONFIG_INTERFACE,
> > +                                             "Netmask");
> > +
> > +             if (netconfig->dbus_path && netconfig->connected &&
> > +                             netconfig_address_cmp_address(
> > +                                                     netconfig->v6_address,
> > +                                                     address))
> > +                     l_dbus_property_changed(dbus, netconfig->dbus_path,
> > +                                             IWD_IPV6_CONFIG_INTERFACE,
> > +                                             "Address");
> > +
> > +             l_rtnl_address_free(netconfig->v6_address);
> > +             netconfig->v4_address = address;
>
> v6 address?

Oops, yes.

>
> Also similar comments as above?  Why not just register the IPv6 interface here
> instead?
>
> Also, I'm really not sure whether just Netmask/Address/Gateway are the only
> things that can change.  DNS, NTP, domain, etc information might also change.
> So a more generic old lease / new lease comparison might be needed.  Or always
> send a PropertyChanged for all attributes and have the caller figure it out?

I mentioned in the commit that the signals don't cover all of the
properties since those are unlikely to be useful and are unlikely to
change, in fact ell/dhcp.c also only checks the netmask, the address
and the router when emitting the event.  I'll add the missing signals
just in case.

>
> <snip>
>
> > +static bool netconfig_property_get_domains(struct l_dbus *dbus,
> > +                                     struct l_dbus_message *message,
> > +                                     struct l_dbus_message_builder *builder,
> > +                                     void *user_data)
> > +{
> > +     const struct interface_data *data = user_data;
> > +
> > +     if (data->is_ipv4) {
> > +             L_AUTO_FREE_VAR(char *, domain) =
> > +                     l_settings_get_string(data->netconfig->active_settings,
> > +                                             "IPv4", "DomainName");
> > +
> > +             if (!domain) {
> > +                     const struct l_dhcp_lease *lease;
> > +
> > +                     if (data->netconfig->rtm_protocol != RTPROT_DHCP)
> > +                             return false;
> > +
> > +                     lease = l_dhcp_client_get_lease(
> > +                                             data->netconfig->dhcp_client);
> > +                     if (!lease)
> > +                             return false;
> > +
> > +                     if (!(domain = l_dhcp_lease_get_domain_name(lease)))
> > +                             return false;
> > +             }
> > +
> > +             l_dbus_message_builder_enter_array(builder, "s");
> > +             l_dbus_message_builder_append_basic(builder, 's', domain);
> > +             l_dbus_message_builder_leave_array(builder);
> > +     } else {
> > +             const struct l_dhcp6_lease *lease;
> > +             char **domains;
> > +             char **i;
> > +
> > +             if (data->netconfig->rtm_v6_protocol != RTPROT_DHCP)
> > +                     return false;
> > +
> > +             lease = l_dhcp6_client_get_lease(data->netconfig->dhcp6_client);
> > +             if (!lease)
> > +                     return false;
> > +
> > +             domains = l_dhcp6_lease_get_domains(lease);
> > +             if (!domains)
> > +                     return false;
> > +
> > +             l_dbus_message_builder_enter_array(builder, "s");
> > +
> > +             for (i = domains; *i; i++)
> > +                     l_dbus_message_builder_append_basic(builder, 's', *i);
> > +
> > +             l_dbus_message_builder_leave_array(builder);
> > +             l_strv_free(domains);
> > +     }
> > +
> > +     return true;
>
> Might as well make this two separate functions

Ok.

Best regards

Re: [PATCH 2/2] netconfig: Add IP configuration properties on Station and P2P

[Andrew Zaborowski]

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

Hi Denis,

On Wed, 15 Sept 2021 at 00:08, Denis Kenzior <denkenz(a)gmail.com> wrote:
> On 9/8/21 4:44 PM, Andrew Zaborowski wrote:
> > @@ -703,6 +732,43 @@ static void netconfig_route_generic_cb(int error, uint16_t type,
> >       }
> >   }
> >
> > +static void netconfig_connected(struct netconfig *netconfig)
> > +{
> > +     struct l_dbus *dbus = dbus_get_bus();
> > +
> > +     netconfig->connected = true;
> > +
> > +     if (netconfig->notify) {
> > +             netconfig->notify(NETCONFIG_EVENT_CONNECTED,
> > +                                     netconfig->user_data);
> > +             netconfig->notify = NULL;
> > +     }
> > +
> > +     if (netconfig->dbus_path &&
> > +                     unlikely(!l_dbus_object_add_interface(dbus,
> > +                                             netconfig->dbus_path,
> > +                                             IWD_IPV4_CONFIG_INTERFACE,
> > +                                             &netconfig->ipv4_data)))
> > +             l_info("Unable to add %s at %s",
> > +                     IWD_IPV4_CONFIG_INTERFACE, netconfig->dbus_path);
> > +
> > +     if (netconfig->dbus_path &&
> > +                     unlikely(!l_dbus_object_add_interface(dbus,
> > +                                             netconfig->dbus_path,
> > +                                             IWD_IPV6_CONFIG_INTERFACE,
> > +                                             &netconfig->ipv6_data)))
> > +             l_info("Unable to add %s at %s",
> > +                     IWD_IPV6_CONFIG_INTERFACE, netconfig->dbus_path);
> > +
> > +     if (netconfig->dbus_path &&
> > +                     !l_dbus_object_add_interface(dbus, netconfig->dbus_path,
> > +                                             L_DBUS_INTERFACE_PROPERTIES,
> > +                                             netconfig))
> > +             /* Properties may already exist on the object, not an error */
> > +             l_debug("Unable to add %s at %s",
> > +                     L_DBUS_INTERFACE_PROPERTIES, netconfig->dbus_path);
>
> So the connected event is fired once we have IPv4 or IPv6 configuration, right?
>
> Why would we populate both IPv6 and IPv4 interfaces here though?  Why don't you
> just make IPv6 interface available when IPv6 is configured, and similarly for
> IPv4?  Would make a lot of the code simpler and require less
> l_dbus_property_changed() calls.

Yep, I was considering that, the semantics of the two interfaces are
going to be a little different but that should be fine for NM as the
potential user of the API.

I added a netconfig.connected, I'll need to split this into a
.connected4 and .connected6 or similar, to track whether we've added
the interfaces yet.

>
> > +}
> > +
> >   static void netconfig_route_add_cmd_cb(int error, uint16_t type,
> >                                               const void *data, uint32_t len,
> >                                               void *user_data)
>
> <snip>
>
> > @@ -897,10 +989,47 @@ static void netconfig_ipv4_dhcp_event_handler(struct l_dhcp_client *client,
> >                                       netconfig->v4_address,
> >                                       netconfig_ifaddr_del_cmd_cb,
> >                                       netconfig, NULL));
> > -             l_rtnl_address_free(netconfig->v4_address);
> >               /* Fall through. */
> >       case L_DHCP_CLIENT_EVENT_LEASE_OBTAINED:
> > -             netconfig->v4_address = netconfig_get_dhcp4_address(netconfig);
> > +     {
> > +             char *gateway_str;
> > +             struct l_rtnl_address *address;
> > +
> > +             gateway_str = netconfig_ipv4_get_gateway(netconfig, NULL);
> > +             if (l_streq0(netconfig->v4_gateway_str, gateway_str))
> > +                     l_free(gateway_str);
> > +             else {
> > +                     l_free(netconfig->v4_gateway_str);
> > +                     netconfig->v4_gateway_str = gateway_str;
> > +
> > +                     if (netconfig->dbus_path && netconfig->connected)
> > +                             l_dbus_property_changed(dbus,
> > +                                             netconfig->dbus_path,
> > +                                             IWD_IPV4_CONFIG_INTERFACE,
> > +                                             "Gateway");
> > +             }
>
> So in theory LEASE_OBTAINED should only ever be signaled once.  Why not register
> the IPv4 interface now and avoid all this logic?  If we lose the lease for some
> reason, then we should get a LEASE_LOST or LEASE_EXPIRED event first.

Yep, that'd be in line with making the interfaces be present only
during the time that we have a lease (if method is DHCP.)

>
> > +
> > +             address = netconfig_get_dhcp4_address(netconfig);
> > +
> > +             if (netconfig->dbus_path && netconfig->connected &&
> > +                             !netconfig_address_cmp_prefix_len(
> > +                                                     netconfig->v4_address,
> > +                                                     address))
> > +                     l_dbus_property_changed(dbus, netconfig->dbus_path,
> > +                                             IWD_IPV4_CONFIG_INTERFACE,
> > +                                             "Netmask");
> > +
> > +             if (netconfig->dbus_path && netconfig->connected &&
> > +                             netconfig_address_cmp_address(
> > +                                                     netconfig->v4_address,
> > +                                                     address))
> > +                     l_dbus_property_changed(dbus, netconfig->dbus_path,
> > +                                             IWD_IPV4_CONFIG_INTERFACE,
> > +                                             "Address");
> > +
> > +             l_rtnl_address_free(netconfig->v4_address);
> > +             netconfig->v4_address = address;
> > +
> >               if (!netconfig->v4_address) {
> >                       l_error("netconfig: Failed to obtain IP addresses from "
> >                                                       "DHCPv4 lease.");
> > @@ -913,6 +1042,7 @@ static void netconfig_ipv4_dhcp_event_handler(struct l_dhcp_client *client,
> >                                       netconfig_ipv4_ifaddr_add_cmd_cb,
> >                                       netconfig, NULL)));
> >               break;
> > +     }
> >       case L_DHCP_CLIENT_EVENT_LEASE_RENEWED:
> >               break;
> >       case L_DHCP_CLIENT_EVENT_LEASE_EXPIRED:
> > @@ -923,6 +1053,25 @@ static void netconfig_ipv4_dhcp_event_handler(struct l_dhcp_client *client,
> >               l_rtnl_address_free(netconfig->v4_address);
> >               netconfig->v4_address = NULL;
> >
> > +             if (netconfig->dbus_path && netconfig->connected) {
> > +                     l_dbus_property_changed(dbus, netconfig->dbus_path,
> > +                                             IWD_IPV4_CONFIG_INTERFACE,
> > +                                             "Address");
> > +                     l_dbus_property_changed(dbus, netconfig->dbus_path,
> > +                                             IWD_IPV4_CONFIG_INTERFACE,
> > +                                             "Netmask");
> > +             }
> > +
> > +             if (netconfig->v4_gateway_str) {
> > +                     if (netconfig->dbus_path && netconfig->connected)
> > +                             l_dbus_property_changed(dbus,
> > +                                             netconfig->dbus_path,
> > +                                             IWD_IPV4_CONFIG_INTERFACE,
> > +                                             "Gateway");
> > +
> > +                     l_free(l_steal_ptr(netconfig->v4_gateway_str));
> > +             }
>
> Why not simply remove the IPv4 interface?

Yep, same here.

>
> > +
> >               /* Fall through. */
> >       case L_DHCP_CLIENT_EVENT_NO_LEASE:
> >               /*
> > @@ -945,18 +1094,87 @@ static void netconfig_dhcp6_event_handler(struct l_dhcp6_client *client,
> >                                               void *userdata)
> >   {
> >       struct netconfig *netconfig = userdata;
> > +     struct l_dbus *dbus = dbus_get_bus();
> >
> >       switch (event) {
> >       case L_DHCP6_CLIENT_EVENT_IP_CHANGED:
> >       case L_DHCP6_CLIENT_EVENT_LEASE_OBTAINED:
> >       case L_DHCP6_CLIENT_EVENT_LEASE_RENEWED:
> > +     {
> > +             __auto_type lease =
> > +                     l_dhcp6_client_get_lease(netconfig->dhcp6_client);
> > +             L_AUTO_FREE_VAR(char *, addr_str) =
> > +                     l_dhcp6_lease_get_address(lease);
> > +             struct l_rtnl_address *address;
> > +             __auto_type icmp6 =
> > +                     l_dhcp6_client_get_icmp6(netconfig->dhcp6_client);
> > +             __auto_type router = l_icmp6_client_get_router(icmp6);
> > +             char *gateway_str = l_icmp6_router_get_address(router);
> > +
> > +             if (l_streq0(netconfig->v6_gateway_str, gateway_str))
> > +                     l_free(gateway_str);
> > +             else {
> > +                     l_free(netconfig->v6_gateway_str);
> > +                     netconfig->v6_gateway_str = gateway_str;
> > +
> > +                     if (netconfig->dbus_path && netconfig->connected)
> > +                             l_dbus_property_changed(dbus,
> > +                                             netconfig->dbus_path,
> > +                                             IWD_IPV6_CONFIG_INTERFACE,
> > +                                             "Gateway");
> > +             }
> > +
> > +             address = l_rtnl_address_new(addr_str,
> > +                                     l_dhcp6_lease_get_prefix_length(lease));
> > +
> > +             if (netconfig->dbus_path && netconfig->connected &&
> > +                             !netconfig_address_cmp_prefix_len(
> > +                                                     netconfig->v6_address,
> > +                                                     address))
> > +                     l_dbus_property_changed(dbus, netconfig->dbus_path,
> > +                                             IWD_IPV6_CONFIG_INTERFACE,
> > +                                             "Netmask");
> > +
> > +             if (netconfig->dbus_path && netconfig->connected &&
> > +                             netconfig_address_cmp_address(
> > +                                                     netconfig->v6_address,
> > +                                                     address))
> > +                     l_dbus_property_changed(dbus, netconfig->dbus_path,
> > +                                             IWD_IPV6_CONFIG_INTERFACE,
> > +                                             "Address");
> > +
> > +             l_rtnl_address_free(netconfig->v6_address);
> > +             netconfig->v4_address = address;
>
> v6 address?

Oops, yes.

>
> Also similar comments as above?  Why not just register the IPv6 interface here
> instead?
>
> Also, I'm really not sure whether just Netmask/Address/Gateway are the only
> things that can change.  DNS, NTP, domain, etc information might also change.
> So a more generic old lease / new lease comparison might be needed.  Or always
> send a PropertyChanged for all attributes and have the caller figure it out?

I mentioned in the commit that the signals don't cover all of the
properties since those are unlikely to be useful and are unlikely to
change, in fact ell/dhcp.c also only checks the netmask, the address
and the router when emitting the event.  I'll add the missing signals
just in case.

>
> <snip>
>
> > +static bool netconfig_property_get_domains(struct l_dbus *dbus,
> > +                                     struct l_dbus_message *message,
> > +                                     struct l_dbus_message_builder *builder,
> > +                                     void *user_data)
> > +{
> > +     const struct interface_data *data = user_data;
> > +
> > +     if (data->is_ipv4) {
> > +             L_AUTO_FREE_VAR(char *, domain) =
> > +                     l_settings_get_string(data->netconfig->active_settings,
> > +                                             "IPv4", "DomainName");
> > +
> > +             if (!domain) {
> > +                     const struct l_dhcp_lease *lease;
> > +
> > +                     if (data->netconfig->rtm_protocol != RTPROT_DHCP)
> > +                             return false;
> > +
> > +                     lease = l_dhcp_client_get_lease(
> > +                                             data->netconfig->dhcp_client);
> > +                     if (!lease)
> > +                             return false;
> > +
> > +                     if (!(domain = l_dhcp_lease_get_domain_name(lease)))
> > +                             return false;
> > +             }
> > +
> > +             l_dbus_message_builder_enter_array(builder, "s");
> > +             l_dbus_message_builder_append_basic(builder, 's', domain);
> > +             l_dbus_message_builder_leave_array(builder);
> > +     } else {
> > +             const struct l_dhcp6_lease *lease;
> > +             char **domains;
> > +             char **i;
> > +
> > +             if (data->netconfig->rtm_v6_protocol != RTPROT_DHCP)
> > +                     return false;
> > +
> > +             lease = l_dhcp6_client_get_lease(data->netconfig->dhcp6_client);
> > +             if (!lease)
> > +                     return false;
> > +
> > +             domains = l_dhcp6_lease_get_domains(lease);
> > +             if (!domains)
> > +                     return false;
> > +
> > +             l_dbus_message_builder_enter_array(builder, "s");
> > +
> > +             for (i = domains; *i; i++)
> > +                     l_dbus_message_builder_append_basic(builder, 's', *i);
> > +
> > +             l_dbus_message_builder_leave_array(builder);
> > +             l_strv_free(domains);
> > +     }
> > +
> > +     return true;
>
> Might as well make this two separate functions

Ok.

Best regards

Re: [PATCH 1/2] doc: Add net.connman.iwd.IPv{4,6}Configuration API doc

[Denis Kenzior]

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

Hi Andrew,

On 9/8/21 4:44 PM, Andrew Zaborowski wrote:
> ---
>   doc/ip-configuration-api.txt | 47 ++++++++++++++++++++++++++++++++++++
>   1 file changed, 47 insertions(+)
>   create mode 100644 doc/ip-configuration-api.txt

Applied, thanks.

Regards,
-Denis

Re: [PATCH 1/2] doc: Add net.connman.iwd.IPv{4,6}Configuration API doc

[Denis Kenzior]

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

Hi Andrew,

On 9/8/21 4:44 PM, Andrew Zaborowski wrote:
> ---
>   doc/ip-configuration-api.txt | 47 ++++++++++++++++++++++++++++++++++++
>   1 file changed, 47 insertions(+)
>   create mode 100644 doc/ip-configuration-api.txt

Applied, thanks.

Regards,
-Denis