[PATCH 1/4] dhcp: Add public l_dhcp_lease_{new,free}

[PATCH 1/4] dhcp: Add public l_dhcp_lease_{new,free}

[Andrew Zaborowski]

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

Make the constructor & destructor for struct l_dhcp_lease public, this
is to be used with the new l_dhcp_client_set_lease() method.
---
 ell/dhcp-lease.c   |  8 ++++----
 ell/dhcp-private.h |  2 --
 ell/dhcp-server.c  | 18 +++++++++---------
 ell/dhcp.c         |  4 ++--
 ell/dhcp.h         |  2 ++
 ell/ell.sym        |  2 ++
 6 files changed, 19 insertions(+), 17 deletions(-)

diff --git a/ell/dhcp-lease.c b/ell/dhcp-lease.c
index c596c6b..a28a3f3 100644
--- a/ell/dhcp-lease.c
+++ b/ell/dhcp-lease.c
@@ -35,14 +35,14 @@
 #include "utf8.h"
 #include "net.h"
 
-struct l_dhcp_lease *_dhcp_lease_new(void)
+LIB_EXPORT struct l_dhcp_lease *l_dhcp_lease_new(void)
 {
 	struct l_dhcp_lease *ret = l_new(struct l_dhcp_lease, 1);
 
 	return ret;
 }
 
-void _dhcp_lease_free(struct l_dhcp_lease *lease)
+LIB_EXPORT void l_dhcp_lease_free(struct l_dhcp_lease *lease)
 {
 	if (!lease)
 		return;
@@ -56,7 +56,7 @@ void _dhcp_lease_free(struct l_dhcp_lease *lease)
 
 struct l_dhcp_lease *_dhcp_lease_parse_options(struct dhcp_message_iter *iter)
 {
-	struct l_dhcp_lease *lease = _dhcp_lease_new();
+	struct l_dhcp_lease *lease = l_dhcp_lease_new();
 	uint8_t t, l;
 	const void *v;
 
@@ -183,7 +183,7 @@ struct l_dhcp_lease *_dhcp_lease_parse_options(struct dhcp_message_iter *iter)
 
 	return lease;
 error:
-	_dhcp_lease_free(lease);
+	l_dhcp_lease_free(lease);
 	return NULL;
 }
 
diff --git a/ell/dhcp-private.h b/ell/dhcp-private.h
index bc10765..5864dc3 100644
--- a/ell/dhcp-private.h
+++ b/ell/dhcp-private.h
@@ -175,8 +175,6 @@ struct l_dhcp_lease {
 	bool offering : 1;
 };
 
-struct l_dhcp_lease *_dhcp_lease_new(void);
-void _dhcp_lease_free(struct l_dhcp_lease *lease);
 struct l_dhcp_lease *_dhcp_lease_parse_options(struct dhcp_message_iter *iter);
 
 struct dhcp_message_builder {
diff --git a/ell/dhcp-server.c b/ell/dhcp-server.c
index 05f9b73..5d9f3f6 100644
--- a/ell/dhcp-server.c
+++ b/ell/dhcp-server.c
@@ -257,12 +257,12 @@ static void set_next_expire_timer(struct l_dhcp_server *server,
 		if (!expired->offering) {
 			if (l_queue_length(server->expired_list) >
 					server->max_expired)
-				_dhcp_lease_free(l_queue_pop_head(
+				l_dhcp_lease_free(l_queue_pop_head(
 							server->expired_list));
 
 			l_queue_push_tail(server->expired_list, expired);
 		} else
-			_dhcp_lease_free(expired);
+			l_dhcp_lease_free(expired);
 	}
 
 	next = l_queue_peek_tail(server->lease_list);
@@ -356,7 +356,7 @@ static bool remove_lease(struct l_dhcp_server *server,
 	if (!l_queue_remove(server->lease_list, lease))
 		return false;
 
-	_dhcp_lease_free(lease);
+	l_dhcp_lease_free(lease);
 	set_next_expire_timer(server, NULL);
 	return true;
 }
@@ -449,7 +449,7 @@ static uint32_t find_free_or_expired_ip(struct l_dhcp_server *server,
 		return 0;
 
 	ip_addr = lease->address;
-	_dhcp_lease_free(lease);
+	l_dhcp_lease_free(lease);
 	return ip_addr;
 }
 
@@ -997,9 +997,9 @@ LIB_EXPORT void l_dhcp_server_destroy(struct l_dhcp_server *server)
 	l_free(server->ifname);
 
 	l_queue_destroy(server->lease_list,
-				(l_queue_destroy_func_t) _dhcp_lease_free);
+				(l_queue_destroy_func_t) l_dhcp_lease_free);
 	l_queue_destroy(server->expired_list,
-				(l_queue_destroy_func_t) _dhcp_lease_free);
+				(l_queue_destroy_func_t) l_dhcp_lease_free);
 
 	if (server->dns_list)
 		l_free(server->dns_list);
@@ -1414,7 +1414,7 @@ LIB_EXPORT bool l_dhcp_server_lease_remove(struct l_dhcp_server *server,
 			!l_queue_remove(server->expired_list, lease)))
 		return false;
 
-	_dhcp_lease_free(lease);
+	l_dhcp_lease_free(lease);
 	set_next_expire_timer(server, NULL);
 	return true;
 }
@@ -1440,11 +1440,11 @@ static bool dhcp_expire_by_mac(void *data, void *user_data)
 
 	if (!lease->offering) {
 		if (l_queue_length(server->expired_list) > server->max_expired)
-			_dhcp_lease_free(l_queue_pop_head(server->expired_list));
+			l_dhcp_lease_free(l_queue_pop_head(server->expired_list));
 
 		l_queue_push_tail(server->expired_list, lease);
 	} else
-		_dhcp_lease_free(lease);
+		l_dhcp_lease_free(lease);
 
 	expire_data->expired_cnt++;
 	return true;
diff --git a/ell/dhcp.c b/ell/dhcp.c
index 32e4a7c..e06ab05 100644
--- a/ell/dhcp.c
+++ b/ell/dhcp.c
@@ -705,7 +705,7 @@ static int dhcp_client_receive_ack(struct l_dhcp_client *client,
 				client->lease->router != lease->router)
 			r = L_DHCP_CLIENT_EVENT_IP_CHANGED;
 
-		_dhcp_lease_free(client->lease);
+		l_dhcp_lease_free(client->lease);
 	}
 
 	client->lease = lease;
@@ -1218,7 +1218,7 @@ LIB_EXPORT bool l_dhcp_client_stop(struct l_dhcp_client *client)
 	client->start_t = 0;
 	CLIENT_ENTER_STATE(DHCP_STATE_INIT);
 
