[PATCH 1/5] acd: store IP in network order

[PATCH 1/5] acd: store IP in network order

[James Prestwood]

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

The IP target was being converted into host order during
l_acd_start, which required converting back to network order
many times throughout the ACD process. Instead just leave
the IP in network order and use directly. This avoids the
need for conversions and additional temporary variables.
---
 ell/acd.c | 24 +++++++++---------------
 1 file changed, 9 insertions(+), 15 deletions(-)

diff --git a/ell/acd.c b/ell/acd.c
index ef72fef..7d912d9 100644
--- a/ell/acd.c
+++ b/ell/acd.c
@@ -125,8 +125,6 @@ static int acd_send_packet(struct l_acd *acd, uint32_t source_ip)
 {
 	struct sockaddr_ll dest;
 	struct ether_arp p;
-	uint32_t ip_source;
-	uint32_t ip_target;
 	int n;
 	int fd = l_io_get_fd(acd->io);
 
@@ -138,19 +136,17 @@ static int acd_send_packet(struct l_acd *acd, uint32_t source_ip)
 	dest.sll_halen = ETH_ALEN;
 	memset(dest.sll_addr, 0xFF, ETH_ALEN);
 
-	ip_source = htonl(source_ip);
-	ip_target = htonl(acd->ip);
 	p.arp_hrd = htons(ARPHRD_ETHER);
 	p.arp_pro = htons(ETHERTYPE_IP);
 	p.arp_hln = ETH_ALEN;
 	p.arp_pln = 4;
 	p.arp_op = htons(ARPOP_REQUEST);
 
-	ACD_DEBUG("sending packet with target IP %s", IP_STR(ip_target));
+	ACD_DEBUG("sending packet with target IP %s", IP_STR(acd->ip));
 
 	memcpy(&p.arp_sha, acd->mac, ETH_ALEN);
-	memcpy(&p.arp_spa, &ip_source, sizeof(p.arp_spa));
-	memcpy(&p.arp_tpa, &ip_target, sizeof(p.arp_tpa));
+	memcpy(&p.arp_spa, &source_ip, sizeof(p.arp_spa));
+	memcpy(&p.arp_tpa, &acd->ip, sizeof(p.arp_tpa));
 
 	n = sendto(fd, &p, sizeof(p), 0,
 			(struct sockaddr*) &dest, sizeof(dest));
@@ -178,7 +174,7 @@ static void announce_wait_timeout(struct l_timeout *timeout, void *user_data)
 
 	if (acd->state == ACD_STATE_PROBE) {
 		ACD_DEBUG("No conflicts found for %s, announcing address",
-				IP_STR(htonl(acd->ip)));
+				IP_STR(acd->ip));
 
 		acd->state = ACD_STATE_ANNOUNCED;
 
@@ -275,7 +271,6 @@ static bool acd_read_handler(struct l_io *io, void *user_data)
 	int source_conflict;
 	int target_conflict;
 	bool probe;
-	uint32_t ip;
 
 	memset(&arp, 0, sizeof(arp));
 	len = read(l_io_get_fd(acd->io), &arp, sizeof(arp));
@@ -292,15 +287,14 @@ static bool acd_read_handler(struct l_io *io, void *user_data)
 	if (memcmp(arp.arp_sha, acd->mac, ETH_ALEN) == 0)
 		return true;
 
-	ip = htonl(acd->ip);
-	source_conflict = !memcmp(arp.arp_spa, &ip, sizeof(uint32_t));
+	source_conflict = !memcmp(arp.arp_spa, &acd->ip, sizeof(uint32_t));
 	probe = l_memeqzero(arp.arp_spa, sizeof(uint32_t));
 	target_conflict = probe &&
-		!memcmp(arp.arp_tpa, &ip, sizeof(uint32_t));
+		!memcmp(arp.arp_tpa, &acd->ip, sizeof(uint32_t));
 
 	if (!source_conflict && !target_conflict) {
 		ACD_DEBUG("No target or source conflict detected for %s",
-				IP_STR(ip));
+				IP_STR(acd->ip));
 		return true;
 	}
 
