conncheck: rework the stun requests ordering per timer tick

With this patch, we merge the two variables stun_sent and
keep_timer_going. The three functions that are a possible source of a
new stun request returns a boolean value stating if a request has been
sent.  The semantic of keep_timer_going can now be deduced from
stun_sent and from the result of priv_conn_check_stream_nominate().

The trick that makes this merge possible is to repurpose the return
value of priv_conn_check_tick_stream(), because keep_timer_going set
when the conncheck list contains in-progress pairs in this function is
redundant with the same check later in function
priv_conn_check_tick_stream_nominate().

diff --git a/agent/agent-priv.h b/agent/agent-priv.h
index 6132a8d..eeb90dc 100644 (file)
--- a/agent/agent-priv.h
+++ b/agent/agent-priv.h
@@ -185,7 +185,6 @@ struct _NiceAgent
   guint conncheck_ongoing_idle_delay; /* ongoing delay before timer stop */
   gboolean controlling_mode;          /* controlling mode used by the
                                          conncheck */
-  gboolean stun_sent;                 /* a stun request has been */
   /* XXX: add pointer to internal data struct for ABI-safe extensions */
 };
 

diff --git a/agent/conncheck.c b/agent/conncheck.c
index 7090fd5..e726d7a 100644 (file)
--- a/agent/conncheck.c
+++ b/agent/conncheck.c
@@ -656,14 +656,13 @@ candidate_check_pair_fail (NiceStream *stream, NiceAgent *agent, CandidateCheckP
  * Helper function for connectivity check timer callback that
  * runs through the stream specific part of the state machine. 
  *
- * @param schedule if TRUE, schedule a new check
- *
- * @return will return FALSE when no more pending timers.
+ * @param agent context pointer
+ * @param stream which stream (of the agent)
+ * @return will return TRUE if a new stun request has been sent
  */
 static gboolean
 priv_conn_check_tick_stream (NiceAgent *agent, NiceStream *stream)
 {
-  gboolean keep_timer_going = FALSE;
   gboolean pair_failed = FALSE;
   GSList *i, *j;
   unsigned int timeout;
@@ -722,8 +721,6 @@ timer_return_timeout:
                 stun_message_length (&stun->message),
                 (gchar *)stun->buffer);
 
-            agent->stun_sent = TRUE;
-
             /* note: convert from milli to microseconds for g_time_val_add() */
             stun->next_tick = now + timeout * 1000;
 
@@ -742,9 +739,7 @@ timer_return_timeout:
       index++;
     }
 
-    if (remaining > 0)
-      keep_timer_going = TRUE;
-    else {
+    if (remaining == 0) {
       nice_address_to_string (&p->local->addr, tmpbuf1);
       nice_address_to_string (&p->remote->addr, tmpbuf2);
       nice_debug ("Agent %p : Retransmissions failed, giving up on pair %p",
@@ -764,17 +759,18 @@ timer_return_timeout:
       conn_check_update_check_list_state_for_ready (agent, stream, component);
     }
   }
+
   if (pair_failed)
     priv_print_conn_check_lists (agent, G_STRFUNC, ", retransmission failed");
 
-  return keep_timer_going;
+  return FALSE;
 }
 
 static gboolean
 priv_conn_check_ordinary_check (NiceAgent *agent, NiceStream *stream)
 {
   CandidateCheckPair *pair;
-  gboolean keep_timer_going = FALSE;
+  gboolean stun_sent = FALSE;
 
   /* step: perform an ordinary check, sec 6.1.4.2 point 3. (Performing
    * Connectivity Checks) of ICE spec (RFC8445)
@@ -792,19 +788,18 @@ priv_conn_check_ordinary_check (NiceAgent *agent, NiceStream *stream)
   }
 
   if (pair) {
-    keep_timer_going = priv_conn_check_initiate (agent, pair);
+    stun_sent = priv_conn_check_initiate (agent, pair);
     priv_print_conn_check_lists (agent, G_STRFUNC,
         ", initiated an ordinary connection check");
   }
-
-  return keep_timer_going;
+  return stun_sent;
 }
 
 static gboolean
 priv_conn_check_triggered_check (NiceAgent *agent, NiceStream *stream)
 {
   CandidateCheckPair *pair;
-  gboolean keep_timer_going = FALSE;
+  gboolean stun_sent = FALSE;
 
   /* step: perform a test from the triggered checks list,
    * sect 6.1.4.2 point 1. (Performing Connectivity Checks) of ICE
@@ -813,13 +808,13 @@ priv_conn_check_triggered_check (NiceAgent *agent, NiceStream *stream)
   pair = priv_get_pair_from_triggered_check_queue (agent);
 
   if (pair) {
-    if (conn_check_send (agent, pair))
-      SET_PAIR_STATE (agent, pair, NICE_CHECK_FAILED);
-    keep_timer_going = TRUE;
+    /* A pair in triggered check list must be in state in-progress */
+    g_assert_cmpint (pair->state, ==, NICE_CHECK_IN_PROGRESS);
+    stun_sent = priv_conn_check_initiate (agent, pair);
     priv_print_conn_check_lists (agent, G_STRFUNC,
         ", initiated a connection check from triggered check list");
   }
