Acked-By: Frank Lichtenheld <fr...@lichtenheld.com>
I'm convinced that this does what it is supposed to do.
Some typo fixes and one potential improvement to the UT noted below.
> Arne Schwabe <a...@rfc2549.org> hat am 22.04.2022 16:29 geschrieben:
> This adds an LRU cache for the last seen packets from the peer to send acks
> to all recently packets. This also packets to be acknowledged even if a single
"recent"? Or "recently received"?
"This also packets" -> "This leads to packets" ? Still not pretty but at least
somewhat readable.
> P_ACK_V1 gets lost, avoiding retransmissions. The downside is that we add up
> to 28 byte to an P_ACK_V1 (7* packet_id) and up to 24 bytes to other control
> channel packets (4* packet_id + peer session id). However these small
> increases
> in packet size are a small price to pay for increased reliability.
>
> Currently OpenVPN will only send the absolute minimum of ACK messages. A
> single
> lost ACK message will trigger a resend from the peer and another ACK message.
>
> diff --git a/src/openvpn/reliable.c b/src/openvpn/reliable.c
> index 28f99d187..3792089a9 100644
> --- a/src/openvpn/reliable.c
> +++ b/src/openvpn/reliable.c
> @@ -213,10 +213,56 @@ reliable_ack_parse(struct buffer *buf, struct
> reliable_ack *ack,
> return true;
> }
>
> +/**
> + * Copies the first n acks from \c ack to \c ack_lru
> + */
> +void
> +copy_acks_to_lru(struct reliable_ack *ack, struct reliable_ack *ack_lru, int
> n)
> +{
> + ASSERT(ack->len >= n);
> + /* This loops is backward to ensure the same order as in ack */
Either "loop" or "loops backwards"
> + for (int i = n-1; i >= 0; i--)
> + {
> + packet_id_type id = ack->packet_id[i];
> +
> + /* Handle special case of ack_lru empty */
> + if (ack_lru->len == 0)
> + {
> + ack_lru->len = 1;
> + ack_lru->packet_id[0] = id;
> + }
> +
> + bool idfound = false;
> +
> + /* Move all existing entry one to the right */
"entries"
> + packet_id_type move = id;
> +
> + for (int j = 0; j < ack_lru->len; j++)
> + {
> + packet_id_type tmp = ack_lru->packet_id[j];
> + ack_lru->packet_id[j] = move;
> + move = tmp;
> +
> + if (move == id)
> + {
> + idfound = true;
> + break;
> + }
> + }
> +
> + if (!idfound && ack_lru->len < RELIABLE_ACK_SIZE)
> + {
> + ack_lru->packet_id[ack_lru->len] = move;
> + ack_lru->len++;
> + }
> + }
> +}
> +
> /* write a packet ID acknowledgement record to buf, */
> /* removing all acknowledged entries from ack */
> bool
> reliable_ack_write(struct reliable_ack *ack,
> + struct reliable_ack *ack_lru,
> struct buffer *buf,
> const struct session_id *sid, int max, bool prepend)
> {
[...]
> diff --git a/src/openvpn/reliable.h b/src/openvpn/reliable.h
> index c80949525..e55507015 100644
> --- a/src/openvpn/reliable.h
> +++ b/src/openvpn/reliable.h
[...]
> @@ -370,6 +374,19 @@ bool reliable_ack_acknowledge_packet_id(struct
> reliable_ack *ack, packet_id_type
> */
> struct reliable_entry *reliable_get_entry_sequenced(struct reliable *rel);
>
> +
> +
> +/**
> + * Copies the first n acks from \c ack to \c ack_lru
> + *
> + * @param ack The reliable structure to copy the acks from
> + * @param ack_lru the reliable structure to insert the acks into
"The" for consistency
> + * @param n The number of ACKS to copy
> + */
> +void
> +copy_acks_to_lru(struct reliable_ack *ack, struct reliable_ack *ack_lru, int
> n);
> +
> +
> /**
> * Remove an entry from a reliable structure.
> *
[...]
> diff --git a/tests/unit_tests/openvpn/test_packet_id.c
> b/tests/unit_tests/openvpn/test_packet_id.c
> index 96d3e870d..84c6455eb 100644
> --- a/tests/unit_tests/openvpn/test_packet_id.c
> +++ b/tests/unit_tests/openvpn/test_packet_id.c
> @@ -210,6 +210,39 @@ test_get_num_output_sequenced_available(void **state)
> reliable_free(rel);
> }
>
> +
> +static void
> +test_copy_acks_to_lru(void **state)
> +{
> + struct reliable_ack ack = { .len = 4, .packet_id = {2, 1, 3, 2} };
> +
> + struct reliable_ack lru_ack = { 0 };
> +
> + /* Test copying to empty ack structure */
> + copy_acks_to_lru(&ack, &lru_ack, 4);
> + assert_int_equal(lru_ack.len, 3);
> + assert_int_equal(lru_ack.packet_id[0], 2);
> + assert_int_equal(lru_ack.packet_id[1], 1);
> + assert_int_equal(lru_ack.packet_id[2], 3);
> +
> + /* Copying again should not change the result */
> + copy_acks_to_lru(&ack, &lru_ack, 4);
> + assert_int_equal(lru_ack.len, 3);
> + assert_int_equal(lru_ack.packet_id[0], 2);
> + assert_int_equal(lru_ack.packet_id[1], 1);
> + assert_int_equal(lru_ack.packet_id[2], 3);
> +
I would suggest adding a call to copy_acks_to_lru where n is 0
and test that it leaves lru_ack unchanged.
AFAICT the current code is correct, but doesn't hurt to have a test.
> + struct reliable_ack ack2 = { .len = 7, .packet_id = {5, 6, 7, 8,
> 9,10,11} };
> +
> + /* Adding multiple acks tests if the a full array is handled correctly */
> + copy_acks_to_lru(&ack2, &lru_ack, 7);
> +
> + struct reliable_ack expected_ack = { .len = 8, .packet_id = {5, 6, 7, 8,
> 9, 10, 11,2}};
> + assert_int_equal(lru_ack.len, expected_ack.len);
> +
> + assert_memory_equal(lru_ack.packet_id, expected_ack.packet_id,
> sizeof(expected_ack.packet_id));
> +}
> +
> int
> main(void)
> {
Regards,
--
Frank Lichtenheld
_______________________________________________
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