@@ -309,7 +303,7 @@ static bool acd_read_handler(struct l_io *io, void *user_data)
 		/* No reason to continue probing */
 		ACD_DEBUG("%s conflict detected for %s",
 				target_conflict ? "Target" : "Source",
-				IP_STR(ip));
+				IP_STR(acd->ip));
 
 		if (acd->event_func)
 			acd->event_func(L_ACD_EVENT_CONFLICT, acd->user_data);
@@ -454,7 +448,7 @@ LIB_EXPORT bool l_acd_start(struct l_acd *acd, const char *ip)
 	l_io_set_close_on_destroy(acd->io, true);
 	l_io_set_read_handler(acd->io, acd_read_handler, acd, NULL);
 
-	acd->ip = ntohl(ia.s_addr);
+	acd->ip = ia.s_addr;
 
 	/*
 	 * Optimization to allows skipping the probe stage. The RFC does not
-- 
2.26.2

[PATCH 2/5] acd: remove acd_random_delay_ms

[James Prestwood]

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

time.c privately exposes a similar function
---
 ell/acd.c | 15 +++------------
 1 file changed, 3 insertions(+), 12 deletions(-)

diff --git a/ell/acd.c b/ell/acd.c
index 7d912d9..ae6f7b1 100644
--- a/ell/acd.c
+++ b/ell/acd.c
@@ -36,6 +36,7 @@
 #include "net.h"
 #include "timeout.h"
 #include "random.h"
+#include "time-private.h"
 
 /* IPv4 Address Conflict Detection (RFC 5227) */
 #define PROBE_WAIT		1
@@ -156,15 +157,6 @@ static int acd_send_packet(struct l_acd *acd, uint32_t source_ip)
 	return n;
 }
 
-static uint32_t acd_random_delay_ms(uint32_t max_sec)
-{
-	uint32_t rand;
-
-	l_getrandom(&rand, sizeof(rand));
-
-	return rand % (max_sec * 1000);
-}
-
 static void announce_wait_timeout(struct l_timeout *timeout, void *user_data)
 {
 	struct l_acd *acd = user_data;
@@ -233,8 +225,7 @@ static void probe_wait_timeout(struct l_timeout *timeout, void *user_data)
 		 * these probe packets spaced randomly and uniformly, PROBE_MIN
 		 * to PROBE_MAX seconds apart."
 		 */
-		delay = acd_random_delay_ms(PROBE_MAX - PROBE_MIN);
-		delay += PROBE_MIN * 1000;
+		delay = _time_pick_interval_secs(PROBE_MIN, PROBE_MAX);
 		acd->timeout = l_timeout_create_ms(delay, probe_wait_timeout,
 							acd, NULL);
 	} else {
@@ -468,7 +459,7 @@ LIB_EXPORT bool l_acd_start(struct l_acd *acd, const char *ip)
 	} else
 		acd->state = ACD_STATE_PROBE;
 
-	delay = acd_random_delay_ms(PROBE_WAIT);
+	delay = _time_pick_interval_secs(0, PROBE_WAIT);
 
 	ACD_DEBUG("Waiting %ums to send probe", delay);
 
-- 
2.26.2

[PATCH 3/5] acd: optimize to use l_timeout_modify

[James Prestwood]

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

Rather than destroy and create a new timeout the existing
timeout can be modified in cases where the same callback
is used.
---
 ell/acd.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/ell/acd.c b/ell/acd.c