-  return keep_timer_going;
+  return stun_sent;
 }
 
 
@@ -1183,37 +1178,45 @@ static gboolean priv_conn_check_tick_agent_locked (NiceAgent *agent,
     gpointer user_data)
 {
   gboolean keep_timer_going = FALSE;
+  gboolean stun_sent = FALSE;
   GSList *i, *j;
 
-  /* A single stun request per timer callback, to respect stun pacing */
-  agent->stun_sent = FALSE;
-
   /* step: process triggered checks
    * these steps are ordered by priority, since a single stun request
    * is sent per callback, we process the important steps first.
+   *
+   * perform a single stun request per timer callback,
+   * to respect stun pacing
    */
-  for (i = agent->streams; i && !agent->stun_sent; i = i->next) {
+  for (i = agent->streams; i && !stun_sent; i = i->next) {
     NiceStream *stream = i->data;
 
-    if (priv_conn_check_triggered_check (agent, stream))
-      keep_timer_going = TRUE;
+    stun_sent = priv_conn_check_triggered_check (agent, stream);
   }
 
   /* step: process ongoing STUN transactions */
-  for (i = agent->streams; i && !agent->stun_sent; i = i->next) {
+  for (i = agent->streams; i && !stun_sent; i = i->next) {
     NiceStream *stream = i->data;
 
-    if (priv_conn_check_tick_stream (agent, stream))
-      keep_timer_going = TRUE;
-    if (priv_conn_check_tick_stream_nominate (agent, stream))
-      keep_timer_going = TRUE;
+    stun_sent = priv_conn_check_tick_stream (agent, stream);
   }
 
   /* step: process ordinary checks */
-  for (i = agent->streams; i && !agent->stun_sent; i = i->next) {
+  for (i = agent->streams; i && !stun_sent; i = i->next) {
+    NiceStream *stream = i->data;
+
+    stun_sent = priv_conn_check_ordinary_check (agent, stream);
+  }
+
+  if (stun_sent)
+    keep_timer_going = TRUE;
+
+  /* step: try to nominate a pair
+   */
+  for (i = agent->streams; i; i = i->next) {
     NiceStream *stream = i->data;
 
-    if (priv_conn_check_ordinary_check (agent, stream))
+    if (priv_conn_check_tick_stream_nominate (agent, stream))
       keep_timer_going = TRUE;
   }
 
@@ -2990,8 +2993,6 @@ int conn_check_send (NiceAgent *agent, CandidateCheckPair *pair)
     ms_ice2_legacy_conncheck_send (&stun->message, pair->sockptr,
         &pair->remote->addr);
 
-  agent->stun_sent = TRUE;
-
   return 0;
 }