-	_dhcp_lease_free(client->lease);
+	l_dhcp_lease_free(client->lease);
 	client->lease = NULL;
 
 	if (client->acd) {
diff --git a/ell/dhcp.h b/ell/dhcp.h
index 5873c90..4f18f01 100644
--- a/ell/dhcp.h
+++ b/ell/dhcp.h
@@ -104,6 +104,8 @@ bool l_dhcp_client_set_debug(struct l_dhcp_client *client,
 				l_dhcp_debug_cb_t function,
 				void *user_data, l_dhcp_destroy_cb_t destroy);
 
+struct l_dhcp_lease *l_dhcp_lease_new(void);
+void l_dhcp_lease_free(struct l_dhcp_lease *lease);
 
 char *l_dhcp_lease_get_address(const struct l_dhcp_lease *lease);
 char *l_dhcp_lease_get_gateway(const struct l_dhcp_lease *lease);
diff --git a/ell/ell.sym b/ell/ell.sym
index b29d98d..66ebf46 100644
--- a/ell/ell.sym
+++ b/ell/ell.sym
@@ -223,6 +223,8 @@ global:
 	l_dbus_remove_signal_watch;
 	l_dbus_name_acquire;
 	/* dhcp */
+	l_dhcp_lease_new;
+	l_dhcp_lease_free;
 	l_dhcp_lease_get_address;
 	l_dhcp_lease_get_gateway;
 	l_dhcp_lease_get_mac;
-- 
2.30.2

[PATCH 2/4] dhcp: Add some setters for struct l_dhcp_lease

[Andrew Zaborowski]

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

---
 ell/dhcp-lease.c | 107 +++++++++++++++++++++++++++++++++++++++++++++++
 ell/dhcp.h       |   8 ++++
 ell/ell.sym      |   6 +++
 3 files changed, 121 insertions(+)

diff --git a/ell/dhcp-lease.c b/ell/dhcp-lease.c
index a28a3f3..40dba83 100644
--- a/ell/dhcp-lease.c
+++ b/ell/dhcp-lease.c
@@ -300,3 +300,110 @@ LIB_EXPORT const uint8_t *l_dhcp_lease_get_mac(const struct l_dhcp_lease *lease)
 
 	return lease->mac;
 }
+
+LIB_EXPORT bool l_dhcp_lease_set_address(struct l_dhcp_lease *lease,
+						const char *address_str)
+{
+	struct in_addr in_addr;
+
+	if (unlikely(!lease || !address_str))
+		return false;
+
+	if (unlikely(inet_pton(AF_INET, address_str, &in_addr) != 1))
+		return false;
+
+	lease->address = in_addr.s_addr;
+	return true;
+}
+
+LIB_EXPORT bool l_dhcp_lease_set_gateway(struct l_dhcp_lease *lease,
+						const char *gateway_str)
+{
+	struct in_addr in_addr;
+
+	if (unlikely(!lease))
+		return false;
+
+	if (!gateway_str) {
+		lease->router = 0;
+		return true;
+	}
+
+	if (unlikely(inet_pton(AF_INET, gateway_str, &in_addr) != 1))
+		return false;
+
+	lease->router = in_addr.s_addr;
+	return true;
+}
+
+LIB_EXPORT bool l_dhcp_lease_set_mac(struct l_dhcp_lease *lease,
+					const uint8_t *mac)
+{
+	if (unlikely(!lease || !mac))
+		return false;
+
+	memcpy(lease->mac, mac, 6);
+	return true;
+}
+
+LIB_EXPORT bool l_dhcp_lease_set_dns(struct l_dhcp_lease *lease,
+					const char **dns_str_list)
+{
+	unsigned int n;
+	L_AUTO_FREE_VAR(uint32_t *, new_list) = NULL;
+
+	if (unlikely(!lease))
+		return false;
+
+	if (!dns_str_list) {
+		l_free(l_steal_ptr(lease->dns));
+		return true;
+	}
+
+	for (n = 0; dns_str_list[n]; n++);
+	new_list = l_new(uint32_t, n + 1);
+
+	for (n = 0; dns_str_list[n]; n++) {
+		struct in_addr in_addr;
+
+		if (unlikely(inet_pton(AF_INET, dns_str_list[n],
+					&in_addr) != 1))
+			return false;
+
+		new_list[n] = in_addr.s_addr;
+	}
+
+	l_free(lease->dns);
+	lease->dns = l_steal_ptr(new_list);
+	return true;
+}
+
+LIB_EXPORT bool l_dhcp_lease_set_lifetime(struct l_dhcp_lease *lease,
+						uint32_t lifetime)
+{
+	if (unlikely(!lease))
+		return false;
+
+	lease->lifetime = lifetime;
+	return true;
+}
+
+LIB_EXPORT bool l_dhcp_lease_set_server_id(struct l_dhcp_lease *lease,
+						const char *server_id_str)
+{
+	struct in_addr in_addr;
+
+	if (unlikely(!lease))
+		return false;
+
+	if (!server_id_str) {
+		lease->server_address = 0;
+		return true;
+	}
+
+	if (unlikely(inet_pton(AF_INET, server_id_str, &in_addr) != 1))
+		return false;
+
+	lease->server_address = in_addr.s_addr;
+	return true;
+}
diff --git a/ell/dhcp.h b/ell/dhcp.h
index 4f18f01..4406cd2 100644
--- a/ell/dhcp.h
+++ b/ell/dhcp.h
@@ -120,6 +120,14 @@ uint32_t l_dhcp_lease_get_t1(const struct l_dhcp_lease *lease);
 uint32_t l_dhcp_lease_get_t2(const struct l_dhcp_lease *lease);
 uint32_t l_dhcp_lease_get_lifetime(const struct l_dhcp_lease *lease);
 
+bool l_dhcp_lease_set_address(struct l_dhcp_lease *lease, const char *address);
+bool l_dhcp_lease_set_gateway(struct l_dhcp_lease *lease, const char *gateway);
+bool l_dhcp_lease_set_mac(struct l_dhcp_lease *lease, const uint8_t *mac);
+bool l_dhcp_lease_set_dns(struct l_dhcp_lease *lease, const char **dns);
+bool l_dhcp_lease_set_lifetime(struct l_dhcp_lease *lease, uint32_t lifetime);
+bool l_dhcp_lease_set_server_id(struct l_dhcp_lease *lease,
+				const char *address);
+
 struct l_dhcp_server *l_dhcp_server_new(int ifindex);
 void l_dhcp_server_destroy(struct l_dhcp_server *server);
 bool l_dhcp_server_start(struct l_dhcp_server *server);
diff --git a/ell/ell.sym b/ell/ell.sym
index 66ebf46..f96a52d 100644
--- a/ell/ell.sym
+++ b/ell/ell.sym
@@ -236,6 +236,12 @@ global:
 	l_dhcp_lease_get_t1;
 	l_dhcp_lease_get_t2;
 	l_dhcp_lease_get_lifetime;
+	l_dhcp_lease_set_address;
+	l_dhcp_lease_set_gateway;
+	l_dhcp_lease_set_mac;
+	l_dhcp_lease_set_dns;
+	l_dhcp_lease_set_lifetime;
+	l_dhcp_lease_set_server_id;
 	l_dhcp_client_new;
 	l_dhcp_client_destroy;
 	l_dhcp_client_add_request_option;
-- 
2.30.2

[PATCH 3/4] dhcp: Add l_dhcp_client_set_lease

[Andrew Zaborowski]

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

This method can be used when the DHCP server sends us a new lease
through an alternative protocol/transport such as when a lower layer's
(e.g. 802.11) initial handshake frames are reused to send the server's
IP assignment for faster connection setup.  If the lease has a defined
lifetime the user will want to use l_dhcp_client to track the expiry and
renewal of the leases.  The user will build an l_dhcp_lease object
using the data that was included in the handshake frames and call
l_dhcp_client_set_lease() and then the l_dhcp_client_start() as normal
once the ethernet link is setup.  The L_DHCP_CLIENT_EVENT_LEASE_OBTAINED
event will be sent inside the later l_dhcp_client_start() call.

With more work this method could be used for restoring the DHCP client
state from an older instance.
---
 ell/dhcp-lease.c   |  26 ++++
 ell/dhcp-private.h |   1 +
 ell/dhcp.c         | 311 ++++++++++++++++++++++++++++-----------------
 ell/dhcp.h         |   2 +
 ell/ell.sym        |   1 +
 5 files changed, 226 insertions(+), 115 deletions(-)

diff --git a/ell/dhcp-lease.c b/ell/dhcp-lease.c
index 40dba83..81d0943 100644
--- a/ell/dhcp-lease.c
+++ b/ell/dhcp-lease.c
@@ -54,6 +54,32 @@ LIB_EXPORT void l_dhcp_lease_free(struct l_dhcp_lease *lease)
 	l_free(lease);
 }
 
