search

g4klx / MMDVMHost

The host program for the MMDVM
GNU General Public License v2.0
379 stars 274 forks source link

Strange behavior in CUDPSocket::open(const sockaddr_storage& address) does not really use the the resolved address #663

Open shawnchain opened 3 years ago

shawnchain commented 3 years ago

CUDPSocket:open(const sockaddr_storage& address) does not really read the resolved address from method parameter, but instead it finally reads from m_address_saved, this is weird.

https://github.com/g4klx/MMDVMHost/blob/136deac61a5a76959c091372ccd9cb91fb23c1cd/UDPSocket.cpp#L164

@jg1uaa

jg1uaa commented 3 years ago

Because this is inherited from original UDPSocket.cpp design. IPv4 address string and port number is stored into m_address and m_port when calling constructor CUDPSocket. And CUDPSocket::open() uses stored these information.

So, IPv6 version of CUDPSocket::open(sockaddr_storage &address) refers only address family in sockaddr_storage, not IP address.

Is the time to replace open(sockaddr_storage &address) to open(unsigned int af)?

jg1uaa commented 3 years ago

665

randybuildsthings commented 3 years ago

It's a side effect. There's a LOT to unpack from a computer science perspective on this particular class and how it's used. I looked through it for about half an hour of bouncing between files, and the CUDPSocket class behaves largely like a utility class that perhaps could've been implemented solely with static methods. However, it's a hybrid and relies on a pretty intricate set of side effects and what some would call "magical behavior" (what we refer to in the software trade as "tight coupling") in order to do what it needs to do.

Above all, this is not a trivial thing to consider fixing, since it shows up as a core piece of how all of the networking functions in MMDVMHost. It would take time to unwind it.

Not seeing a huge value in spending time fixing this any time soon. The whole network engine would need rework.