* [PATCH 1/4] dhcp-server: Fix lease expiry time calculation
@ 2021-06-01 2:03 Andrew Zaborowski
2021-06-01 2:03 ` [PATCH 2/4] dhcp-server: Expire the lease from the right list Andrew Zaborowski
` (3 more replies)
0 siblings, 4 replies; 6+ messages in thread
From: Andrew Zaborowski @ 2021-06-01 2:03 UTC (permalink / raw)
To: ell
[-- Attachment #1: Type: text/plain, Size: 1397 bytes --]
We were comparing the l_time_now() (usecs) with lease->lifetime (secs)
and then converting the result from usecs to secs, so the "diff" and the
"to_sec" operations should be switched around. While there also replace
the "diff" operation so that if lease->lifetime is already in the past
(e.g. because processing took to long), we schedule the timeout in 0
seconds instead of the absolute value of the difference.
Since we're using uint32_t for the timestamps, use the overflow-safe(r)
comparison.
---
ell/dhcp-server.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/ell/dhcp-server.c b/ell/dhcp-server.c
index d390856..9e39fe6 100644
--- a/ell/dhcp-server.c
+++ b/ell/dhcp-server.c
@@ -192,6 +192,7 @@ static void set_next_expire_timer(struct l_dhcp_server *server,
{
struct l_dhcp_lease *next;
unsigned int next_timeout;
+ uint32_t now;
/*
* If this is an expiring lease put it into the expired queue, removing
@@ -213,8 +214,9 @@ static void set_next_expire_timer(struct l_dhcp_server *server,
return;
}
- next_timeout = l_time_to_secs(l_time_diff(l_time_now(),
- next->lifetime));
+ now = l_time_to_secs(l_time_now());
+ next_timeout = (int32_t) (next->lifetime - now) > 0 ?
+ next->lifetime - now : 0;
if (server->next_expire)
l_timeout_modify(server->next_expire, next_timeout);
--
2.30.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/4] dhcp-server: Expire the lease from the right list
2021-06-01 2:03 [PATCH 1/4] dhcp-server: Fix lease expiry time calculation Andrew Zaborowski
@ 2021-06-01 2:03 ` Andrew Zaborowski
2021-06-01 14:44 ` Denis Kenzior
2021-06-01 2:03 ` [PATCH 3/4] dhcp-server: Don't check if lease expired in lease_release Andrew Zaborowski
` (2 subsequent siblings)
3 siblings, 1 reply; 6+ messages in thread
From: Andrew Zaborowski @ 2021-06-01 2:03 UTC (permalink / raw)
To: ell
[-- Attachment #1: Type: text/plain, Size: 913 bytes --]
We were trying to expire the oldest lease from server->expired_list but
we should be expiring the oldest lease in server->lease_list. We were
never adding any new entries to server->expired_list and it would be
always empty.
---
ell/dhcp-server.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/ell/dhcp-server.c b/ell/dhcp-server.c
index 9e39fe6..9226ba4 100644
--- a/ell/dhcp-server.c
+++ b/ell/dhcp-server.c
@@ -229,7 +229,7 @@ static void set_next_expire_timer(struct l_dhcp_server *server,
static void lease_expired_cb(struct l_timeout *timeout, void *user_data)
{
struct l_dhcp_server *server = user_data;
- struct l_dhcp_lease *lease = l_queue_peek_tail(server->expired_list);
+ struct l_dhcp_lease *lease = l_queue_peek_tail(server->lease_list);
if (server->event_handler)
server->event_handler(server, L_DHCP_SERVER_EVENT_LEASE_EXPIRED,
--
2.30.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 3/4] dhcp-server: Don't check if lease expired in lease_release
2021-06-01 2:03 [PATCH 1/4] dhcp-server: Fix lease expiry time calculation Andrew Zaborowski
2021-06-01 2:03 ` [PATCH 2/4] dhcp-server: Expire the lease from the right list Andrew Zaborowski
@ 2021-06-01 2:03 ` Andrew Zaborowski
2021-06-01 2:03 ` [PATCH 4/4] dhcp-server: Validate the offering state when handling message Andrew Zaborowski
2021-06-01 14:38 ` [PATCH 1/4] dhcp-server: Fix lease expiry time calculation Denis Kenzior
3 siblings, 0 replies; 6+ messages in thread
From: Andrew Zaborowski @ 2021-06-01 2:03 UTC (permalink / raw)
To: ell
[-- Attachment #1: Type: text/plain, Size: 1181 bytes --]
When lease_release() is called by listener_event(), the lease must have
been found in server->lease_list meaning that its expiry timeout hasn't
happened yet. Even if lease->lifetime happens to have just expired, we
probably want to emit the LEASE_EXPIRED notification immediately and
move the lease from least_list and expired_list, rather than wait for
the timeout.
---
ell/dhcp-server.c | 8 --------
1 file changed, 8 deletions(-)
diff --git a/ell/dhcp-server.c b/ell/dhcp-server.c
index 9226ba4..217844b 100644
--- a/ell/dhcp-server.c
+++ b/ell/dhcp-server.c
@@ -287,14 +287,6 @@ static struct l_dhcp_lease *add_lease(struct l_dhcp_server *server,
static void lease_release(struct l_dhcp_server *server,
struct l_dhcp_lease *lease)
{
- /*
- * If the client released the lease after the server timeout expired
- * there is nothing to do. Otherwise the client is releasing the
- * lease early which may require re-setting the lease expire timer
- */
- if (is_expired_lease(lease))
- return;
-
if (server->event_handler)
server->event_handler(server, L_DHCP_SERVER_EVENT_LEASE_EXPIRED,
server->user_data, lease);
--
2.30.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 4/4] dhcp-server: Validate the offering state when handling message
2021-06-01 2:03 [PATCH 1/4] dhcp-server: Fix lease expiry time calculation Andrew Zaborowski
2021-06-01 2:03 ` [PATCH 2/4] dhcp-server: Expire the lease from the right list Andrew Zaborowski
2021-06-01 2:03 ` [PATCH 3/4] dhcp-server: Don't check if lease expired in lease_release Andrew Zaborowski
@ 2021-06-01 2:03 ` Andrew Zaborowski
2021-06-01 14:38 ` [PATCH 1/4] dhcp-server: Fix lease expiry time calculation Denis Kenzior
3 siblings, 0 replies; 6+ messages in thread
From: Andrew Zaborowski @ 2021-06-01 2:03 UTC (permalink / raw)
To: ell
[-- Attachment #1: Type: text/plain, Size: 1859 bytes --]
Make extra sure we don't call remove_lease() on a lease that is not in
the "offering" state because that would remove the lease without
respecting the lease_list/expire_lease semantics or emitting the
LEASE_EXPIRED event. Similarly make sure we don't call lease_expire()
on a lease that is in the "offering" state because that would move it
directly to expire_lease, emit a false LEASE_EXPIRED event and update
the expiry timeout wrongly.
---
I wonder if instead we should insert the leases being offered into
server->lease_list same as the active leases (i.e. according to their
leas->lifetime), and call set_next_expire_timer() so that their lifetimes
are tracked and so that they're also expired. We'd only need to check
the lease->offered flag before emitting the LEASE_EXPIRED events.
As it is now, I think a lease that is offered never times out if the
client never replies to the offer, the 5-minute lifetime is ignored.
ell/dhcp-server.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/ell/dhcp-server.c b/ell/dhcp-server.c
index 217844b..f4d08d6 100644
--- a/ell/dhcp-server.c
+++ b/ell/dhcp-server.c
@@ -646,7 +646,8 @@ static void listener_event(const void *data, size_t len, void *user_data)
case DHCP_MESSAGE_TYPE_DECLINE:
SERVER_DEBUG("Received DECLINE");
- if (!server_id_opt || !requested_ip_opt || !lease)
+ if (!server_id_opt || !requested_ip_opt || !lease ||
+ !lease->offering)
break;
if (requested_ip_opt == lease->address)
@@ -656,7 +657,7 @@ static void listener_event(const void *data, size_t len, void *user_data)
case DHCP_MESSAGE_TYPE_RELEASE:
SERVER_DEBUG("Received RELEASE");
- if (!server_id_opt || !lease)
+ if (!server_id_opt || !lease || lease->offering)
break;
if (message->ciaddr == lease->address)
--
2.30.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/4] dhcp-server: Fix lease expiry time calculation
2021-06-01 2:03 [PATCH 1/4] dhcp-server: Fix lease expiry time calculation Andrew Zaborowski
` (2 preceding siblings ...)
2021-06-01 2:03 ` [PATCH 4/4] dhcp-server: Validate the offering state when handling message Andrew Zaborowski
@ 2021-06-01 14:38 ` Denis Kenzior
3 siblings, 0 replies; 6+ messages in thread
From: Denis Kenzior @ 2021-06-01 14:38 UTC (permalink / raw)
To: ell
[-- Attachment #1: Type: text/plain, Size: 1675 bytes --]
Hi Andrew,
On 5/31/21 9:03 PM, Andrew Zaborowski wrote:
> We were comparing the l_time_now() (usecs) with lease->lifetime (secs)
> and then converting the result from usecs to secs, so the "diff" and the
> "to_sec" operations should be switched around. While there also replace
> the "diff" operation so that if lease->lifetime is already in the past
> (e.g. because processing took to long), we schedule the timeout in 0
> seconds instead of the absolute value of the difference.
>
> Since we're using uint32_t for the timestamps, use the overflow-safe(r)
> comparison.
> ---
> ell/dhcp-server.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/ell/dhcp-server.c b/ell/dhcp-server.c
> index d390856..9e39fe6 100644
> --- a/ell/dhcp-server.c
> +++ b/ell/dhcp-server.c
> @@ -192,6 +192,7 @@ static void set_next_expire_timer(struct l_dhcp_server *server,
> {
> struct l_dhcp_lease *next;
> unsigned int next_timeout;
> + uint32_t now;
>
> /*
> * If this is an expiring lease put it into the expired queue, removing
> @@ -213,8 +214,9 @@ static void set_next_expire_timer(struct l_dhcp_server *server,
> return;
> }
>
> - next_timeout = l_time_to_secs(l_time_diff(l_time_now(),
> - next->lifetime));
> + now = l_time_to_secs(l_time_now());
> + next_timeout = (int32_t) (next->lifetime - now) > 0 ?
> + next->lifetime - now : 0;
0 is not a valid timeout value, it is used to rearm the timer. So this case
probably needs a different approach.
>
> if (server->next_expire)
> l_timeout_modify(server->next_expire, next_timeout);
>
Regards,
-Denis
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/4] dhcp-server: Expire the lease from the right list
2021-06-01 2:03 ` [PATCH 2/4] dhcp-server: Expire the lease from the right list Andrew Zaborowski
@ 2021-06-01 14:44 ` Denis Kenzior
0 siblings, 0 replies; 6+ messages in thread
From: Denis Kenzior @ 2021-06-01 14:44 UTC (permalink / raw)
To: ell
[-- Attachment #1: Type: text/plain, Size: 440 bytes --]
Hi Andrew,
On 5/31/21 9:03 PM, Andrew Zaborowski wrote:
> We were trying to expire the oldest lease from server->expired_list but
> we should be expiring the oldest lease in server->lease_list. We were
> never adding any new entries to server->expired_list and it would be
> always empty.
> ---
> ell/dhcp-server.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
Patch 2-4 applied, thanks.
Regards,
-Denis
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-06-01 14:44 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-01 2:03 [PATCH 1/4] dhcp-server: Fix lease expiry time calculation Andrew Zaborowski
2021-06-01 2:03 ` [PATCH 2/4] dhcp-server: Expire the lease from the right list Andrew Zaborowski
2021-06-01 14:44 ` Denis Kenzior
2021-06-01 2:03 ` [PATCH 3/4] dhcp-server: Don't check if lease expired in lease_release Andrew Zaborowski
2021-06-01 2:03 ` [PATCH 4/4] dhcp-server: Validate the offering state when handling message Andrew Zaborowski
2021-06-01 14:38 ` [PATCH 1/4] dhcp-server: Fix lease expiry time calculation Denis Kenzior
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).