Fixing resolution of broadcast address if multiple ip addresses are available #100
Conversation
| .ToArray(); | ||
|
|
||
| return unicastAddresses.Length == 1 | ||
| ? unicastAddresses.Single() | ||
| return unicastAddresses.Length >= 1 | ||
| ? unicastAddresses.First() | ||
| : null; |
There was a problem hiding this comment.
How about we remove .ToArray() and do
return unicastAddresses.FirstOrDefault()There was a problem hiding this comment.
But, this would change the original behavior. What is the issue issue with having null returned here? Is it the invalid broadcast address used later?
There was a problem hiding this comment.
Exactly, when null is returned the broadcast address is resolved to 255.255.255.255 which is not allowed in BACnet.
There was a problem hiding this comment.
Taking one step back, the original problem is when using 0.0.0.0 when you have multipe interfaces active or an interface in use with multiple ip's bound to it. -> In this case, it's not clear which broadcast address should be used at all?
In my opinion, there has to be a check when initializing the udp transport to bind to a dedicated ip address available on the device, if multiple options are available. In this case, the correct broadcast address will always be resolved automatically!
There was a problem hiding this comment.
Ok. But if we take TcpListener for example and tell it to bound to 0.0.0.0, it wil listen on all available interfaces. I think this is the reason the original authors wanted to listen on all interfaces, not only one. As for broadcast, I'm not sure, does sending udp packet to 255.255.255.255 results in sending it via all interfaces or it doesn't work at all? If it's the later, then maybe broadcast address should always be explicitly specified and not automatically resolved.
There was a problem hiding this comment.
That is correct! 0.0.0.0 will listen on all interfaces -> the resolution of the broadcast address is the only issue here.
Regarding the validity of the 255.255.255.255 broadcast address in BACnet, I'll have a detailed look on the spec.
As far as I know it is not allowed. Unfortunately, I already saw devices from big vendors relying on it, though.
-> has to be double checked before merging :-(
Will give an update on this...
There was a problem hiding this comment.
A UDP broadcast packet is 'sent' to an IP address with the subnet of the broadcasting device and all 1's in the host portion. For example, if a device has an address 128.253.109.10 and a subnet mask 255.255.255.0, it can use the broadcast address 128.253.109.255. Although most networks also allow the use of the IP broadcast address 255.255.255.255, we have ruled this out for BACnet/IP because of some IP protocol stack limitations that certain implementors have encountered.
There was a problem hiding this comment.
This bug bit us a few times. I would prefer some sort of a thrown exception if a reasonable interface can't be found (after filtering out lo). Using first available interface may not be good, as it would be non-deterministic, and might pick the wrong one
There was a problem hiding this comment.
+1 this bit me the first time I tested the library out as well.
I would prefer an exception if no suitable interface could be found. If multiple suitable interfaces are found, either pick the first one or better yet - being able to specify which one based on some sort of rule set (interface name, type, identifier etc). Or expose a public method that gets the list of suitable interfaces and accept one of them as the argument.
If a network interface has multiple ip addresses configured or there are multiple network interfaces available and up, the broadcast address was resolved to 255.255.255.255 which is not supported by the bacnet standard.
To mitigate this behavior the first interface found is chosen and the correct broadcast address for this interface will be calculated and used for broadcasting.
This solves #93