Repository navigation
zbeacon: Fix IPv4 multicast (and small cache dir fix to zhash/zhashx selftests) - #2326
Merged
Merged
Conversation
Clangd uses `.cache` directory which conflicts with the directory used by the selftests of zhash and zhashx, which caused the tests to fail unnecessarily. Changed those to use `src/selftest-rw` like other tests.
zbeacon multicast always fails at `setsockopt` for IP_MULTICAST_IF on Linux, because Linux expects the parameter type to be either `ip_mreqn`, `ip_mreq`, or `in_addr` structure instead of interface index. On the other hand, Windows does not support those structures, but instead supports a DWORD of IPv4 address or an interface index. OSX seems to support `in_addr`. Modified the parameter type to be interface index for Windows and `in_addr` for others. Multicast beacons were also received from all multicast groups instead of just the one we join to. This is because the code bound the listening UDP socket to `INADDR_ANY` when using multicast. Modified to bind the listening UDP socket only to the specified multicast address.
Contributor
|
thanks, this really needed attention 👍 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I didn't manage to get zbeacon IPv4 multicast to work on Linux (debian), and took a look at it and now I don't know how (if at all) this could've worked to begin with? In original code,
setsockopt IP_MULTICAST_IFpassed interface as an index, but looking at Linux docs it should only acceptip_mreq,ip_mreqnorin_addrstructs, and indeed in my testing the multicast always fails on thissetsockoptcall. To further complicate things, on OSX onlyin_addrstruct is accepted, and on Windows it can only be a DWORD containing interface index or IP address.To make this work on Linux, OSX and Windows, I changed it to use interface index only on Windows and use
in_addrstruct on other platforms. At least tested this on those three platforms and now IPv4 multicast beacon worked on all of them.Then I noticed another problem that the original code always bound the listening socket to
INADDR_ANYon *NIX platform, which causes that any beacons in any multicast group will be heard by the socket, which in turn makes using multicast meaningless compared to broadcast (beacons in different multicast groups hear each other). This was fixed by binding the listening socket on *NIX to the multicast address instead when we are using multicast, after which the beacons in one multicast group were only heard by beacons that are members in that specific group. I assume this is the way this was intended to work, please correct me if I'm wrong?On Windows, the original code already bound to the host address instead, and for some reason that already also works correctly with multicast and only the beacon in the specific multicast group is heard. Honestly, I don't know why that works, but I guess Windows handles the IPv4 membership for the socket differently?
zbeacon selftest also had a similarly broken check for determining if IPv4 multicast was available. I actually just removed the check and relied on the zbeacon
CONFIGUREoutput instead to determine if multicast is there.zhash/zhashx cache dir fix
Another commit makes a small fix to zhash/zhashx selftests that used
.cacheas a temporary file for testing. The problem is that if you have clangd static analysis running, it creates.cachedirectory for the analysis which annoyingly breaks the tests. I replaced this according to the convention from other tests that RW data from tests should go toselftest-rwdirectory instead.