Hi Andrew,

On 11/8/21 5:28 AM, Andrew Zaborowski wrote:
> Refactor and unify how the netconfig values, that we obtain from DHCP or
> the settings or otherwise, are written/committed to the system netdevs.
> We basically concentrate all of those actions in a new function
> netconfig_set() that is called whenever any value changes.  The only
> exceptions are the sysfs writes and some of the ARP table writes.
> 
> This will allow us to more easily delegate committing the values to an
> (optional) D-Bus agent, or expose them as D-Bus properties if we ever
> choose to do that in the future.  For that I'm also passing a bitmask
> telling netconfig_set() which values may have actually changed, so we
> don't bother looking at those that definitely haven't changed.
> ---
>   src/netconfig.c | 541 +++++++++++++++++++++++++++++++++++-------------
>   1 file changed, 398 insertions(+), 143 deletions(-)
> 

It would be far easier if this was broken down into more digestible chunks.  I 
could even apply some right away since many make sense on their own.

> diff --git a/src/netconfig.c b/src/netconfig.c
> index e7fd4855..ea77e818 100644
> --- a/src/netconfig.c
> +++ b/src/netconfig.c
> @@ -52,6 +52,15 @@
>   #include "src/netconfig.h"
>   #include "src/sysfs.h"
>   
> +enum netconfig_changed_flags {
> +	NETCONFIG_CHANGED_ADDRESS	= 1 << 0,
> +	NETCONFIG_CHANGED_NETMASK	= 1 << 1,

Isn't netmask effectively part of the address?  If netmask changes, then you 
need to change the connected route and the address on the interface anyway.

> +	NETCONFIG_CHANGED_GATEWAY	= 1 << 2,
> +	NETCONFIG_CHANGED_DNS		= 1 << 3,
> +	NETCONFIG_CHANGED_DOMAINS	= 1 << 4,
> +	NETCONFIG_CHANGED_ALL		= 0x1f,
> +};
> +
>   struct netconfig {
>   	uint32_t ifindex;
>   	struct l_dhcp_client *dhcp_client;
> @@ -59,7 +68,9 @@ struct netconfig {
>   	uint8_t rtm_protocol;
>   	uint8_t rtm_v6_protocol;
>   	struct l_rtnl_address *v4_address;
> +	struct l_rtnl_address *stale_v4_address;
>   	struct l_rtnl_address *v6_address;
> +	struct l_rtnl_address *stale_v6_address;

I really don't think these belong here.  If we're going in the direction of 
netconfig delegating the management of applying IP configuration settings, then 
this is up to the applying entity to store this information.  netconfig would 
only be responsible for obtaining the settings and notifying the new management 
entity.

The new management entity would either:
A) invoke the agent, or
B) apply the settings itself.

Preferably, A & B would use the same API and be subclasses of some generic 
abstract interface.  Similarly to how we handle auth_protos, or perhaps modeled 
after resolv_ops.

