Bugzilla – Bug 2474
UdpEchoClient does not call Connect with addresses of type Inet[6]SocketAddress
Last modified: 2016-09-02 05:03:34 UTC
Created attachment 2537 [details] Proposed patch UdpEchoClient::StartApplication only binds the socket and connect to the destination address if the latter is an Ipv4Address or an Ipv6Address. However, the user can specify an InetSocketAddress or an Inet6SocketAddress, so as to exploit, e.g., the possibility to set the ToS. The attached patch solves this issue, which has been spotted by Mark Rison (who also tested the attached patch). Also, the following constructor of UdpEchoClientHelper is a bit confusing: UdpEchoClientHelper::UdpEchoClientHelper (Address address, uint16_t port) because, if address is of type Inet[6]SocketAddress, the port is included in the address and should not be specified again. If address and port are to be specified separately, the other constructors can be used: UdpEchoClientHelper::UdpEchoClientHelper (Ipv4Address address, uint16_t port) UdpEchoClientHelper::UdpEchoClientHelper (Ipv6Address address, uint16_t port) If agreed, I can enhance the attached patch by removing the port parameter for the first of the constructors listed above.
I agree on the patch, but... 1) Also UdpClient and UdpTraceClient should be updated, and 2) Do not remove the constructors or the SetRemote functions. Instead, mark them as deprecated.
Created attachment 2543 [details] Proposed patch v2 Thanks for looking into this. I have only deprecated the port argument. If you think it is better to deprecate the whole method, I will prepare a new patch.
(In reply to Stefano Avallone from comment #2) > Created attachment 2543 [details] > Proposed patch v2 > > Thanks for looking into this. > > I have only deprecated the port argument. If you think it is better to > deprecate the whole method, I will prepare a new patch. Given the fact that people do NOT look at the documentation, I'd add new methods without the port and I'd deprecate the old ones altogether. The NS_DEPRECATED macro provides a way more visible effect (on a MacOS it hangs the compilation, evil).
Created attachment 2544 [details] Proposed patch v3 Agreed. Here it is the updated patch. Please let me know if I can push it to ns-3-dev, thanks.
Created attachment 2552 [details] Proposed patch v4 The previous patch (v3) caused a build failure with clang, as the deprecated helper constructor (the one that takes an Address and an uint32_t) is used by an example (udp-echo.cc). Looking at this example made me realize that we just need two constructor variants: UdpEchoClientHelper (Address ip, uint16_t port); UdpEchoClientHelper (Address addr); Indeed, thanks to the Address() operator, we can pass Ipv4Address and Ipv6Address to the former and InetSocketAddress and Inet6SocketAddress to the latter. So, there is no need for the current variants: UdpEchoClientHelper (Ipv4Address ip, uint16_t port); UdpEchoClientHelper (Ipv6Address ip, uint16_t port); The attached patch (verified by building with clang) adopts such an approach for all the helper constructors and the application SetRemote methods.
Just to help you evaluate the attached patch, I want to clarify that it can be decomposed in three parts: a) make udp clients work if the specified address is of type InetSockAddress. I think this part is required, especially now that InetSocketAddresses are used to set the ToS. b) add constructors/SetRemote methods that only take an Address as input. This part prevents users from getting confused by the need of setting the port value twice (once in the InetSockAddress struct, once as parameter of the constructor/SetRemote method). I think this part would be a nice addition. c) remove redundant versions of constructors/SetRemote methods. Currently, we have, e.g.: UdpEchoClientHelper (Address ip, uint16_t port); UdpEchoClientHelper (Ipv4Address ip, uint16_t port); UdpEchoClientHelper (Ipv6Address ip, uint16_t port); The last two variants are useless because, thanks to the Address() operator, the former variant can be called when the address is of type Ipv4Address or Ipv6Address. This part is optional, but it is safe and - in my opinion - improves clarity of code and removes clutter (from users point of view, too). Please let me know if you are comfortable with me applying all the three parts or you want me to just apply a) and b). Thanks!
I am fine with the whole patch; if Tommaso gives a +1, please apply the patch.
Created attachment 2559 [details] Proposed patch v4 bis +1 with these small improvements (a couple of logs forgotten and a safety check: the suer could be so dumb to initialize the address to a non-IP value now).
Created attachment 2560 [details] Proposed patch v4 bis right patch file
Pushed the last patch with changeset 12289 2c0a5994185d