n Montag, 7. Mai 2018 12:23:57 CEST Sven Eckelmann wrote:
This would sound to me like the vlan should just be created instead of adding this workaround. But maybe you should discuss the details here with Antonio because at the end it is just about the different ways of handling "!vlan" case from your patch.
Just had a look at the cleanup code for vlans and it seems like each vlan in vlan_list must contain at least one entry (according to vlan->tt.num_entries) and otherwise batadv_tt_global_size_mod will drop it. So it isn't as easy as just adding a simple vlan for each received vlan entry via TT response.
So your patch (or a variant of it) will most likely be the way to go for receivers.
Now to your commit message:
On Sonntag, 6. Mai 2018 21:55:59 CEST Leonardo Mörlein wrote:
batman-adv: mitigate issue when empty vlan is received
Something more specific would be nice. Maybe somebody else has a better idea but what about:
batman-adv: Mitigate TT loop when receiving empty VLAN
This patch is not a fix for the issue itself, but a fix for the other nodes, which are also influenced by the issue.
I personally don't like to read "this patch" in commit messages. This part should maybe be reformulated and put at the end (before the Fixes line). Something like:
While the problem is caused by the sender, the receiver can mitigate the problem by ignoring (potentially) empty VLANs during the CRC check.
[...]
- This behaviour is a bug! Batman-adv nodes must not send TT announcements for empty vlans.
Where is this defined as incorrect message? But yes, the current implementation cannot handle it.
- The receiving batman-adv can not handle incoming TT announcements with empty vlans. (The crc check routine batadv_tt_global_check_crc() fails.)
This is to vague as discussed earlier. Maybe something more like:
- The receiving batman-adv can not handle incoming TT announcements with empty vlans. No new struct batadv_orig_node_vlan entry will be added to batadv_orig_node->vlan_list. This will cause the crc check routine batadv_tt_global_check_crc() to fail because it cannot find the local representation of this empty vlan.
[...]
- This causes a lot of unneeded TT traffic. In Freifunk Hannover we decreased the amount of from ~20kpp/s to ~4kpp/s on our supernodes. We have ~800 nodes, which are connected via vpn to one of six supernodes. Those supernodes are connected with each other in a fully connected mesh.
This part doesn't seem to fit in the list and might just be a new paragraph at the end to summarize the impact of the problem.
A "Fixes: " line is missing here. Please check whether this sounds about right:
Fixes: 7ea7b4a14275 ("batman-adv: make the TT CRC logic VLAN specific")
Kind regards, Sven