>   	char **dns4_overrides;
>   	char **dns6_overrides;
>   	char **dns4_list;
> @@ -70,6 +81,8 @@ struct netconfig {
>   	char *v6_gateway_str;
>   	char *v4_domain;
>   	char **v6_domains;
> +	unsigned int pending_v4_set;
> +	unsigned int pending_v6_set;

These probably belong in the new management entity as well.

>   
>   	const struct l_settings *active_settings;
>   
> @@ -813,10 +826,8 @@ static void netconfig_route6_add_cb(int error, uint16_t type,
>   	}
>   }
>   
> -static bool netconfig_ipv4_routes_install(struct netconfig *netconfig)
> +static bool netconfig_ipv4_subnet_route_install(struct netconfig *netconfig)
>   {
> -	L_AUTO_FREE_VAR(char *, gateway) = NULL;
> -	const uint8_t *gateway_mac = NULL;
>   	struct in_addr in_addr;
>   	char ip[INET_ADDRSTRLEN];
>   	char network[INET_ADDRSTRLEN];
> @@ -839,10 +850,19 @@ static bool netconfig_ipv4_routes_install(struct netconfig *netconfig)
>   						netconfig_route_generic_cb,
>   						netconfig, NULL)) {
>   		l_error("netconfig: Failed to add subnet route.");
> -
>   		return false;
>   	}
>   
> +	return true;
> +}
> +
> +static bool netconfig_ipv4_gateway_route_install(struct netconfig *netconfig)
> +{
> +	L_AUTO_FREE_VAR(char *, gateway) = NULL;
> +	const uint8_t *gateway_mac = NULL;
> +	struct in_addr in_addr;
> +	char ip[INET_ADDRSTRLEN];
> +
>   	gateway = netconfig_ipv4_get_gateway(netconfig, &gateway_mac);
>   	if (!gateway) {
>   		l_debug("No gateway obtained from %s.",
> @@ -858,6 +878,10 @@ static bool netconfig_ipv4_routes_install(struct netconfig *netconfig)
>   		return true;
>   	}
>   
> +	if (!l_rtnl_address_get_address(netconfig->v4_address, ip) ||
> +			inet_pton(AF_INET, ip, &in_addr) < 1)
> +		return false;
> +
>   	netconfig->route4_add_gateway_cmd_id =
>   		l_rtnl_route4_add_gateway(rtnl, netconfig->ifindex, gateway, ip,
>   						ROUTE_PRIORITY_OFFSET,
> @@ -871,50 +895,77 @@ static bool netconfig_ipv4_routes_install(struct netconfig *netconfig)
>   		return false;
>   	}
>   
> -	if (gateway_mac) {
> -		/*
> -		 * Attempt to use the gateway MAC address received from the AP
> -		 * by writing the mapping directly into the netdev's ARP table
> -		 * so as to save one data frame roundtrip before first IP
> -		 * connections are established.  This is very low-priority but
> -		 * print error messages just because they may indicate bigger
> -		 * problems.
> -		 */
> -		if (!l_rtnl_neighbor_set_hwaddr(rtnl, netconfig->ifindex,
> +	/*
> +	 * Attempt to use the gateway MAC address received from the AP by
> +	 * writing the mapping directly into the netdev's ARP table so as
> +	 * to save one data frame roundtrip before first IP connections
> +	 * are established.  This is very low-priority but print error
> +	 * messages just because they may indicate bigger problems.
> +	 */
> +	if (gateway_mac && !l_rtnl_neighbor_set_hwaddr(rtnl, netconfig->ifindex,
>   					AF_INET,
>   					&netconfig->fils_override->ipv4_gateway,
>   					gateway_mac, 6,
>   					netconfig_set_neighbor_entry_cb, NULL,
>   					NULL))
> -			l_debug("l_rtnl_neighbor_set_hwaddr failed");
> +		l_debug("l_rtnl_neighbor_set_hwaddr failed");
> +
> +	return true;
> +}

I cherry-picked these changes into a separate commit, please review.

> +
> +static bool netconfig_ipv6_static_gateway_route_install(
> +						struct netconfig *netconfig)
> +{
> +	_auto_(l_rtnl_route_free) struct l_rtnl_route *gateway = NULL;
> +	const uint8_t *gateway_mac;
> +
> +	gateway = netconfig_get_static6_gateway(netconfig,
> +						&netconfig->v6_gateway_str,
> +						&gateway_mac);
> +	if (!gateway) {
> +		l_debug("No IPv6 gateway set");
> +		return true;
>   	}
>   
> +	netconfig->route6_add_cmd_id = l_rtnl_route_add(rtnl,
> +							netconfig->ifindex,
> +							gateway,
> +							netconfig_route6_add_cb,
> +							netconfig, NULL);
> +	if (!L_WARN_ON(unlikely(!netconfig->route6_add_cmd_id)))
> +		return false;
> +
> +	if (gateway_mac && !l_rtnl_neighbor_set_hwaddr(rtnl, netconfig->ifindex,
> +					AF_INET6,
> +					netconfig->fils_override->ipv6_gateway,
> +					gateway_mac, 6,
> +					netconfig_set_neighbor_entry_cb, NULL,
> +					NULL))
> +		l_debug("l_rtnl_neighbor_set_hwaddr failed");
> +
>   	return true;
>   }

Note that netconfig behaves differently with dhcp6 client vs dhcp.  With dhcp, 
netconfig is the one setting routes and addresses.  But with dhcp6, we let the 
dhcp6_client itself set the route and the address.  So this may need to be 
refactored to act similarly to how dhcp client is managed.  Otherwise the 
settings would be applied despite the agent being registered.

>   
> +static void netconfig_set(struct netconfig *netconfig, uint8_t af,
> +				unsigned int changed);
> +

doc/coding-style.txt item M17

<snip>

> @@ -978,21 +1005,169 @@ 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);
> +}
> +

This probably belongs in ell.

> +static void netconfig_set(struct netconfig *netconfig, uint8_t af,
> +				unsigned int changed)
> +{
> +	struct l_rtnl_address *stale_addr;
> +
> +	if (af == AF_INET)
> +		stale_addr = l_steal_ptr(netconfig->stale_v4_address);
> +	else
> +		stale_addr = l_steal_ptr(netconfig->stale_v6_address);
> +
> +	if (stale_addr) {
> +		L_WARN_ON(!l_rtnl_ifaddr_delete(rtnl, netconfig->ifindex,
> +						stale_addr,
> +						netconfig_ifaddr_del_cmd_cb,
> +						netconfig, NULL));
> +		l_rtnl_address_free(stale_addr);
> +	}
> +
> +	if (af == AF_INET) {
> +		if (!netconfig->v4_address)
> +			return;
> +
> +		if ((changed & (NETCONFIG_CHANGED_ADDRESS |
> +					NETCONFIG_CHANGED_NETMASK)) &&
> +				!netconfig->addr4_add_cmd_id) {
> +			netconfig->addr4_add_cmd_id = l_rtnl_ifaddr_add(rtnl,
> +					netconfig->ifindex,
> +					netconfig->v4_address,
> +					netconfig_ipv4_ifaddr_add_cmd_cb,
> +					netconfig, NULL);
> +			if (!L_WARN_ON(!netconfig->addr4_add_cmd_id))
> +				changed &= ~(NETCONFIG_CHANGED_ADDRESS |
> +						NETCONFIG_CHANGED_NETMASK);
> +		}
> +
> +		/*
> +		 * If we either started the address addition now or one was
> +		 * already in progress, wait for that operation's callback
> +		 * and we'll be called again to set the rest of the values
> +		 * in the changed bitmask.

So I'd rather see the ipv4_ifaddr_add_cmd_cb or an intermediate function being 
invoked than trying to re-invoke netconfig_set in the callback.

> +		 */
> +		if (netconfig->addr4_add_cmd_id) {
> +			netconfig->pending_v4_set |= changed;
> +			return;
> +		}
> +
> +		/*
> +		 * For IPv6 the address, the subnet route and the gateway
> +		 * route are handled automatically for DHCP, only cover the
> +		 * static and FILS-provided cases here.
> +		 */
> +	} else if (netconfig->rtm_v6_protocol == RTPROT_STATIC ||
> +			(netconfig->fils_override &&
> +			 !l_memeqzero(netconfig->fils_override->ipv6_addr,
> +					16))) {
> +		if (!netconfig->v6_address)
> +			return;
> +

See my comment above about dhcp6_client setting addresses & routes by itself.

> +		if ((changed & (NETCONFIG_CHANGED_ADDRESS |
> +					NETCONFIG_CHANGED_NETMASK)) &&
> +				!netconfig->addr6_add_cmd_id) {
> +			netconfig->addr6_add_cmd_id = l_rtnl_ifaddr_add(rtnl,
> +					netconfig->ifindex,
> +					netconfig->v6_address,
> +					netconfig_ipv6_ifaddr_add_cmd_cb,
> +					netconfig, NULL);
> +			if (!L_WARN_ON(!netconfig->addr6_add_cmd_id))
> +				changed &= ~(NETCONFIG_CHANGED_ADDRESS |
> +						NETCONFIG_CHANGED_NETMASK);
> +		}
> +
> +		/*
> +		 * If we either started the address addition now or one was
> +		 * already in progress, wait for that operation's callback
> +		 * and we'll be called again to set the rest of the values
> +		 * in the changed bitmask.
> +		 */

Is there a reason why we need to wait for the ipv4 address to be set prior to 
setting the v6 address?

> +		if (netconfig->addr6_add_cmd_id) {
> +			netconfig->pending_v4_set |= changed;
> +			return;
> +		}
> +	}
> +
> +	if (af == AF_INET) {
> +		if (changed & NETCONFIG_CHANGED_GATEWAY) {
> +			netconfig_gateway_to_arp(netconfig);
> +
> +			if (netconfig_ipv4_gateway_route_install(netconfig))
> +				changed &= ~NETCONFIG_CHANGED_GATEWAY;
> +		} else if (netconfig->notify) {

This is really quite weird since netconfig_ipv4_gateway_route_install() checks 
that the gateway is NULL.  What is this trying to take care of?

> +			/*
> +			 * The gateway route addition callback notifies the user
> +			 * of netconfig success if a gateway is provided, otherwise
> +			 * notify here.
> +			 */
> +			netconfig->notify(NETCONFIG_EVENT_CONNECTED,
> +						netconfig->user_data);
> +			netconfig->notify = NULL;
> +		}
> +	} else if (netconfig->rtm_v6_protocol == RTPROT_STATIC ||
> +			(netconfig->fils_override &&
> +			 !l_memeqzero(netconfig->fils_override->ipv6_gateway,
> +					16))) {
> +		if (changed & (NETCONFIG_CHANGED_GATEWAY))
> +			if (netconfig_ipv6_static_gateway_route_install(
> +								netconfig))
> +				changed &= ~NETCONFIG_CHANGED_GATEWAY;
> +	}
> +
> +	if (changed & NETCONFIG_CHANGED_DNS) {
> +		netconfig_set_dns(netconfig);
> +		changed &= ~NETCONFIG_CHANGED_DNS;
> +	}
> +
> +	if (changed & NETCONFIG_CHANGED_DOMAINS) {
> +		netconfig_set_domains(netconfig);
> +		changed &= ~NETCONFIG_CHANGED_DOMAINS;
> +	}
> +}
> +
>   static void netconfig_ipv4_dhcp_event_handler(struct l_dhcp_client *client,
>   						enum l_dhcp_client_event event,
>   						void *userdata)
>   {
>   	struct netconfig *netconfig = userdata;
> +	unsigned int changed = 0;
>   
>   	l_debug("DHCPv4 event %d", event);
>   
>   	switch (event) {
>   	case L_DHCP_CLIENT_EVENT_IP_CHANGED:
> -		L_WARN_ON(!l_rtnl_ifaddr_delete(rtnl, netconfig->ifindex,
> -					netconfig->v4_address,
> -					netconfig_ifaddr_del_cmd_cb,
> -					netconfig, NULL));
> -		/* Fall through. */
>   	case L_DHCP_CLIENT_EVENT_LEASE_OBTAINED:
>   	{
>   		char *gateway_str;
> @@ -1004,9 +1179,19 @@ static void netconfig_ipv4_dhcp_event_handler(struct l_dhcp_client *client,
>   		else {
>   			l_free(netconfig->v4_gateway_str);
>   			netconfig->v4_gateway_str = gateway_str;
> +			changed |= NETCONFIG_CHANGED_GATEWAY;
>   		}
>   
>   		address = netconfig_get_dhcp4_address(netconfig);
> +
> +		if (!netconfig_address_cmp_prefix_len(netconfig->v4_address,
> +							address))
> +			changed |= NETCONFIG_CHANGED_NETMASK;
> +
> +		if (!netconfig_address_cmp_address(netconfig->v4_address,
> +							address))
> +			changed |= NETCONFIG_CHANGED_ADDRESS;
> +

My impression is that the comparison of new vs old settings should be given over 
to the new management entity.

>   		l_rtnl_address_free(netconfig->v4_address);
>   		netconfig->v4_address = address;
>   
> @@ -1016,26 +1201,36 @@ static void netconfig_ipv4_dhcp_event_handler(struct l_dhcp_client *client,
>   			return;
>   		}
>   
> -		netconfig_dns_list_update(netconfig, AF_INET);
> -		netconfig_domains_update(netconfig, AF_INET);
> +		if (netconfig_dns_list_update(netconfig, AF_INET))
> +			changed |= NETCONFIG_CHANGED_DNS;
>   
> -		L_WARN_ON(!(netconfig->addr4_add_cmd_id =
> -				l_rtnl_ifaddr_add(rtnl, netconfig->ifindex,
> -					netconfig->v4_address,
> -					netconfig_ipv4_ifaddr_add_cmd_cb,
> -					netconfig, NULL)));
> +		if (netconfig_domains_update(netconfig, AF_INET))
> +			changed |= NETCONFIG_CHANGED_DOMAINS;
> +
> +		netconfig_set(netconfig, AF_INET, changed);
>   		break;
>   	}
>   	case L_DHCP_CLIENT_EVENT_LEASE_RENEWED:
>   		break;
>   	case L_DHCP_CLIENT_EVENT_LEASE_EXPIRED:
> -		L_WARN_ON(!l_rtnl_ifaddr_delete(rtnl, netconfig->ifindex,
> -					netconfig->v4_address,
> -					netconfig_ifaddr_del_cmd_cb,
> -					netconfig, NULL));
> -		l_rtnl_address_free(netconfig->v4_address);
> -		netconfig->v4_address = NULL;
> -		l_free(l_steal_ptr(netconfig->v4_gateway_str));
> +		l_debug("Lease for interface %u expired", netconfig->ifindex);
> +		changed = NETCONFIG_CHANGED_ADDRESS | NETCONFIG_CHANGED_NETMASK;
> +		l_rtnl_address_free(netconfig->stale_v4_address);
> +		netconfig->stale_v4_address =
> +			l_steal_ptr(netconfig->v4_address);
> +
> +		if (netconfig->v4_gateway_str) {
> +			changed |= NETCONFIG_CHANGED_GATEWAY;
> +			l_free(l_steal_ptr(netconfig->v4_gateway_str));

Gateway should be cleared, not changed.  I believe this is done automagically 
when removing the address.

> +		}
> +
> +		if (netconfig_dns_list_update(netconfig, AF_INET))
> +			changed |= NETCONFIG_CHANGED_DNS;
> +
> +		if (netconfig_domains_update(netconfig, AF_INET))
> +			changed |= NETCONFIG_CHANGED_DOMAINS;
> +
> +		netconfig_set(netconfig, AF_INET, changed);

Perhaps lease expiration, and thus the clearing out of all settings should be a 
separate operation?  There's not much point setting domains, dns, etc in this case.

>   
>   		/* Fall through. */
>   	case L_DHCP_CLIENT_EVENT_NO_LEASE:

<snip>

Regards,
-Denis