Fix several problems with the Device Information TLV #825
Conversation
|
Although I'm Author of these commits, everything was done together with Mario. |
| std::shared_ptr<ieee1905_1::cLocalInterfaceInfo> localInterfaceInfo = | ||
| tlvDeviceInformation->create_local_interface_list(); | ||
|
|
||
| ieee1905_1::eMediaType media_type = ieee1905_1::eMediaType::UNKNONWN_MEDIA; | ||
| if (SPEED_100 == speed) { | ||
| media_type = ieee1905_1::eMediaType::IEEE_802_3U_FAST_ETHERNET; | ||
| } else if (SPEED_1000 == speed) { | ||
| } else if (SPEED_1000 <= speed) { |
There was a problem hiding this comment.
Should there be a catch all for very unlikely case of a bad 10 Mbps link?
media_type = ieee1905_1::eMediaType::IEEE_802_3U_FAST_ETHERNET;
if (SPEED_1000 <= speed) {
media_type = ieee1905_1::eMediaType::IEEE_802_3AB_GIGABIT_ETHERNET;
}
There was a problem hiding this comment.
I tried to explain in the commit message what we intentionally don't do that. Actually the catch-all for gigabit is not ideal either.
There was a problem hiding this comment.
Yes, I thought that too at first, but then looked in the spec. It requires to report media type IEEE 802.3u (Fast Ethernet) vs IEEE 802.3ab (Gigabit Ethernet). I do not know of a better way, but using current (negotiated) link speed to decide the media type is flawed in the first place. That's why IMO the right thing to do is the best guess/closest match (as you did for 10Gbps).
Strictly speaking your changes already make it better, so this is just a suggestion.
| ip link add wlan0 address "${base_mac}:00:10" type dummy | ||
| ip link add wlan2 address "${base_mac}:00:20" type dummy | ||
| ip link set dev wlan0 up | ||
| ip link set dev wlan2 up | ||
| for iface in wlan0 wlan2 @BEEROCKS_BH_WIRE_IFACE@ $DATA_IFACE | ||
| for iface in wlan0 wlan2 $DATA_IFACE |
There was a problem hiding this comment.
I don't understand this one. Isn't it the purpose of the platform_init code to make (for Linux target) a bridge and put wireless and wired interfaces under similarly to a typical AP?
There was a problem hiding this comment.
The DATA_IFACE is supposed to be the wired backhaul interface. I also tried to explain that in the commit message.
There was a problem hiding this comment.
Ah, I didn't read the commit messages, just the review. No more references to BEEROCKS_BH_WIRE_IFACE in the script and passing the interface name in as a parameter sounds good.
arnout
force-pushed
the
hotfix/device-information-fixes
branch
from
c4aa1b8 to
1bc4134
Compare
To generate the Device Information TLV, we use ethtool for wired interfaces. However, the dummy interface we use for sim-eth0 in docker doesn't support ethtool. Therefore, it will error with "operation not supported". To be able to generate valid Device Information TLV also in docker, use eth0 instead of sim-eth0. sim-eth0 was introduced in commit d16115c to avoid messing up the host when prplmesh-utils.sh is used natively instead of in docker. However, since then, prplmesh-utils.sh has been updated with a DATA_IFACE option, which would/should be eth0. Also, the docker images no longer use prplmesh-utils.sh to set up the interfaces. Therefore, remove the sim-eth0 dummy interface from prplmesh-utils.sh as well. It is no longer used. Remove the sim-eth0 dummy interface from the docker entrypoint as well. Note that leaving the dummy interface in prplmesh-utils.sh and in docker entrypoint does not hurt. It's just not needed any more. Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <[email protected]>
The Device Information TLV contains a list of interfaces. We currently put the bridge in it, but this is not actually an interface we should put in there. Instead, we should put the backhaul interface in it. Since we currently only support wired backhaul, use wired_iface. Note that the wireless interfaces should also be added here, but currently we don't have an easy way to get their speed (i.e. 802.11 subspec), so we simply don't add them. There is already a TODO comment that explains this. Note that the other wired interfaces that are in the bridge should also be mentioned in the interfaces list, but currently we don't. Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <[email protected]>
The IEEE 1905.1 specification only supports 2 Ethernet media types: 100Mbps and 1Gbps. However, many more media types faster than 1Gbps exist. In fact, the virtual ethernet interface which is used in docker is represented as a 10Gbps interface. Since we don't want to report "unknown media" in our tests, use 1Gbps for all Ethernet speeds larger than 1Gbps. Smaller speeds are kept as "unknown media". Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <[email protected]>
eMediaType is a 16-bit enum, so it must be swapped. This is currently not done, which results in the wrong values sent over the wire. To fix this, do swapping for all enums, based on the size of the type. This does result in redundant swaps for the 8-bit enums, but that is negligible. In addition, tlvf_swap is inlined anyway so when optimising the code will be removed entirely. Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <[email protected]>
tomereli
force-pushed
the
hotfix/device-information-fixes
branch
from
1bc4134 to
df33317
Compare
As reported by @mariomaz, the Device Information TLV was not correct.
This PR fixes all of the issues.
Unfortunately, since we don't inspect the packets sent over the wire, it's not possible to add a test to test_flows.