Apply suggestions from code review

Co-authored-by: Iñaki Baz Castillo <[email protected]>

node/src/PlainTransport.ts

@@ -109,7 +109,8 @@ export type PlainTransportOptions<PlainTransportAppData extends AppData = AppDat
 	srtpCryptoSuite?: SrtpCryptoSuite;
 
 	/**
-	 * Enable Multicast. Default false.
+	 * Enable multicast for UDP in case listening IP is a multicast valid IP.
+	 * Default false.
 	 */
 	enableMulticast?: boolean;
 
@@ -139,7 +140,7 @@ export type PlainTransportObserverEvents = TransportObserverEvents &
 {
 	tuple: [TransportTuple];
 	rtcptuple: [TransportTuple];
-	sctpstatechange: [SctpState];	
+	sctpstatechange: [SctpState];
 };
 
 type PlainTransportConstructorOptions<PlainTransportAppData> =

node/src/Router.ts

@@ -727,11 +727,6 @@ export class Router<RouterAppData extends AppData = AppData>
 			};
 		}
 
-		if (enableMulticast)
-		{
-			logger.debug('createPlainTransport() | enableMulticast');
-		}
-
 		const transportId = generateUUIDv4();
 
 		/* Build Request. */

node/src/tests/test-PlainTransport.ts

@@ -319,13 +319,11 @@ test('router.createPlainTransport() with non bindable IP rejects with Error', as
 test('router.createPlainTransport() with enableMulticast succeeds', async () =>
 {
 	// Use default cryptoSuite: 'AES_CM_128_HMAC_SHA1_80'.
-	const transport1 = await router.createPlainTransport(
+	await expect(router.createPlainTransport(
 		{
-			listenIp   : '127.0.0.1',
-			enableMulticast: true
-		});
-
-	expect(typeof transport1.id).toBe('string');
+			listenIp        : '127.0.0.1',
+			enableMulticast : true
+		})).not.toThrow();
 }, 2000);
 
 test('plainTransport.getStats() succeeds', async () =>

rust/src/messages.rs

@@ -911,6 +911,7 @@ pub(crate) struct RouterCreatePlainTransportData {
     sctp_send_buffer_size: u32,
     enable_srtp: bool,
     srtp_crypto_suite: SrtpCryptoSuite,
+    enable_multicast: bool,
     is_data_channel: bool,
 }
 
@@ -931,6 +932,7 @@ impl RouterCreatePlainTransportData {
             sctp_send_buffer_size: plain_transport_options.sctp_send_buffer_size,
             enable_srtp: plain_transport_options.enable_srtp,
             srtp_crypto_suite: plain_transport_options.srtp_crypto_suite,
+            enable_multicast: plain_transport_options.enable_multicast,
             is_data_channel: false,
         }
     }
@@ -955,6 +957,7 @@ impl RouterCreatePlainTransportData {
             comedia: self.comedia,
             enable_srtp: self.enable_srtp,
             srtp_crypto_suite: Some(SrtpCryptoSuite::to_fbs(self.srtp_crypto_suite)),
+            enable_multicast: self.enable_multicast,
         }
     }
 }

rust/src/router/plain_transport.rs

@@ -73,7 +73,7 @@ pub struct PlainTransportOptions {
     /// The SRTP crypto suite to be used if enableSrtp is set.
     /// Default 'AesCm128HmacSha180'.
     pub srtp_crypto_suite: SrtpCryptoSuite,
-    /// Enable Multicast.
+    /// Enable multicast for UDP in case listening IP is a multicast valid IP.
     /// Default false.
     pub enable_multicast: bool,
     /// Custom application data.
@@ -95,6 +95,7 @@ impl PlainTransportOptions {
             sctp_send_buffer_size: 262_144,
             enable_srtp: false,
             srtp_crypto_suite: SrtpCryptoSuite::default(),
+            enable_multicast: false,
             app_data: AppData::default(),
         }
     }

rust/tests/integration/plain_transport.rs

@@ -265,6 +265,32 @@ fn create_succeeds() {
                 assert_eq!(transport_dump.sctp_state, transport2.sctp_state());
             }
         }
+
+        {
+            let transport = router
+                .create_plain_transport({
+                    let mut plain_transport_options = PlainTransportOptions::new(ListenInfo {
+                        protocol: Protocol::Udp,
+                        ip: IpAddr::V4(Ipv4Addr::LOCALHOST),
+                        announced_ip: Some("4.4.4.4".parse().unwrap()),
+                        port: None,
+                        send_buffer_size: None,
+                        recv_buffer_size: None,
+                    });
+                    plain_transport_options.enable_multicast = true;
+
+                    plain_transport_options
+                })
+                .await
+                .expect("Failed to create Plain transport");
+
+            let router_dump = router.dump().await.expect("Failed to dump router");
+            assert_eq!(router_dump.transport_ids, {
+                let mut set = HashedSet::default();
+                set.insert(transport.id());
+                set
+            });
+        }
     });
 }
 

