agent: report duplicated port in udp bsd sockets too

This patch fixes cases, where the range is full, some ports fail with
HOST_CANDIDATE_CANT_CREATE_SOCKET, other fail with
HOST_CANDIDATE_DUPLICATE_PORT, the value of res we keep when leaving the
loop is randomly the one of the last iteration of the loop.

CANT_CREATE_SOCKET still happens when trying to create an udp bsd socket
with the same address and port than one of another component, so it is
also a case of duplicate port in fact.

To be homogeneous, we add a gerror for nice_udp_bsd_socket_new(), like
we did in nice_tcp_passive_socket_new(), and we can catch the same
G_IO_ERROR_ADDRESS_IN_USE there too, when failing to get free available
udp ports.

This patch is a complement to merge request !158

diff --git a/agent/agent.c b/agent/agent.c
index d7d779e..50ea756 100644 (file)
--- a/agent/agent.c
+++ b/agent/agent.c
@@ -2699,7 +2699,7 @@ priv_add_new_candidate_discovery_turn (NiceAgent *agent,
       NiceSocket *new_socket;
       nice_address_set_port (&addr, 0);
 
-      new_socket = nice_udp_bsd_socket_new (&addr);
+      new_socket = nice_udp_bsd_socket_new (&addr, NULL);
       if (new_socket) {
         _priv_set_socket_tos (agent, new_socket, stream->tos);
         nice_component_attach_socket (component, new_socket);
@@ -3229,6 +3229,25 @@ agent_remove_local_candidate (NiceAgent *agent, NiceStream *stream,
 
 #endif
 
+static const gchar *
+priv_host_candidate_result_to_string (HostCandidateResult result)
+{
+  switch (result) {
+    case HOST_CANDIDATE_SUCCESS:
+      return "success";
+    case HOST_CANDIDATE_FAILED:
+      return "failed";
+    case HOST_CANDIDATE_CANT_CREATE_SOCKET:
+      return "can't create socket";
+    case HOST_CANDIDATE_REDUNDANT:
+      return "redundant";
+    case HOST_CANDIDATE_DUPLICATE_PORT:
+      return "duplicate port";
+    default:
+      g_assert_not_reached ();
+  }
+}
+
 NICEAPI_EXPORT gboolean
 nice_agent_gather_candidates (
   NiceAgent *agent,
@@ -3349,11 +3368,20 @@ nice_agent_gather_candidates (
         host_candidate = NULL;
         while (res == HOST_CANDIDATE_CANT_CREATE_SOCKET ||
             res == HOST_CANDIDATE_DUPLICATE_PORT) {
-          nice_debug ("Agent %p: Trying to create %s host candidate on port %d", agent,
-              nice_candidate_transport_to_string (transport), current_port);
           nice_address_set_port (addr, current_port);
           res = discovery_add_local_host_candidate (agent, stream->id, cid,
               addr, transport, accept_duplicate, &host_candidate);
+          if (nice_debug_is_enabled ()) {
+            gchar ip[NICE_ADDRESS_STRING_LEN];
+            nice_address_to_string (addr, ip);
+            nice_debug ("Agent %p: s%d/c%d: creation of host candidate "
+                "%s:[%s]:%u: %s%s", agent, stream->id, cid,
+                nice_candidate_transport_to_string (transport), ip,
+                transport == NICE_CANDIDATE_TRANSPORT_TCP_ACTIVE ?
+                    0 : current_port,
+                priv_host_candidate_result_to_string (res),
+                accept_duplicate ? " (accept duplicate)" : "");
+          }
           if (current_port > 0)
             current_port++;
           if (current_port > component->max_port)
@@ -3367,38 +3395,13 @@ nice_agent_gather_candidates (
             break;
         }
 
-        if (res == HOST_CANDIDATE_REDUNDANT) {
-          nice_debug ("Agent %p: Ignoring local candidate, it's redundant",
-              agent);
-          continue;
-        } else if (res == HOST_CANDIDATE_FAILED) {
-          nice_debug ("Agent %p: Could not retrieve component %d/%d", agent,
-              stream->id, cid);
-          continue;
-        } else if (res == HOST_CANDIDATE_CANT_CREATE_SOCKET) {
-          if (nice_debug_is_enabled ()) {
-            gchar ip[NICE_ADDRESS_STRING_LEN];
-
-            nice_address_to_string (addr, ip);
-            nice_debug ("Agent %p: Unable to add local host %s candidate %s"
-                " for s%d:%d. Invalid interface?", agent,
-                nice_candidate_transport_to_string (transport), ip,
-                stream->id, component->id);
-          }
+        if (res == HOST_CANDIDATE_REDUNDANT ||
+            res == HOST_CANDIDATE_FAILED ||
+            res == HOST_CANDIDATE_CANT_CREATE_SOCKET)
           continue;
-        } else if (res == HOST_CANDIDATE_DUPLICATE_PORT) {
-           if (nice_debug_is_enabled ()) {
-            gchar ip[NICE_ADDRESS_STRING_LEN];
-
-            nice_address_to_string (addr, ip);
-            nice_debug ("Agent %p: Unable to add local host %s candidate %s"
-                " for"
-                " s%d:%d. Every port is duplicated", agent, ip,
-                nice_candidate_transport_to_string (transport), stream->id,
-                component->id);
-           }
-           ret = FALSE;
-           goto error;
+        else if (res == HOST_CANDIDATE_DUPLICATE_PORT) {
+          ret = FALSE;
+          goto error;
         }
 
         found_local_address = TRUE;

diff --git a/agent/discovery.c b/agent/discovery.c
index 215aaf1..7f2d13c 100644 (file)
--- a/agent/discovery.c
+++ b/agent/discovery.c
@@ -646,13 +646,30 @@ priv_local_host_candidate_duplicate_port (NiceAgent *agent,
 
             nice_address_to_string (&candidate->addr, ip);
             nice_address_to_string (&c->addr, ip2);
-            nice_debug ("Agent %p: Local host %s candidate %s"
-                " for s%d:%d will use the same port %d as %s .", agent, ip,
+            nice_debug ("Agent %p: s%d/c%d: host candidate %s:[%s]:%u "
+                " will use the same port as %s:[%s]:%u", agent,
+                stream->id, component->id,
+                nice_candidate_transport_to_string (candidate->transport),
+                ip, nice_address_get_port (&candidate->addr),
                 nice_candidate_transport_to_string (c->transport),
-                stream->id, component->id, nice_address_get_port (&c->addr),
-                ip2);
+                ip2, nice_address_get_port (&c->addr));
             return FALSE;
           }
+          {
+            gchar ip[NICE_ADDRESS_STRING_LEN];
+            gchar ip2[NICE_ADDRESS_STRING_LEN];
+
+            nice_address_to_string (&candidate->addr, ip);
+            nice_address_to_string (&c->addr, ip2);
+            nice_debug ("Agent %p: s%d/c%d: host candidate %s:[%s]:%u "
+                " has the same port as %s:[%s]:%u from s%d/c%d", agent,
+                candidate->stream_id, candidate->component_id,
+                nice_candidate_transport_to_string (candidate->transport),
+                ip, nice_address_get_port (&candidate->addr),
+                nice_candidate_transport_to_string (c->transport),
+                ip2, nice_address_get_port (&c->addr),
+                stream->id, component->id);
+          }
 
           return TRUE;
         }
@@ -714,7 +731,7 @@ HostCandidateResult discovery_add_local_host_candidate (
   /* note: candidate username and password are left NULL as stream
      level ufrag/password are used */
   if (transport == NICE_CANDIDATE_TRANSPORT_UDP) {
-    nicesock = nice_udp_bsd_socket_new (address);
+    nicesock = nice_udp_bsd_socket_new (address, &error);
   } else if (transport == NICE_CANDIDATE_TRANSPORT_TCP_ACTIVE) {
     nicesock = nice_tcp_active_socket_new (agent->main_context, address);
   } else if (transport == NICE_CANDIDATE_TRANSPORT_TCP_PASSIVE) {

diff --git a/socket/udp-bsd.c b/socket/udp-bsd.c
index 82209bc..d1c8513 100644 (file)
--- a/socket/udp-bsd.c
+++ b/socket/udp-bsd.c
@@ -79,7 +79,7 @@ struct UdpBsdSocketPrivate
 };
 
 NiceSocket *
-nice_udp_bsd_socket_new (NiceAddress *addr)
+nice_udp_bsd_socket_new (NiceAddress *addr, GError **error)
 {
   union {
     struct sockaddr_storage storage;
@@ -100,7 +100,7 @@ nice_udp_bsd_socket_new (NiceAddress *addr)
 
   if (name.storage.ss_family == AF_UNSPEC || name.storage.ss_family == AF_INET) {
     gsock = g_socket_new (G_SOCKET_FAMILY_IPV4, G_SOCKET_TYPE_DATAGRAM,
-        G_SOCKET_PROTOCOL_UDP, NULL);
+        G_SOCKET_PROTOCOL_UDP, error);
     name.storage.ss_family = AF_INET;
 #ifdef HAVE_SA_LEN
     name.storage.ss_len = sizeof (struct sockaddr_in);
@@ -123,7 +123,7 @@ nice_udp_bsd_socket_new (NiceAddress *addr)
   g_socket_set_blocking (gsock, false);
   gaddr = g_socket_address_new_from_native (&name.addr, sizeof (name));
   if (gaddr != NULL) {
-    gret = g_socket_bind (gsock, gaddr, FALSE, NULL);
+    gret = g_socket_bind (gsock, gaddr, FALSE, error);
     g_object_unref (gaddr);
   }
 

diff --git a/socket/udp-bsd.h b/socket/udp-bsd.h
index c8d6190..50e1c3f 100644 (file)
--- a/socket/udp-bsd.h
+++ b/socket/udp-bsd.h
@@ -44,7 +44,7 @@
 G_BEGIN_DECLS
 
 NiceSocket *
-nice_udp_bsd_socket_new (NiceAddress *addr);
+nice_udp_bsd_socket_new (NiceAddress *addr, GError **error);
 
 G_END_DECLS
 

diff --git a/tests/test-bsd.c b/tests/test-bsd.c
index c182164..6538a2f 100644 (file)
--- a/tests/test-bsd.c
+++ b/tests/test-bsd.c
@@ -61,8 +61,10 @@ static void
 test_socket_initial_properties (void)
 {
   NiceSocket *sock;
+  GError *error = NULL;
 
-  sock = nice_udp_bsd_socket_new (NULL);
+  sock = nice_udp_bsd_socket_new (NULL, &error);
+  g_assert_no_error (error);
   g_assert (sock != NULL);
 
   // not bound to a particular interface
@@ -78,8 +80,10 @@ test_socket_address_properties (void)
 {
   NiceSocket *sock;
   NiceAddress tmp;
+  GError *error = NULL;
 
-  sock = nice_udp_bsd_socket_new (NULL);
+  sock = nice_udp_bsd_socket_new (NULL, &error);
+  g_assert_no_error (error);
   g_assert (sock != NULL);
 
   g_assert (nice_address_set_from_string (&tmp, "127.0.0.1"));
@@ -97,11 +101,14 @@ test_simple_send_recv (void)
   NiceSocket *client;
   NiceAddress tmp;
   gchar buf[5];
+  GError *error = NULL;
 
-  server = nice_udp_bsd_socket_new (NULL);
+  server = nice_udp_bsd_socket_new (NULL, &error);
+  g_assert_no_error (error);
   g_assert (server != NULL);
 
-  client = nice_udp_bsd_socket_new (NULL);
+  client = nice_udp_bsd_socket_new (NULL, &error);
+  g_assert_no_error (error);
   g_assert (client != NULL);
 
   g_assert (nice_address_set_from_string (&tmp, "127.0.0.1"));
@@ -132,8 +139,10 @@ test_zero_send_recv (void)
   gchar buf[5];
   NiceOutputMessage local_out_message;
   NiceInputMessage local_in_message;
+  GError *error = NULL;
 
-  sock = nice_udp_bsd_socket_new (NULL);
+  sock = nice_udp_bsd_socket_new (NULL, &error);
+  g_assert_no_error (error);
   g_assert (sock != NULL);
 
   g_assert (nice_address_set_from_string (&tmp, "127.0.0.1"));
@@ -168,11 +177,14 @@ test_multi_buffer_recv (void)
   NiceAddress tmp;
   guint8 buf[20];
   guint8 dummy_buf[9];
+  GError *error = NULL;
 
-  server = nice_udp_bsd_socket_new (NULL);
+  server = nice_udp_bsd_socket_new (NULL, &error);
+  g_assert_no_error (error);
   g_assert (server != NULL);
 
-  client = nice_udp_bsd_socket_new (NULL);
+  client = nice_udp_bsd_socket_new (NULL, &error);
+  g_assert_no_error (error);
   g_assert (client != NULL);
 
   g_assert (nice_address_set_from_string (&tmp, "127.0.0.1"));
@@ -239,11 +251,14 @@ test_multi_message_recv (guint n_sends, guint n_receives,
   NiceSocket *server;
   NiceSocket *client;
   NiceAddress tmp;
+  GError *error = NULL;
 
-  server = nice_udp_bsd_socket_new (NULL);
+  server = nice_udp_bsd_socket_new (NULL, &error);
+  g_assert_no_error (error);
   g_assert (server != NULL);
 
-  client = nice_udp_bsd_socket_new (NULL);
+  client = nice_udp_bsd_socket_new (NULL, &error);
+  g_assert_no_error (error);
   g_assert (client != NULL);
 
   g_assert (nice_address_set_from_string (&tmp, "127.0.0.1"));

diff --git a/tests/test-drop-invalid.c b/tests/test-drop-invalid.c
index def5b03..a484c2d 100644 (file)
--- a/tests/test-drop-invalid.c
+++ b/tests/test-drop-invalid.c
@@ -419,13 +419,15 @@ static int run_full_test (NiceAgent *lagent, NiceAgent *ragent, NiceAddress *bas
     NiceCandidate *local_cand = NULL;
     NiceCandidate *remote_cand = NULL;
     NiceSocket *tmpsock;
+    GError *error = NULL;
 
     g_assert (nice_agent_get_selected_pair (lagent, ls_id, 1, &local_cand,
             &remote_cand));
     g_assert (local_cand);
     g_assert (remote_cand);
 
-    tmpsock = nice_udp_bsd_socket_new (NULL);
+    tmpsock = nice_udp_bsd_socket_new (NULL, &error);
+    g_assert_no_error (error);
     nice_socket_send (tmpsock, &remote_cand->addr, 4, "ABCD");
     nice_socket_send (tmpsock, &local_cand->addr, 5, "ABCDE");
     nice_socket_free (tmpsock);

diff --git a/tests/test-socket-is-based-on.c b/tests/test-socket-is-based-on.c
index db48924..17ca5fd 100644 (file)
--- a/tests/test-socket-is-based-on.c
+++ b/tests/test-socket-is-based-on.c
@@ -84,6 +84,7 @@ main (int argc, char *argv[])
   GMainLoop *mainloop = NULL;
 
   NiceAddress addr;
+  GError *error = NULL;
 
   g_networking_init ();
 
@@ -95,7 +96,8 @@ main (int argc, char *argv[])
   nice_address_set_from_string (&addr, "127.0.0.1");
 
   /* Standalone socket */
-  udp_bsd = nice_udp_bsd_socket_new (&addr);
+  udp_bsd = nice_udp_bsd_socket_new (&addr, &error);
+  g_assert_no_error (error);
 
   /* tcp_passive -> pseudossl -> udp_turn_over_tcp */
   tcp_active = nice_tcp_active_socket_new (g_main_loop_get_context (mainloop),