From 20b9c8985542d1877afa5e6e5a22931b81879935 Mon Sep 17 00:00:00 2001
From: huitema <huitema@huitema.net>
Date: Sun, 9 Dec 2018 23:03:20 -0800
Subject: [PATCH] fix series of embarassing migration bugs
---
picoquic/frames.c | 4 ++--
picoquic/picoquic.h | 1 +
picoquic/quicctx.c | 2 +-
picoquic/sender.c | 27 +++++++++++++++------------
picoquicfirst/picoquicdemo.c | 1 -
5 files changed, 19 insertions(+), 16 deletions(-)
diff --git a/picoquic/frames.c b/picoquic/frames.c
index 11a22732..e1184583 100644
--- a/picoquic/frames.c
+++ b/picoquic/frames.c
@@ -1646,8 +1646,7 @@ static int picoquic_process_ack_range(
picoquic_packet_t* next = p->next_packet;
picoquic_path_t * old_path = p->send_path;
- /* TODO: understand why we need this check, instead of just a check for NULL */
- if (old_path == cnx->path[0]) {
+ if (old_path != NULL) {
if (cnx->congestion_alg != NULL) {
cnx->congestion_alg->alg_notify(old_path,
picoquic_congestion_notification_acknowledgement,
@@ -1661,6 +1660,7 @@ static int picoquic_process_ack_range(
old_path->mtu_probe_sent = 0;
}
}
+
/* If the packet contained an ACK frame, perform the ACK of ACK pruning logic */
picoquic_process_possible_ack_of_ack_frame(cnx, p);
diff --git a/picoquic/picoquic.h b/picoquic/picoquic.h
index 7f33a1a8..714a435d 100644
--- a/picoquic/picoquic.h
+++ b/picoquic/picoquic.h
@@ -208,6 +208,7 @@ typedef struct st_picoquic_packet_t {
unsigned int is_evaluated : 1;
unsigned int is_pure_ack : 1;
unsigned int contains_crypto : 1;
+ unsigned int is_mtu_probe : 1;
uint8_t bytes[PICOQUIC_MAX_PACKET_SIZE];
} picoquic_packet_t;
diff --git a/picoquic/quicctx.c b/picoquic/quicctx.c
index 0ba74a5e..b000d83e 100644
--- a/picoquic/quicctx.c
+++ b/picoquic/quicctx.c
@@ -828,7 +828,7 @@ void picoquic_delete_path(picoquic_cnx_t* cnx, int path_index)
/* Remove old path data from retransmit queue */
for (picoquic_packet_context_enum pc = 0; pc < picoquic_nb_packet_context; pc++)
{
- p = cnx->pkt_ctx[pc].retransmit_oldest;
+ p = cnx->pkt_ctx[pc].retransmit_newest;
while (p != NULL) {
if (p->send_path == path_x) {
diff --git a/picoquic/sender.c b/picoquic/sender.c
index 366c8d9b..397e1205 100644
--- a/picoquic/sender.c
+++ b/picoquic/sender.c
@@ -840,10 +840,11 @@ int picoquic_retransmit_needed(picoquic_cnx_t* cnx,
/* TODO: while packets are pure ACK, drop them from retransmit queue */
while (p != NULL) {
+ picoquic_path_t * old_path = p->send_path; /* should be the path on which the packet was transmitted */
int should_retransmit = 0;
int timer_based_retransmit = 0;
uint64_t lost_packet_number = p->sequence_number;
- picoquic_packet_t* p_next = p->next_packet;
+ picoquic_packet_t* p_next = p->previous_packet;
uint8_t * new_bytes = packet->bytes;
int ret = 0;
@@ -872,11 +873,11 @@ int picoquic_retransmit_needed(picoquic_cnx_t* cnx,
size_t frame_length = 0;
size_t byte_index = 0; /* Used when parsing the old packet */
size_t checksum_length = 0;
- /* should be the path on which the packet was transmitted */
- picoquic_path_t * old_path = p->send_path;
- /* we'll report it where it got lost */
- if (old_path) old_path->retrans_count++;
+ /* we'll report it where it got lost */
+ if (old_path) {
+ old_path->retrans_count++;
+ }
*header_length = 0;
@@ -934,10 +935,12 @@ int picoquic_retransmit_needed(picoquic_cnx_t* cnx,
*is_cleartext_mode = 1;
}
- if (old_path != NULL && (p->length + p->checksum_overhead) > old_path->send_mtu) {
- /* MTU probe was lost, presumably because of packet too big */
- old_path->mtu_probe_sent = 0;
- old_path->send_mtu_max_tried = (uint32_t)(p->length + p->checksum_overhead);
+ if (packet->is_mtu_probe) {
+ if (old_path != NULL) {
+ /* MTU probe was lost, presumably because of packet too big */
+ old_path->mtu_probe_sent = 0;
+ old_path->send_mtu_max_tried = (uint32_t)(p->length + p->checksum_overhead);
+ }
/* MTU probes should not be retransmitted */
packet_is_pure_ack = 1;
do_not_detect_spurious = 0;
@@ -1017,12 +1020,11 @@ int picoquic_retransmit_needed(picoquic_cnx_t* cnx,
packet->length = length;
cnx->nb_retransmission_total++;
- /* TODO: understand why we need to check for exact path value, instead of NULL */
- if (cnx->congestion_alg != NULL && old_path == cnx->path[0]) {
+ if (cnx->congestion_alg != NULL && old_path != NULL) {
cnx->congestion_alg->alg_notify(old_path,
(timer_based_retransmit == 0) ? picoquic_congestion_notification_repeat : picoquic_congestion_notification_timeout,
0, 0, lost_packet_number, current_time);
- }
+ }
break;
}
@@ -2357,6 +2359,7 @@ int picoquic_prepare_packet_ready(picoquic_cnx_t* cnx, picoquic_path_t * path_x,
length = picoquic_prepare_mtu_probe(cnx, path_x, header_length, checksum_overhead, bytes);
packet->length = length;
packet->send_path = path_x;
+ packet->is_mtu_probe = 1;
path_x->mtu_probe_sent = 1;
is_pure_ack = 0;
}
diff --git a/picoquicfirst/picoquicdemo.c b/picoquicfirst/picoquicdemo.c
index c44037f2..b3df8a06 100644
--- a/picoquicfirst/picoquicdemo.c
+++ b/picoquicfirst/picoquicdemo.c
@@ -905,7 +905,6 @@ int quic_client_migrate(picoquic_cnx_t * cnx, SOCKET_TYPE * fd, struct sockaddr
{
fprintf(F_log, "Switch to new port, sending probe.\n");
}
- cnx->path[0]->path_is_demoted = 1;
}
}
}
--
GitLab