[PATCH 2/8] netconfig: Cache the IPv6 l_rtnl_address object

[PATCH 2/8] netconfig: Cache the IPv6 l_rtnl_address object

[Andrew Zaborowski]

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

For symmetry with IPv4 and to make it easier to track changes to the
IPv6 address and netmask in future commits, add a netconfig->v6_address
treated in the same way as the existing netconfig->v4_address.
---
 src/netconfig.c | 46 ++++++++++++++++++++++++++++++++--------------
 1 file changed, 32 insertions(+), 14 deletions(-)

diff --git a/src/netconfig.c b/src/netconfig.c
index da775480..493a5c13 100644
--- a/src/netconfig.c
+++ b/src/netconfig.c
@@ -58,6 +58,7 @@ 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;
@@ -983,13 +984,28 @@ static void netconfig_dhcp6_event_handler(struct l_dhcp6_client *client,
 	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;
+
+		address = l_rtnl_address_new(addr_str,
+					l_dhcp6_lease_get_prefix_length(lease));
+		l_rtnl_address_free(netconfig->v6_address);
+		netconfig->v6_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;
 
 		/* Fall through */
 	case L_DHCP6_CLIENT_EVENT_NO_LEASE:
@@ -1135,7 +1151,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");
@@ -1144,10 +1159,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;
@@ -1157,11 +1169,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,
@@ -1171,12 +1184,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;
 	}
 
@@ -1291,7 +1304,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");
@@ -1309,11 +1321,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);
 
@@ -1399,6 +1414,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;
 
-- 
2.30.2

Re: [PATCH 2/8] netconfig: Cache the IPv6 l_rtnl_address object

[Denis Kenzior]

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

Hi Andrew,

On 9/24/21 8:11 PM, Andrew Zaborowski wrote:
> For symmetry with IPv4 and to make it easier to track changes to the
> IPv6 address and netmask in future commits, add a netconfig->v6_address
> treated in the same way as the existing netconfig->v4_address.
> ---
>   src/netconfig.c | 46 ++++++++++++++++++++++++++++++++--------------
>   1 file changed, 32 insertions(+), 14 deletions(-)
> 
> diff --git a/src/netconfig.c b/src/netconfig.c
> index da775480..493a5c13 100644
> --- a/src/netconfig.c
> +++ b/src/netconfig.c
> @@ -58,6 +58,7 @@ 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;
> @@ -983,13 +984,28 @@ static void netconfig_dhcp6_event_handler(struct l_dhcp6_client *client,
>   	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);

Please no.  It is fine to use __auto_type in macros where they're out of sight, 
but we shouldn't use this in the general case.

> +		L_AUTO_FREE_VAR(char *, addr_str) =
> +			l_dhcp6_lease_get_address(lease);

_auto_ would be preferred and shorter.

> +		struct l_rtnl_address *address;
> +
> +		address = l_rtnl_address_new(addr_str,
> +					l_dhcp6_lease_get_prefix_length(lease));
> +		l_rtnl_address_free(netconfig->v6_address);
> +		netconfig->v6_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;
>   
>   		/* Fall through */
>   	case L_DHCP6_CLIENT_EVENT_NO_LEASE:

<snip>

> @@ -1291,7 +1304,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);

Ugh.  Can we just make v6_address use _auto_.  The current upstream code is 
bonkers right now.

>   
>   		if (unlikely(!v6_address)) {
>   			l_error("netconfig: Can't parse IPv6 address");
> @@ -1309,11 +1321,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);

If the intent behind netconfig_load_settings to do more than validate the 
settings, then you should make sure you're not leaking anything.

Before you started refactoring the API, the semantics were quite clear:

netconfig_configure() --> Apply settings
netconfig_reset() -> Tear down

ad228461abbf ("netconfig: Move loading settings to new method, refactor") does 
not mention anything about changing the API semantics of this method, and for 
the most part it didn't.  I think only problem area is v4_address handling.

If you want to change the API semantics, then you need to explicitly call that 
out.   netconfig_load_settings() should either be safely callable multiple 
times, or introduce a new API that would tear down anything that 
netconfig_load_settings did.

>   		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);
>   

Regards,
-Denis