index ae6f7b1..a50b26e 100644
--- a/ell/acd.c
+++ b/ell/acd.c
@@ -161,9 +161,6 @@ static void announce_wait_timeout(struct l_timeout *timeout, void *user_data)
 {
 	struct l_acd *acd = user_data;
 
-	l_timeout_remove(acd->timeout);
-	acd->timeout = NULL;
-
 	if (acd->state == ACD_STATE_PROBE) {
 		ACD_DEBUG("No conflicts found for %s, announcing address",
 				IP_STR(acd->ip));
@@ -190,13 +187,14 @@ static void announce_wait_timeout(struct l_timeout *timeout, void *user_data)
 			return;
 		}
 
-		acd->timeout = l_timeout_create(ANNOUNCE_INTERVAL,
-						announce_wait_timeout,
-						acd, NULL);
+		l_timeout_modify(acd->timeout, ANNOUNCE_INTERVAL);
 
 		return;
 	}
 
+	l_timeout_remove(acd->timeout);
+	acd->timeout = NULL;
+
 	ACD_DEBUG("Done announcing");
 }
 
@@ -207,9 +205,6 @@ static void probe_wait_timeout(struct l_timeout *timeout, void *user_data)
 
 	ACD_DEBUG("Sending ACD Probe");
 
-	l_timeout_remove(acd->timeout);
-	acd->timeout = NULL;
-
 	if (acd_send_packet(acd, 0) < 0) {
 		ACD_DEBUG("Failed to send ACD probe");
 		return;
@@ -226,8 +221,7 @@ static void probe_wait_timeout(struct l_timeout *timeout, void *user_data)
 		 * to PROBE_MAX seconds apart."
 		 */
 		delay = _time_pick_interval_secs(PROBE_MIN, PROBE_MAX);
-		acd->timeout = l_timeout_create_ms(delay, probe_wait_timeout,
-							acd, NULL);
+		l_timeout_modify_ms(acd->timeout, delay);
 	} else {
 		/*
 		 * Wait for ANNOUNCE_WAIT seconds after probe period before
@@ -235,6 +229,9 @@ static void probe_wait_timeout(struct l_timeout *timeout, void *user_data)
 		 */
 		ACD_DEBUG("Done probing");
 
+		l_timeout_remove(acd->timeout);
+		acd->timeout = NULL;
+
 		acd->retries = 1;
 
 		acd->timeout = l_timeout_create(ANNOUNCE_WAIT,
-- 
2.26.2

[PATCH 4/5] acd: fix valgrind warning of uninitialized buffer

[James Prestwood]

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

---
 ell/acd.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/ell/acd.c b/ell/acd.c
index a50b26e..e80e322 100644
--- a/ell/acd.c
+++ b/ell/acd.c
@@ -130,6 +130,7 @@ static int acd_send_packet(struct l_acd *acd, uint32_t source_ip)
 	int fd = l_io_get_fd(acd->io);
 
 	memset(&dest, 0, sizeof(dest));
+	memset(&p, 0, sizeof(p));
 
 	dest.sll_family = AF_PACKET;
 	dest.sll_protocol = htons(ETH_P_ARP);
-- 
2.26.2

[PATCH 5/5] acd: stop ACD if l_acd_destroy is called

[James Prestwood]

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

---
 ell/acd.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/ell/acd.c b/ell/acd.c
index e80e322..6381747 100644
--- a/ell/acd.c
+++ b/ell/acd.c
@@ -511,6 +511,8 @@ LIB_EXPORT void l_acd_destroy(struct l_acd *acd)
 	if (unlikely(!acd))
 		return;
 
+	l_acd_stop(acd);
+
 	if (acd->destroy)
 		acd->destroy(acd->user_data);
 
-- 
2.26.2

Re: [PATCH 1/5] acd: store IP in network order

[Denis Kenzior]

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

Hi James,

On 12/8/20 12:42 PM, James Prestwood wrote:
> The IP target was being converted into host order during
> l_acd_start, which required converting back to network order
> many times throughout the ACD process. Instead just leave
> the IP in network order and use directly. This avoids the
> need for conversions and additional temporary variables.
> ---
>   ell/acd.c | 24 +++++++++---------------
>   1 file changed, 9 insertions(+), 15 deletions(-)

All applied, thanks.

Regards,
-Denis