+struct l_dhcp_lease *_dhcp_lease_clone(const struct l_dhcp_lease *orig)
+{
+	struct l_dhcp_lease *lease;
+
+	if (unlikely(!orig))
+		return NULL;
+
+	lease = l_memdup(orig, sizeof(*lease));
+
+	if (lease->dns) {
+		unsigned int n;
+
+		for (n = 0; lease->dns[n]; n++);
+		lease->dns = l_memdup(lease->dns, (n + 1) * 4);
+	}
+
+	if (lease->domain_name)
+		lease->domain_name = l_strdup(lease->domain_name);
+
+	if (lease->client_id)
+		lease->client_id = l_memdup(lease->client_id,
+						lease->client_id[0] + 1);
+
+	return lease;
+}
+
 struct l_dhcp_lease *_dhcp_lease_parse_options(struct dhcp_message_iter *iter)
 {
 	struct l_dhcp_lease *lease = l_dhcp_lease_new();
diff --git a/ell/dhcp-private.h b/ell/dhcp-private.h
index 5864dc3..5b27e7e 100644
--- a/ell/dhcp-private.h
+++ b/ell/dhcp-private.h
@@ -175,6 +175,7 @@ struct l_dhcp_lease {
 	bool offering : 1;
 };
 
+struct l_dhcp_lease *_dhcp_lease_clone(const struct l_dhcp_lease *orig);
 struct l_dhcp_lease *_dhcp_lease_parse_options(struct dhcp_message_iter *iter);
 
 struct dhcp_message_builder {
diff --git a/ell/dhcp.c b/ell/dhcp.c
index e06ab05..49398d5 100644
--- a/ell/dhcp.c
+++ b/ell/dhcp.c
@@ -422,7 +422,8 @@ static int dhcp_client_send_request(struct l_dhcp_client *client)
 		 *
 		 * NOTE: 'SELECTING' is meant to be 'REQUESTING' in the RFC
 		 */
-		if (!_dhcp_message_builder_append(&builder,
+		if (client->lease->server_address &&
+				!_dhcp_message_builder_append(&builder,
 					L_DHCP_OPTION_SERVER_IDENTIFIER,
 					4, &client->lease->server_address)) {
 			CLIENT_DEBUG("Failed to append server ID");
@@ -468,7 +469,8 @@ static int dhcp_client_send_request(struct l_dhcp_client *client)
 	 * 'server identifier' option for any unicast requests to the DHCP
 	 * server.
 	 */
-	if (client->state == DHCP_STATE_RENEWING)
+	if (client->state == DHCP_STATE_RENEWING &&
+			client->lease->server_address)
 		return dhcp_client_send_unicast(client, request, len);
 
 	return client->transport->l2_send(client->transport,
@@ -504,7 +506,8 @@ static void dhcp_client_send_release(struct l_dhcp_client *client)
 
 	request->ciaddr = client->lease->address;
 
-	if (!_dhcp_message_builder_append(&builder,
+	if (client->lease->server_address &&
+			!_dhcp_message_builder_append(&builder,
 					L_DHCP_OPTION_SERVER_IDENTIFIER,
 					4, &client->lease->server_address)) {
 		CLIENT_DEBUG("Failed to append server ID");
@@ -513,7 +516,15 @@ static void dhcp_client_send_release(struct l_dhcp_client *client)
 
 	_dhcp_message_builder_finalize(&builder, &len);
 
-	dhcp_client_send_unicast(client, request, len);
+	if (client->lease->server_address) {
+		dhcp_client_send_unicast(client, request, len);
+		return;
+	}
+
+	client->transport->l2_send(client->transport,
+					INADDR_ANY, DHCP_PORT_CLIENT,
+					INADDR_BROADCAST, DHCP_PORT_SERVER,
+					NULL, request, len);
 }
 
 static void dhcp_client_timeout_resend(struct l_timeout *timeout,
@@ -672,6 +683,40 @@ static void dhcp_client_address_add_cb(int error, uint16_t type,
 	}
 }
 
+static void dhcp_client_address_add(struct l_dhcp_client *client)
+{
+	struct l_rtnl_address *a;
+	L_AUTO_FREE_VAR(char *, ip) = l_dhcp_lease_get_address(client->lease);
+	uint8_t prefix_len;
+	uint32_t l = l_dhcp_lease_get_lifetime(client->lease);
+	L_AUTO_FREE_VAR(char *, netmask) =
+			l_dhcp_lease_get_netmask(client->lease);
+	L_AUTO_FREE_VAR(char *, broadcast) =
+			l_dhcp_lease_get_broadcast(client->lease);
+	struct in_addr in_addr;
+
+	if (inet_pton(AF_INET, netmask, &in_addr) > 0)
+		prefix_len = __builtin_popcountl(in_addr.s_addr);
+	else
+		prefix_len = 24;
+
+	a = l_rtnl_address_new(ip, prefix_len);
+	l_rtnl_address_set_noprefixroute(a, true);
+	l_rtnl_address_set_lifetimes(a, l, l);
+	l_rtnl_address_set_broadcast(a, broadcast);
+
+	client->rtnl_add_cmdid =
+		l_rtnl_ifaddr_add(client->rtnl, client->ifindex, a,
+					dhcp_client_address_add_cb,
+					client, NULL);
+	if (client->rtnl_add_cmdid)
+		client->rtnl_configured_address = a;
+	else {
+		CLIENT_DEBUG("Configuring address via RTNL failed");
+		l_rtnl_address_free(a);
+	}
+}
+
 static int dhcp_client_receive_ack(struct l_dhcp_client *client,
 					const struct dhcp_message *ack,
 					size_t len)
@@ -715,39 +760,8 @@ static int dhcp_client_receive_ack(struct l_dhcp_client *client,
 			client->state == DHCP_STATE_REBOOTING)
 		r = L_DHCP_CLIENT_EVENT_LEASE_OBTAINED;
 
-	if (client->rtnl) {
-		struct l_rtnl_address *a;
-		L_AUTO_FREE_VAR(char *, ip) =
-			l_dhcp_lease_get_address(client->lease);
-		uint8_t prefix_len;
-		uint32_t l = l_dhcp_lease_get_lifetime(client->lease);
-		L_AUTO_FREE_VAR(char *, netmask) =
-				l_dhcp_lease_get_netmask(client->lease);
-		L_AUTO_FREE_VAR(char *, broadcast) =
-				l_dhcp_lease_get_broadcast(client->lease);
-		struct in_addr in_addr;
-
-		if (inet_pton(AF_INET, netmask, &in_addr) > 0)
-			prefix_len = __builtin_popcountl(in_addr.s_addr);
-		else
-			prefix_len = 24;
-
-		a = l_rtnl_address_new(ip, prefix_len);
-		l_rtnl_address_set_noprefixroute(a, true);
-		l_rtnl_address_set_lifetimes(a, l, l);
-		l_rtnl_address_set_broadcast(a, broadcast);
-
-		client->rtnl_add_cmdid =
-			l_rtnl_ifaddr_add(client->rtnl, client->ifindex, a,
-						dhcp_client_address_add_cb,
-						client, NULL);
-		if (client->rtnl_add_cmdid)
-			client->rtnl_configured_address = a;
-		else {
-			CLIENT_DEBUG("Configuring address via RTNL failed");
-			l_rtnl_address_free(a);
-		}
-	}
+	if (client->rtnl)
+		dhcp_client_address_add(client);
 
 	return r;
 }
@@ -775,18 +789,93 @@ static int dhcp_client_receive_offer(struct l_dhcp_client *client,
 	return 0;
 }
 
+static void dhcp_client_bound(struct l_dhcp_client *client)
+{
+	char buf[INET_ADDRSTRLEN];
+	struct in_addr ia;
+
+	CLIENT_ENTER_STATE(DHCP_STATE_BOUND);
+	l_timeout_remove(client->timeout_resend);
+	client->timeout_resend = NULL;
+
+	if (client->transport->bind) {
+		int e = client->transport->bind(client->transport,
+						client->lease->address);
+
+		if (e < 0) {
+			CLIENT_DEBUG("Failed to bind dhcp socket. "
+				"Error %d: %s", e, strerror(-e));
+		}
+	}
+
+	client->lease->bound_time = l_time_now();
+
+	/*
+	 * Start T1, once it expires we will start the T2 timer.  If
+	 * we renew the lease, we will end up back here.
+	 *
+	 * RFC2131, Section 4.4.5 states:
+	 * "Times T1 and T2 SHOULD be chosen with some random "fuzz"
+	 * around a fixed value, to avoid synchronization of client
+	 * reacquisition."
+	 */
+	l_timeout_remove(client->timeout_lease);
+	client->timeout_lease = NULL;
+
+	/* Infinite lease, no need to start t1 */
+	if (client->lease->lifetime != 0xffffffffu) {
+		uint32_t next_timeout = dhcp_fuzz_secs(client->lease->t1);
+
+		CLIENT_DEBUG("T1 expiring in %u ms", next_timeout);
+		client->timeout_lease = l_timeout_create_ms(next_timeout,
+							dhcp_client_t1_expired,
+							client, NULL);
+	}
+
+	/* ACD is already running, no need to re-announce */
+	if (client->acd)
+		return;
+
+	client->acd = l_acd_new(client->ifindex);
+
+	if (client->debug_handler)
+		l_acd_set_debug(client->acd, client->debug_handler,
+				client->debug_data,
+				client->debug_destroy);
+
+	/*
+	 * TODO: There is no mechanism yet to deal with IPs leased by
+	 * the DHCP server which conflict with other devices. For now
+	 * the ACD object is being initialized to defend infinitely
+	 * which is effectively no different than the non-ACD behavior
+	 * (ignore conflicts and continue using address). The only
+	 * change is that announcements will be sent if conflicts are
+	 * found.
+	 */
+	l_acd_set_defend_policy(client->acd, L_ACD_DEFEND_POLICY_INFINITE);
+	l_acd_set_skip_probes(client->acd, true);
+
+	ia.s_addr = client->lease->address;
+	inet_ntop(AF_INET, &ia, buf, INET_ADDRSTRLEN);
+
+	/* For unit testing we don't want this to be a fatal error */
+	if (!l_acd_start(client->acd, buf)) {
+		CLIENT_DEBUG("Failed to start ACD on %s, continuing", buf);
+		l_acd_destroy(client->acd);
+		client->acd = NULL;
+	}
+}
+
 static void dhcp_client_rx_message(const void *data, size_t len, void *userdata,
 					const uint8_t *saddr)
 {
 	struct l_dhcp_client *client = userdata;
 	const struct dhcp_message *message = data;
 	struct dhcp_message_iter iter;
-	char buf[INET_ADDRSTRLEN];
 	uint8_t msg_type = 0;
 	uint8_t t, l;
 	const void *v;
-	int r, e;
-	struct in_addr ia;
+	int r;
 
 	CLIENT_DEBUG("");
 
@@ -866,82 +955,8 @@ static void dhcp_client_rx_message(const void *data, size_t len, void *userdata,
 		if (r < 0)
 			return;
 
-		CLIENT_ENTER_STATE(DHCP_STATE_BOUND);
-		l_timeout_remove(client->timeout_resend);
-		client->timeout_resend = NULL;
-
-		if (client->transport->bind) {
-			e = client->transport->bind(client->transport,
-						client->lease->address);
-			if (e < 0) {
-				CLIENT_DEBUG("Failed to bind dhcp socket. "
-					"Error %d: %s", e, strerror(-e));
-			}
-		}
-
+		dhcp_client_bound(client);
 		dhcp_client_event_notify(client, r);
-
-		client->lease->bound_time = l_time_now();
-
-		/*
-		 * Start T1, once it expires we will start the T2 timer.  If
-		 * we renew the lease, we will end up back here.
-		 *
-		 * RFC2131, Section 4.4.5 states:
-		 * "Times T1 and T2 SHOULD be chosen with some random "fuzz"
-		 * around a fixed value, to avoid synchronization of client
-		 * reacquisition."
-		 */
-		l_timeout_remove(client->timeout_lease);
-		client->timeout_lease = NULL;
-
-		/* Infinite lease, no need to start t1 */
-		if (client->lease->lifetime != 0xffffffffu) {
-			uint32_t next_timeout =
-					dhcp_fuzz_secs(client->lease->t1);
-
-			CLIENT_DEBUG("T1 expiring in %u ms", next_timeout);
-			client->timeout_lease =
-				l_timeout_create_ms(next_timeout,
-							dhcp_client_t1_expired,
-							client, NULL);
-		}
-
-		/* ACD is already running, no need to re-announce */
-		if (client->acd)
-			break;
-
-		client->acd = l_acd_new(client->ifindex);
-
-		if (client->debug_handler)
-			l_acd_set_debug(client->acd, client->debug_handler,
-					client->debug_data,
-					client->debug_destroy);
-
-		/*
-		 * TODO: There is no mechanism yet to deal with IPs leased by
-		 * the DHCP server which conflict with other devices. For now
-		 * the ACD object is being initialized to defend infinitely
-		 * which is effectively no different than the non-ACD behavior
-		 * (ignore conflicts and continue using address). The only
-		 * change is that announcements will be sent if conflicts are
-		 * found.
-		 */
-		l_acd_set_defend_policy(client->acd,
-						L_ACD_DEFEND_POLICY_INFINITE);
-		l_acd_set_skip_probes(client->acd, true);
-
-		ia.s_addr = client->lease->address;
-		inet_ntop(AF_INET, &ia, buf, INET_ADDRSTRLEN);
-
-		/* For unit testing we don't want this to be a fatal error */
-		if (!l_acd_start(client->acd, buf)) {
-			CLIENT_DEBUG("Failed to start ACD on %s, continuing",
-						buf);
-			l_acd_destroy(client->acd);
-			client->acd = NULL;
-		}
-
 		break;
 	case DHCP_STATE_INIT_REBOOT:
 	case DHCP_STATE_REBOOTING:
@@ -1114,6 +1129,62 @@ LIB_EXPORT const struct l_dhcp_lease *l_dhcp_client_get_lease(
 	return client->lease;
 }
 
+/*
+ * This is mainly to be used when the DHCP lease has been assigned by the
+ * server and communicated to the client through an alternative protocol,
+ * or to restore a saved state.
+ */
+LIB_EXPORT void l_dhcp_client_set_lease(struct l_dhcp_client *client,
+					const struct l_dhcp_lease *lease)
+{
+	int r = L_DHCP_CLIENT_EVENT_LEASE_OBTAINED;
+
+	if (unlikely(!client || !lease))
+		return;
+
+	l_dhcp_lease_free(client->lease);
+	client->lease = _dhcp_lease_clone(lease);
+
+	if (!client->lease->broadcast)
+		client->lease->broadcast = client->lease->address |
+			~client->lease->subnet_mask;
+
+	if (!client->lease->lifetime)
+		client->lease->lifetime = 0xffffffffu;
+
+	if (client->lease->lifetime != 0xffffffffu) {
+		if (!client->lease->t1)
+			client->lease->t1 = client->lease->lifetime / 2;
+
+		if (!client->lease->t2)
+			client->lease->t2 = client->lease->lifetime / 8 * 7;
+	}
+
+	/*
+	 * If l_dhcp_client_start() hasn't happened yet, everything else
+	 * will be done there.
+	 */
+	if (client->state == DHCP_STATE_INIT)
+		return;
+
+	if (client->rtnl_configured_address) {
+		l_rtnl_ifaddr_delete(client->rtnl, client->ifindex,
+					client->rtnl_configured_address,
+					NULL, NULL, NULL);
+		l_rtnl_address_free(client->rtnl_configured_address);
+		client->rtnl_configured_address = NULL;
+
+		r = L_DHCP_CLIENT_EVENT_IP_CHANGED;
+	}
+
+	if (client->rtnl)
+		dhcp_client_address_add(client);
+
+	dhcp_client_bound(client);
+	dhcp_client_event_notify(client, r);
+	return;
+}
+
 LIB_EXPORT bool l_dhcp_client_start(struct l_dhcp_client *client)
 {
 	int err;
@@ -1164,6 +1235,16 @@ LIB_EXPORT bool l_dhcp_client_start(struct l_dhcp_client *client)
 
 	client->start_t = l_time_now();
 
+	if (client->lease) {
+		if (client->rtnl)
+			dhcp_client_address_add(client);
+
+		dhcp_client_bound(client);
+		dhcp_client_event_notify(client,
+					L_DHCP_CLIENT_EVENT_LEASE_OBTAINED);
+		return true;
+	}
+
 	err = dhcp_client_send_discover(client);
 	if (err < 0)
 		return false;
diff --git a/ell/dhcp.h b/ell/dhcp.h
index 4406cd2..36a693c 100644
--- a/ell/dhcp.h
+++ b/ell/dhcp.h
@@ -91,6 +91,8 @@ bool l_dhcp_client_set_rtnl(struct l_dhcp_client *client,
 
 const struct l_dhcp_lease *l_dhcp_client_get_lease(
 					const struct l_dhcp_client *client);
+void l_dhcp_client_set_lease(struct l_dhcp_client *client,
+					const struct l_dhcp_lease *lease);
 
 bool l_dhcp_client_start(struct l_dhcp_client *client);
 bool l_dhcp_client_stop(struct l_dhcp_client *client);
diff --git a/ell/ell.sym b/ell/ell.sym
index f96a52d..860f4fc 100644
--- a/ell/ell.sym
+++ b/ell/ell.sym
@@ -250,6 +250,7 @@ global:
 	l_dhcp_client_set_rtnl;
 	l_dhcp_client_set_hostname;
 	l_dhcp_client_get_lease;
+	l_dhcp_client_set_lease;
 	l_dhcp_client_start;
 	l_dhcp_client_stop;
 	l_dhcp_client_set_event_handler;
-- 
2.30.2

[PATCH 4/4] dhcp: Optimize dhcp_client_address_add sligtly

[Andrew Zaborowski]

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

Don't allocate/build a netmask string when we can use
client->lease->subnet_mask directly -- we access client->lease fields
directly in most of this file.
---
 ell/dhcp.c | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/ell/dhcp.c b/ell/dhcp.c
index 49398d5..7878f0c 100644
--- a/ell/dhcp.c
+++ b/ell/dhcp.c
@@ -685,22 +685,13 @@ static void dhcp_client_address_add_cb(int error, uint16_t type,
 
 static void dhcp_client_address_add(struct l_dhcp_client *client)
 {
-	struct l_rtnl_address *a;
 	L_AUTO_FREE_VAR(char *, ip) = l_dhcp_lease_get_address(client->lease);
-	uint8_t prefix_len;
+	uint8_t prefix_len = __builtin_popcountl(client->lease->subnet_mask);
+	struct l_rtnl_address *a = l_rtnl_address_new(ip, prefix_len);
 	uint32_t l = l_dhcp_lease_get_lifetime(client->lease);
-	L_AUTO_FREE_VAR(char *, netmask) =
-			l_dhcp_lease_get_netmask(client->lease);
 	L_AUTO_FREE_VAR(char *, broadcast) =
 			l_dhcp_lease_get_broadcast(client->lease);
-	struct in_addr in_addr;
 
-	if (inet_pton(AF_INET, netmask, &in_addr) > 0)
-		prefix_len = __builtin_popcountl(in_addr.s_addr);
-	else
-		prefix_len = 24;
-
-	a = l_rtnl_address_new(ip, prefix_len);
 	l_rtnl_address_set_noprefixroute(a, true);
 	l_rtnl_address_set_lifetimes(a, l, l);
 	l_rtnl_address_set_broadcast(a, broadcast);
-- 
2.30.2

Re: [PATCH 3/4] dhcp: Add l_dhcp_client_set_lease

[Denis Kenzior]

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

Hi Andrew,

On 8/12/21 3:51 PM, Andrew Zaborowski wrote:
> This method can be used when the DHCP server sends us a new lease
> through an alternative protocol/transport such as when a lower layer's
> (e.g. 802.11) initial handshake frames are reused to send the server's
> IP assignment for faster connection setup.  If the lease has a defined
> lifetime the user will want to use l_dhcp_client to track the expiry and
> renewal of the leases.  The user will build an l_dhcp_lease object
> using the data that was included in the handshake frames and call
> l_dhcp_client_set_lease() and then the l_dhcp_client_start() as normal
> once the ethernet link is setup.  The L_DHCP_CLIENT_EVENT_LEASE_OBTAINED
> event will be sent inside the later l_dhcp_client_start() call.
> 
> With more work this method could be used for restoring the DHCP client
> state from an older instance.
> ---
>   ell/dhcp-lease.c   |  26 ++++
>   ell/dhcp-private.h |   1 +
>   ell/dhcp.c         | 311 ++++++++++++++++++++++++++++-----------------
>   ell/dhcp.h         |   2 +
>   ell/ell.sym        |   1 +
>   5 files changed, 226 insertions(+), 115 deletions(-)
> 

<snip>

> diff --git a/ell/dhcp.c b/ell/dhcp.c
> index e06ab05..49398d5 100644
> --- a/ell/dhcp.c
> +++ b/ell/dhcp.c
> @@ -422,7 +422,8 @@ static int dhcp_client_send_request(struct l_dhcp_client *client)
>   		 *
>   		 * NOTE: 'SELECTING' is meant to be 'REQUESTING' in the RFC
>   		 */
> -		if (!_dhcp_message_builder_append(&builder,
> +		if (client->lease->server_address &&

I'm not sure this is right according to the RFC?  Since we are in the Requesting 
state, the server_address must be valid.  If it is somehow not, then shouldn't 
we be in Init-Reboot or Rebinding state instead?

> +				!_dhcp_message_builder_append(&builder,
>   					L_DHCP_OPTION_SERVER_IDENTIFIER,
>   					4, &client->lease->server_address)) {
>   			CLIENT_DEBUG("Failed to append server ID");
> @@ -468,7 +469,8 @@ static int dhcp_client_send_request(struct l_dhcp_client *client)
>   	 * 'server identifier' option for any unicast requests to the DHCP
>   	 * server.
>   	 */
> -	if (client->state == DHCP_STATE_RENEWING)
> +	if (client->state == DHCP_STATE_RENEWING &&
> +			client->lease->server_address)

I think this is against the spec?

>   		return dhcp_client_send_unicast(client, request, len);
>   
>   	return client->transport->l2_send(client->transport,
> @@ -504,7 +506,8 @@ static void dhcp_client_send_release(struct l_dhcp_client *client)
>   
>   	request->ciaddr = client->lease->address;
>   
> -	if (!_dhcp_message_builder_append(&builder,
> +	if (client->lease->server_address &&
> +			!_dhcp_message_builder_append(&builder,

Should we even bother with this if server_address is NULL?  If we have no server 
address to send a release to, then we shouldn't even get here.

I really don't think that DHCP servers will somehow work completely against the 
relevant RFCs in order to support this FILS IP Address assignment use case.  So 
the question is what do we do if a non-infinite validity time for an address is 
given to us.

I can see 3 things that might happen:
- We ask for a PTK Rekey, or the AP triggers one for us.  This should result in 
a new IP Address coming our way.
- We issue a DHCP Inform with our configured address, and some DHCP Server on 
the network confirms it.  We learn the DHCP Server address this way.
- Once the validity time is almost up, we go into Init-Reboot or Rebinding state 
and hope some DHCP Server on the network takes care of us.

The absence of the server address is probably the reason why 'DHCP over FILS' is 
an option in the first place.  With DHCPv4 Rapid Commit it is fully possible to 
only need 2 messages (Associate/Associate Resp) to make this work nicely.

>   					L_DHCP_OPTION_SERVER_IDENTIFIER,
>   					4, &client->lease->server_address)) {
>   		CLIENT_DEBUG("Failed to append server ID");
> @@ -513,7 +516,15 @@ static void dhcp_client_send_release(struct l_dhcp_client *client)
>   
>   	_dhcp_message_builder_finalize(&builder, &len);
>   
> -	dhcp_client_send_unicast(client, request, len);
> +	if (client->lease->server_address) {
> +		dhcp_client_send_unicast(client, request, len);
> +		return;
> +	}
> +

This looks wrong according to the RFC as well?

> +	client->transport->l2_send(client->transport,
> +					INADDR_ANY, DHCP_PORT_CLIENT,
> +					INADDR_BROADCAST, DHCP_PORT_SERVER,
> +					NULL, request, len);
>   }
>   
>   static void dhcp_client_timeout_resend(struct l_timeout *timeout,

<snip>

> @@ -1114,6 +1129,62 @@ LIB_EXPORT const struct l_dhcp_lease *l_dhcp_client_get_lease(
>   	return client->lease;
>   }
>   
> +/*
> + * This is mainly to be used when the DHCP lease has been assigned by the
> + * server and communicated to the client through an alternative protocol,
> + * or to restore a saved state.
> + */
> +LIB_EXPORT void l_dhcp_client_set_lease(struct l_dhcp_client *client,
> +					const struct l_dhcp_lease *lease)
> +{
> +	int r = L_DHCP_CLIENT_EVENT_LEASE_OBTAINED;
> +
> +	if (unlikely(!client || !lease))
> +		return;
> +
> +	l_dhcp_lease_free(client->lease);
> +	client->lease = _dhcp_lease_clone(lease);

I actually wonder why you'd want to clone the lease here and not just take 
ownership of the passed in lease object?  What is the caller going to do with it 
besides free it?

I have no real objection to the other refactoring in this patch.  It might have 
been easier to send that separately first.

Regards,
-Denis

Re: [PATCH 3/4] dhcp: Add l_dhcp_client_set_lease

[Andrew Zaborowski]

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

Hi Denis,

On Fri, 13 Aug 2021 at 17:48, Denis Kenzior <denkenz@gmail.com> wrote:
> On 8/12/21 3:51 PM, Andrew Zaborowski wrote:
> > @@ -422,7 +422,8 @@ static int dhcp_client_send_request(struct l_dhcp_client *client)
> >                *
> >                * NOTE: 'SELECTING' is meant to be 'REQUESTING' in the RFC
> >                */
> > -             if (!_dhcp_message_builder_append(&builder,
> > +             if (client->lease->server_address &&
>
> I'm not sure this is right according to the RFC?  Since we are in the Requesting
> state, the server_address must be valid.  If it is somehow not, then shouldn't
> we be in Init-Reboot or Rebinding state instead?

True, this change should not be needed.

>
> > +                             !_dhcp_message_builder_append(&builder,
> >                                       L_DHCP_OPTION_SERVER_IDENTIFIER,
> >                                       4, &client->lease->server_address)) {
> >                       CLIENT_DEBUG("Failed to append server ID");
> > @@ -468,7 +469,8 @@ static int dhcp_client_send_request(struct l_dhcp_client *client)
> >        * 'server identifier' option for any unicast requests to the DHCP
> >        * server.
> >        */
> > -     if (client->state == DHCP_STATE_RENEWING)
> > +     if (client->state == DHCP_STATE_RENEWING &&
> > +                     client->lease->server_address)
>
> I think this is against the spec?

So in both cases the check should be always true if
l_dhcp_client_set_lease was not used...  Checking an always-true
condition isn't against the spec.

>
> >               return dhcp_client_send_unicast(client, request, len);
> >
> >       return client->transport->l2_send(client->transport,
> > @@ -504,7 +506,8 @@ static void dhcp_client_send_release(struct l_dhcp_client *client)
> >
> >       request->ciaddr = client->lease->address;
> >
> > -     if (!_dhcp_message_builder_append(&builder,
> > +     if (client->lease->server_address &&
> > +                     !_dhcp_message_builder_append(&builder,
>
> Should we even bother with this if server_address is NULL?  If we have no server
> address to send a release to, then we shouldn't even get here.

So we can move the check to the caller.  There's also a bigger
question of whether we should call dhcp_client_send_release() at all.
The only current caller is l_dhcp_client_stop.  I believe most clients
don't use DHCPRELEASE because they cache their leases for future use,
so maybe it makes sense that we send it for now because we don't save
our leases.  In the FILS and P2P cases (which can only exist with
802.11) the AP knows exactly when we disconnect so it can take
whatever action is needed without the DHCPRELEASE.

>
> I really don't think that DHCP servers will somehow work completely against the
> relevant RFCs in order to support this FILS IP Address assignment use case.

On one hand RFC2131 says which messages should be unicast vs.
broadcast, and which messages should contain the "Server Identifier"
option, but it also just says:

"The DHCP client broadcasts DHCPDISCOVER, DHCPREQUEST and DHCPINFORM
messages, unless the client knows the address of a DHCP server."

In the FILS case we don't know the IP of the server until our first
renewal.  You're right that if we strictly follow 4.3.2 or 4.4.5, both
of which say that the DHCPREQUEST in the RENEWING state should be
unicast, we have no way of renewing the lease in a compliant way.  On
the other hand, from when I looked at whether the chaddr field should
be validated against the source address, I found most servers don't
look at either the source or destination addresses by default.

Then again we do have the server's MAC (in IWD...), we just don't have
its IP so we can unicast the message using transport->l2_send maybe?

> So
> the question is what do we do if a non-infinite validity time for an address is
> given to us.

So there are two things that we can do that will be fully compliant
with all specs:

* Instead of renewing the IP by going to the RENEWING state, we can go
back to INIT and go through the full DISCOVER/OFFER/REQUEST/ACK.  We
may be offered the same IP, and if we're offered a different one then
we can still live with it.  I guess, to implement this we don't need
my l_dhcp_client_set_lease() method, we can track the lease expiry in
IWD and start the l_dhcp_client only after the initial lease is close
to expiry.

* Track the lease expiry time in IWD and request a new IP by sending a
fils Robust Action Frame with the IP Address Assignment IE again.

>
> I can see 3 things that might happen:
> - We ask for a PTK Rekey, or the AP triggers one for us.  This should result in
> a new IP Address coming our way.

It's the client that triggers the IP assignment so we don't have to go that far.

> - We issue a DHCP Inform with our configured address, and some DHCP Server on
> the network confirms it.  We learn the DHCP Server address this way.
> - Once the validity time is almost up, we go into Init-Reboot or Rebinding state
> and hope some DHCP Server on the network takes care of us.
>
> The absence of the server address is probably the reason why 'DHCP over FILS' is
> an option in the first place.  With DHCPv4 Rapid Commit it is fully possible to
> only need 2 messages (Associate/Associate Resp) to make this work nicely.
>
> >                                       L_DHCP_OPTION_SERVER_IDENTIFIER,
> >                                       4, &client->lease->server_address)) {
> >               CLIENT_DEBUG("Failed to append server ID");
> > @@ -513,7 +516,15 @@ static void dhcp_client_send_release(struct l_dhcp_client *client)
> >
> >       _dhcp_message_builder_finalize(&builder, &len);
> >
> > -     dhcp_client_send_unicast(client, request, len);
> > +     if (client->lease->server_address) {
> > +             dhcp_client_send_unicast(client, request, len);
> > +             return;
> > +     }
> > +
>
> This looks wrong according to the RFC as well?
>
> > +     client->transport->l2_send(client->transport,
> > +                                     INADDR_ANY, DHCP_PORT_CLIENT,
> > +                                     INADDR_BROADCAST, DHCP_PORT_SERVER,
> > +                                     NULL, request, len);
> >   }
> >
> >   static void dhcp_client_timeout_resend(struct l_timeout *timeout,
>
> <snip>
>
> > @@ -1114,6 +1129,62 @@ LIB_EXPORT const struct l_dhcp_lease *l_dhcp_client_get_lease(
> >       return client->lease;
> >   }
> >
> > +/*
> > + * This is mainly to be used when the DHCP lease has been assigned by the
> > + * server and communicated to the client through an alternative protocol,
> > + * or to restore a saved state.
> > + */
> > +LIB_EXPORT void l_dhcp_client_set_lease(struct l_dhcp_client *client,
> > +                                     const struct l_dhcp_lease *lease)
> > +{
> > +     int r = L_DHCP_CLIENT_EVENT_LEASE_OBTAINED;
> > +
> > +     if (unlikely(!client || !lease))
> > +             return;
> > +
> > +     l_dhcp_lease_free(client->lease);
> > +     client->lease = _dhcp_lease_clone(lease);
>
> I actually wonder why you'd want to clone the lease here and not just take
> ownership of the passed in lease object?  What is the caller going to do with it
> besides free it?

The only reason was that IIRC you didn't like APIs where a method
takes ownership of a pointer.

Best regards

Re: [PATCH 3/4] dhcp: Add l_dhcp_client_set_lease

[Denis Kenzior]

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

Hi Andrew,

>>> @@ -468,7 +469,8 @@ static int dhcp_client_send_request(struct l_dhcp_client *client)
>>>         * 'server identifier' option for any unicast requests to the DHCP
>>>         * server.
>>>         */
>>> -     if (client->state == DHCP_STATE_RENEWING)
>>> +     if (client->state == DHCP_STATE_RENEWING &&
>>> +                     client->lease->server_address)
>>
>> I think this is against the spec?
> 
> So in both cases the check should be always true if
> l_dhcp_client_set_lease was not used...  Checking an always-true
> condition isn't against the spec.

I get the feeling you're arguing for the sake of arguing here.  Strictly 
speaking you're completely right, but ... If the check is always going to be 
true, why check it? :)  Our coding style says to avoid a 'complex if body.'  And 
most likely you'll get into trouble in the future since your brain will flag it. 
  Static analysis will also flag it and will likely want you to check for this 
explicitly.  So you'd end up with a L_WARN_ON if the condition was false, or 
return -EINVAL from this operation, or something.  Which arguably would make 
more sense.

> 
>>
>>>                return dhcp_client_send_unicast(client, request, len);
>>>
>>>        return client->transport->l2_send(client->transport,
>>> @@ -504,7 +506,8 @@ static void dhcp_client_send_release(struct l_dhcp_client *client)
>>>
>>>        request->ciaddr = client->lease->address;
>>>
>>> -     if (!_dhcp_message_builder_append(&builder,
>>> +     if (client->lease->server_address &&
>>> +                     !_dhcp_message_builder_append(&builder,
>>
>> Should we even bother with this if server_address is NULL?  If we have no server
>> address to send a release to, then we shouldn't even get here.
> 
> So we can move the check to the caller.  There's also a bigger
> question of whether we should call dhcp_client_send_release() at all.
> The only current caller is l_dhcp_client_stop.  I believe most clients

What you will find is that there's almost never a time that the release actually 
has a chance to get sent out over the air.  We're usually disassociated by the 
time this fires.  Maybe we can use l_dhcp_client_stop as part of our 
disassociate logic, but we'd need to take extra care that this actually goes out...

> don't use DHCPRELEASE because they cache their leases for future use,
> so maybe it makes sense that we send it for now because we don't save
> our leases.  In the FILS and P2P cases (which can only exist with
> 802.11) the AP knows exactly when we disconnect so it can take
> whatever action is needed without the DHCPRELEASE.
> 

That is what I would expect, yes.

>>
>> I really don't think that DHCP servers will somehow work completely against the
>> relevant RFCs in order to support this FILS IP Address assignment use case.
> 
> On one hand RFC2131 says which messages should be unicast vs.
> broadcast, and which messages should contain the "Server Identifier"
> option, but it also just says:
> 
> "The DHCP client broadcasts DHCPDISCOVER, DHCPREQUEST and DHCPINFORM
> messages, unless the client knows the address of a DHCP server."
> 
> In the FILS case we don't know the IP of the server until our first
> renewal.  You're right that if we strictly follow 4.3.2 or 4.4.5, both
> of which say that the DHCPREQUEST in the RENEWING state should be
> unicast, we have no way of renewing the lease in a compliant way.  On
> the other hand, from when I looked at whether the chaddr field should
> be validated against the source address, I found most servers don't
> look at either the source or destination addresses by default.
> 
> Then again we do have the server's MAC (in IWD...), we just don't have
> its IP so we can unicast the message using transport->l2_send maybe?

I'm not following, how do we have the server's MAC?  The AP is not necessarily 
the DHCP server.

> 
>> So
>> the question is what do we do if a non-infinite validity time for an address is
>> given to us.
> 
> So there are two things that we can do that will be fully compliant
> with all specs:
> 
> * Instead of renewing the IP by going to the RENEWING state, we can go
> back to INIT and go through the full DISCOVER/OFFER/REQUEST/ACK.  We
> may be offered the same IP, and if we're offered a different one then

shouldn't we go into Init-Reboot or Rebinding instead of back to Init?

> we can still live with it.  I guess, to implement this we don't need
> my l_dhcp_client_set_lease() method, we can track the lease expiry in
> IWD and start the l_dhcp_client only after the initial lease is close
> to expiry.

Yes, that makes sense.

> 
> * Track the lease expiry time in IWD and request a new IP by sending a
> fils Robust Action Frame with the IP Address Assignment IE again.

That might be best.  Maybe that is what the spec intends?

<snip>

>>
>> I actually wonder why you'd want to clone the lease here and not just take
>> ownership of the passed in lease object?  What is the caller going to do with it
>> besides free it?
> 
> The only reason was that IIRC you didn't like APIs where a method
> takes ownership of a pointer.
> 

Depends on context, we have plenty of APIs that take ownership :)

Regards,
-Denis

Re: [PATCH 3/4] dhcp: Add l_dhcp_client_set_lease

[Andrew Zaborowski]

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

On Sat, 14 Aug 2021 at 03:30, Denis Kenzior <denkenz@gmail.com> wrote:
> >>> @@ -468,7 +469,8 @@ static int dhcp_client_send_request(struct l_dhcp_client *client)
> >>>         * 'server identifier' option for any unicast requests to the DHCP
> >>>         * server.
> >>>         */
> >>> -     if (client->state == DHCP_STATE_RENEWING)
> >>> +     if (client->state == DHCP_STATE_RENEWING &&
> >>> +                     client->lease->server_address)
> >>
> >> I think this is against the spec?
> >
> > So in both cases the check should be always true if
> > l_dhcp_client_set_lease was not used...  Checking an always-true
> > condition isn't against the spec.
>
> I get the feeling you're arguing for the sake of arguing here.  Strictly
> speaking you're completely right, but ... If the check is always going to be
> true, why check it? :)

What I meant was that it was not against the spec if
l_dhcp_client_set_lease was not used.  If l_dhcp_client_set_lease was
used then it would be against sections 4.3.2 and 4.4.5 to use
broadcast in the RENEWING state but we'd have no other option (also by
my reading unicast wasn't a strict requirement, there's even that
mention of clients that can't *receive* unicast)

>  Our coding style says to avoid a 'complex if body.'  And
> most likely you'll get into trouble in the future since your brain will flag it.
>   Static analysis will also flag it and will likely want you to check for this
> explicitly.  So you'd end up with a L_WARN_ON if the condition was false, or
> return -EINVAL from this operation, or something.  Which arguably would make
> more sense.
>
> >
> >>
> >>>                return dhcp_client_send_unicast(client, request, len);
> >>>
> >>>        return client->transport->l2_send(client->transport,
> >>> @@ -504,7 +506,8 @@ static void dhcp_client_send_release(struct l_dhcp_client *client)
> >>>
> >>>        request->ciaddr = client->lease->address;
> >>>
> >>> -     if (!_dhcp_message_builder_append(&builder,
> >>> +     if (client->lease->server_address &&
> >>> +                     !_dhcp_message_builder_append(&builder,
> >>
> >> Should we even bother with this if server_address is NULL?  If we have no server
> >> address to send a release to, then we shouldn't even get here.
> >
> > So we can move the check to the caller.  There's also a bigger
> > question of whether we should call dhcp_client_send_release() at all.
> > The only current caller is l_dhcp_client_stop.  I believe most clients
>
> What you will find is that there's almost never a time that the release actually
> has a chance to get sent out over the air.

I think I saw it being received in the debug logs of some autotest.
But if it doesn't make it to the peer that'd be more reason to not
send it.

>  We're usually disassociated by the
> time this fires.  Maybe we can use l_dhcp_client_stop as part of our
> disassociate logic, but we'd need to take extra care that this actually goes out...
>
> > don't use DHCPRELEASE because they cache their leases for future use,
> > so maybe it makes sense that we send it for now because we don't save
> > our leases.  In the FILS and P2P cases (which can only exist with
> > 802.11) the AP knows exactly when we disconnect so it can take
> > whatever action is needed without the DHCPRELEASE.
> >
>
> That is what I would expect, yes.
>
> >>
> >> I really don't think that DHCP servers will somehow work completely against the
> >> relevant RFCs in order to support this FILS IP Address assignment use case.
> >
> > On one hand RFC2131 says which messages should be unicast vs.
> > broadcast, and which messages should contain the "Server Identifier"
> > option, but it also just says:
> >
> > "The DHCP client broadcasts DHCPDISCOVER, DHCPREQUEST and DHCPINFORM
> > messages, unless the client knows the address of a DHCP server."
> >
> > In the FILS case we don't know the IP of the server until our first
> > renewal.  You're right that if we strictly follow 4.3.2 or 4.4.5, both
> > of which say that the DHCPREQUEST in the RENEWING state should be
> > unicast, we have no way of renewing the lease in a compliant way.  On
> > the other hand, from when I looked at whether the chaddr field should
> > be validated against the source address, I found most servers don't
> > look at either the source or destination addresses by default.
> >
> > Then again we do have the server's MAC (in IWD...), we just don't have
> > its IP so we can unicast the message using transport->l2_send maybe?
>
> I'm not following, how do we have the server's MAC?  The AP is not necessarily
> the DHCP server.

True, I kinda assumed that it was if the AP is able to return the IP
directly in the Association Response but even then it's not
necessarily true.

>
> >
> >> So
> >> the question is what do we do if a non-infinite validity time for an address is
> >> given to us.
> >
> > So there are two things that we can do that will be fully compliant
> > with all specs:
> >
> > * Instead of renewing the IP by going to the RENEWING state, we can go
> > back to INIT and go through the full DISCOVER/OFFER/REQUEST/ACK.  We
> > may be offered the same IP, and if we're offered a different one then
>
> shouldn't we go into Init-Reboot or Rebinding instead of back to Init?

We could do that, we'll probably never know what is actually
"correct".  Since we already have an IP it's not as important as
during initial setup for speed.

>
> > we can still live with it.  I guess, to implement this we don't need
> > my l_dhcp_client_set_lease() method, we can track the lease expiry in
> > IWD and start the l_dhcp_client only after the initial lease is close
> > to expiry.
>
> Yes, that makes sense.
>
> >
> > * Track the lease expiry time in IWD and request a new IP by sending a
> > fils Robust Action Frame with the IP Address Assignment IE again.
>
> That might be best.  Maybe that is what the spec intends?

This is what it intends but it would be nice to reuse the DHCP client
for the lease renewals.  We can also do it in IWD, it's just that it'd
be hard to stuff into IWD's netconfig.c since it would have to start
sending and receiving action frames, so most of it might end up in
station.c.

>
> <snip>
>
> >>
> >> I actually wonder why you'd want to clone the lease here and not just take
> >> ownership of the passed in lease object?  What is the caller going to do with it
> >> besides free it?
> >
> > The only reason was that IIRC you didn't like APIs where a method
> > takes ownership of a pointer.
> >
>
> Depends on context, we have plenty of APIs that take ownership :)

I've probably used that argument and was told those APIs were a mistake. ;)

Best regards

Re: [PATCH 3/4] dhcp: Add l_dhcp_client_set_lease

[Denis Kenzior]

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

Hi Andrew,

> used then it would be against sections 4.3.2 and 4.4.5 to use
> broadcast in the RENEWING state but we'd have no other option (also by

Also 4.3.6.  I interpret section 4.4.4 to be more general.  So when it talks 
about the client 'knowing the address' it means DHCP Request in the RENEWING 
state.  The state transitions in Section 5 make it pretty clear that we can't 
end up in the Renewing state unless we know the server address.

> my reading unicast wasn't a strict requirement, there's even that
> mention of clients that can't *receive* unicast)

I disagree here.  Renewing state by definition means the client has an address 
assigned and active (the only transition is from the Bound state)  Otherwise the 
client should be in a different state entirely.  You also have Section 4.4.3 
that talks about the possibility of an address being assigned by means other 
than DHCP.  I would think 4.4.3 applies in the FILS use case?

>> What you will find is that there's almost never a time that the release actually
>> has a chance to get sent out over the air.
> 
> I think I saw it being received in the debug logs of some autotest.
> But if it doesn't make it to the peer that'd be more reason to not
> send it.

We could be racing between the genl socket and the data socket too.  It probably 
doesn't really matter at all whether we manage to send a release or not.

>>> * Track the lease expiry time in IWD and request a new IP by sending a
>>> fils Robust Action Frame with the IP Address Assignment IE again.
>>
>> That might be best.  Maybe that is what the spec intends?
> 
> This is what it intends but it would be nice to reuse the DHCP client

Nice, yes.  But it isn't looking like it is in the cards...

> for the lease renewals.  We can also do it in IWD, it's just that it'd
> be hard to stuff into IWD's netconfig.c since it would have to start
> sending and receiving action frames, so most of it might end up in
> station.c.

Agreed.  We'll have to see how we can make this work nicely.

>> Depends on context, we have plenty of APIs that take ownership :)
> 
> I've probably used that argument and was told those APIs were a mistake. ;)
> 

I don't know about that.  I mean look at all the _send() APIs.  They take 
ownership.  I'll admit that this is not something we do (or have a need to do) 
often.

Regards,
-Denis