worker/include/RTC/PlainTransport.hpp

@@ -85,7 +85,6 @@ namespace RTC
 		size_t srtpMasterLength{ 0 };
 		std::string srtpKeyBase64;
 		bool connectCalled{ false }; // Whether connect() was succesfully called.
-		bool multicast{ false };
 	};
 } // namespace RTC
 

worker/include/Utils.hpp

@@ -87,7 +87,7 @@ namespace Utils
 
 		static void NormalizeIp(std::string& ip);
 
-		static bool IsMulticast(const struct sockaddr_storage* addr, const int& family);
+		static bool IsMulticast(const struct sockaddr_storage* addr, const int family);
 	};
 
 	class File

worker/src/RTC/PlainTransport.cpp

@@ -155,18 +155,16 @@ namespace RTC
 			this->srtpKeyBase64 = Utils::String::Base64Encode(this->srtpKey);
 		}
 
-		this->multicast = options->enableMulticast();
-
 		try
 		{
 			// This may throw.
 			if (this->listenInfo.port != 0)
 			{
-				this->udpSocket = new RTC::UdpSocket(this, this->listenInfo.ip, this->listenInfo.port, this->multicast);
+				this->udpSocket = new RTC::UdpSocket(this, this->listenInfo.ip, this->listenInfo.port, options->enableMulticast());
 			}
 			else
 			{
-				this->udpSocket = new RTC::UdpSocket(this, this->listenInfo.ip, this->multicast);
+				this->udpSocket = new RTC::UdpSocket(this, this->listenInfo.ip, options->enableMulticast());
 			}
 
 			if (this->listenInfo.sendBufferSize != 0)
@@ -187,11 +185,11 @@ namespace RTC
 				if (this->rtcpListenInfo.port != 0)
 				{
 					this->rtcpUdpSocket =
-					  new RTC::UdpSocket(this, this->rtcpListenInfo.ip, this->rtcpListenInfo.port, this->multicast);
+					  new RTC::UdpSocket(this, this->rtcpListenInfo.ip, this->rtcpListenInfo.port, options->enableMulticast());
 				}
 				else
 				{
-					this->rtcpUdpSocket = new RTC::UdpSocket(this, this->rtcpListenInfo.ip, this->multicast);
+					this->rtcpUdpSocket = new RTC::UdpSocket(this, this->rtcpListenInfo.ip, options->enableMulticast());
 				}
 
 				if (this->rtcpListenInfo.sendBufferSize != 0)

worker/src/RTC/PortManager.cpp

@@ -218,10 +218,11 @@ namespace RTC
 				case Transport::UDP:
 				{
 #if defined(__linux__)
-					if (enableMulticast && Utils::IP::IsMulticast(&bindAddr, family))
+					if (enableMulticast && Utils::IP::IsMulticast(std::addressof(bindAddr), family))
 					{
-						flags |= UV_UDP_REUSEADDR;
 						MS_DEBUG_DEV("enabling UV_UDP_REUSEADDR");
+
+						flags |= UV_UDP_REUSEADDR;
 					}
 #endif
 
@@ -481,10 +482,11 @@ namespace RTC
 			case Transport::UDP:
 			{
 #if defined(__linux__)
-				if (enableMulticast && Utils::IP::IsMulticast(&bindAddr, family))
+				if (enableMulticast && Utils::IP::IsMulticast(std::addressof(bindAddr), family))
 				{
-					flags |= UV_UDP_REUSEADDR;
 					MS_DEBUG_DEV("enabling UV_UDP_REUSEADDR");
+
+					flags |= UV_UDP_REUSEADDR;
 				}
 #endif
 

worker/src/Utils/IP.cpp

@@ -156,7 +156,7 @@ namespace Utils
 		}
 	}
 
-	bool IP::IsMulticast(const struct sockaddr_storage* addr, const int& family)
+	bool IP::IsMulticast(const struct sockaddr_storage* addr, const int family)
 	{
 		MS_TRACE();
 
@@ -165,29 +165,21 @@ namespace Utils
 			case AF_INET:
 			{
 				uint32_t s_addr = ntohl((reinterpret_cast<const struct sockaddr_in*>(addr))->sin_addr.s_addr);
-				if (s_addr >= 0xe0000000 && s_addr <= 0xefffffff)
-				{
-					return true;
-				}
-				break;
+
+				return (s_addr >= 0xe0000000 && s_addr <= 0xefffffff);
 			}
 
 			case AF_INET6:
 			{
 				uint8_t _s6_addr = (reinterpret_cast<const struct sockaddr_in6*>(addr))->sin6_addr.s6_addr[0];
-				if (_s6_addr == 0xFF)
-				{
-					return true;
-				}
-				break;
+
+				return (_s6_addr == 0xFF);
 			}
 
 			default:
 			{
 				MS_THROW_TYPE_ERROR("invalid family");
 			}
 		}
-
-		return false;
 	}
 } // namespace Utils