[B.A.T.M.A.N.] [PATCH] batman-adv: fix visualization output without neighbors on the primary interface
The primary entry and the corresponding secondary entries are missing when there are no neighbors on the primary interface. This also causes the TT entries to miss and makes nodes with multiply secondary interface fall apart since there is no way to see they are related without a primary entry.
Fix this by always emitting a primary entry.
Signed-off-by: Matthias Schiffer mschiffer@universe-factory.net --- vis.c | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-)
diff --git a/vis.c b/vis.c index cec216f..b293bdb 100644 --- a/vis.c +++ b/vis.c @@ -207,7 +207,6 @@ int vis_seq_print_text(struct seq_file *seq, void *offset) int vis_server = atomic_read(&bat_priv->vis_mode); size_t buff_pos, buf_size; char *buff; - int compare;
primary_if = primary_if_get_selected(bat_priv); if (!primary_if) @@ -228,14 +227,17 @@ int vis_seq_print_text(struct seq_file *seq, void *offset) entries = (struct vis_info_entry *) ((char *)packet + sizeof(*packet));
+ vis_data_insert_interface(packet->vis_orig, + &vis_if_list, true); + for (j = 0; j < packet->entries; j++) { if (entries[j].quality == 0) continue; - compare = - compare_eth(entries[j].src, packet->vis_orig); + if (compare_eth(entries[j].src, packet->vis_orig)) + continue; vis_data_insert_interface(entries[j].src, &vis_if_list, - compare); + false); }
hlist_for_each_entry(entry, pos, &vis_if_list, list) { @@ -276,14 +278,17 @@ int vis_seq_print_text(struct seq_file *seq, void *offset) entries = (struct vis_info_entry *) ((char *)packet + sizeof(*packet));
+ vis_data_insert_interface(packet->vis_orig, + &vis_if_list, true); + for (j = 0; j < packet->entries; j++) { if (entries[j].quality == 0) continue; - compare = - compare_eth(entries[j].src, packet->vis_orig); + if (compare_eth(entries[j].src, packet->vis_orig)) + continue; vis_data_insert_interface(entries[j].src, &vis_if_list, - compare); + false); }
hlist_for_each_entry(entry, pos, &vis_if_list, list) {
WARNING: line over 80 characters #102: FILE: vis.c:236: + if (compare_eth(entries[j].src, packet->vis_orig))
WARNING: line over 80 characters #123: FILE: vis.c:287: + if (compare_eth(entries[j].src, packet->vis_orig))
total: 0 errors, 2 warnings, 0 checks, 47 lines checked
http://www.open-mesh.org/wiki/open-mesh/Contribute#Submitting-patches
Thanks, Sven
Oops, sorry about that. I'll send a fixed version.
Matthias
On 05/05/2012 05:29 PM, Sven Eckelmann wrote:
WARNING: line over 80 characters #102: FILE: vis.c:236:
if (compare_eth(entries[j].src, packet->vis_orig))WARNING: line over 80 characters #123: FILE: vis.c:287:
if (compare_eth(entries[j].src, packet->vis_orig))total: 0 errors, 2 warnings, 0 checks, 47 lines checked
http://www.open-mesh.org/wiki/open-mesh/Contribute#Submitting-patches
Thanks, Sven
The primary entry and the corresponding secondary entries are missing when there are no neighbors on the primary interface. This also causes the TT entries to miss and makes nodes with multiply secondary interface fall apart since there is no way to see they are related without a primary entry.
Fix this by always emitting a primary entry.
Signed-off-by: Matthias Schiffer mschiffer@universe-factory.net --- vis.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-)
diff --git a/vis.c b/vis.c index cec216f..c515927 100644 --- a/vis.c +++ b/vis.c @@ -207,7 +207,6 @@ int vis_seq_print_text(struct seq_file *seq, void *offset) int vis_server = atomic_read(&bat_priv->vis_mode); size_t buff_pos, buf_size; char *buff; - int compare;
primary_if = primary_if_get_selected(bat_priv); if (!primary_if) @@ -228,14 +227,18 @@ int vis_seq_print_text(struct seq_file *seq, void *offset) entries = (struct vis_info_entry *) ((char *)packet + sizeof(*packet));
+ vis_data_insert_interface(packet->vis_orig, + &vis_if_list, true); + for (j = 0; j < packet->entries; j++) { if (entries[j].quality == 0) continue; - compare = - compare_eth(entries[j].src, packet->vis_orig); + if (compare_eth(entries[j].src, + packet->vis_orig)) + continue; vis_data_insert_interface(entries[j].src, &vis_if_list, - compare); + false); }
hlist_for_each_entry(entry, pos, &vis_if_list, list) { @@ -276,14 +279,18 @@ int vis_seq_print_text(struct seq_file *seq, void *offset) entries = (struct vis_info_entry *) ((char *)packet + sizeof(*packet));
+ vis_data_insert_interface(packet->vis_orig, + &vis_if_list, true); + for (j = 0; j < packet->entries; j++) { if (entries[j].quality == 0) continue; - compare = - compare_eth(entries[j].src, packet->vis_orig); + if (compare_eth(entries[j].src, + packet->vis_orig)) + continue; vis_data_insert_interface(entries[j].src, &vis_if_list, - compare); + false); }
hlist_for_each_entry(entry, pos, &vis_if_list, list) {
Acked-by: Simon Wunderlich siwu@hrz.tu-chemnitz.de
Your patch does the trick, although I think this ugly function could use a rewrite. First counting bytes and then allocating this size to do exactly the same thing again is not really good style. If you would like to volunteer to do this job (or plan to work more on vis), please tell me, otherwise I'll put it in on my TODO list.
Thanks Simon
On Sat, May 05, 2012 at 05:51:53PM +0200, Matthias Schiffer wrote:
The primary entry and the corresponding secondary entries are missing when there are no neighbors on the primary interface. This also causes the TT entries to miss and makes nodes with multiply secondary interface fall apart since there is no way to see they are related without a primary entry.
Fix this by always emitting a primary entry.
Signed-off-by: Matthias Schiffer mschiffer@universe-factory.net
vis.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-)
diff --git a/vis.c b/vis.c index cec216f..c515927 100644 --- a/vis.c +++ b/vis.c @@ -207,7 +207,6 @@ int vis_seq_print_text(struct seq_file *seq, void *offset) int vis_server = atomic_read(&bat_priv->vis_mode); size_t buff_pos, buf_size; char *buff;
int compare;
primary_if = primary_if_get_selected(bat_priv); if (!primary_if)
@@ -228,14 +227,18 @@ int vis_seq_print_text(struct seq_file *seq, void *offset) entries = (struct vis_info_entry *) ((char *)packet + sizeof(*packet));
vis_data_insert_interface(packet->vis_orig,&vis_if_list, true);for (j = 0; j < packet->entries; j++) { if (entries[j].quality == 0) continue;
compare =compare_eth(entries[j].src, packet->vis_orig);
if (compare_eth(entries[j].src,packet->vis_orig))continue; vis_data_insert_interface(entries[j].src, &vis_if_list,
compare);
false); } hlist_for_each_entry(entry, pos, &vis_if_list, list) {@@ -276,14 +279,18 @@ int vis_seq_print_text(struct seq_file *seq, void *offset) entries = (struct vis_info_entry *) ((char *)packet + sizeof(*packet));
vis_data_insert_interface(packet->vis_orig,&vis_if_list, true);for (j = 0; j < packet->entries; j++) { if (entries[j].quality == 0) continue;
compare =compare_eth(entries[j].src, packet->vis_orig);
if (compare_eth(entries[j].src,packet->vis_orig))continue; vis_data_insert_interface(entries[j].src, &vis_if_list,
compare);
false); } hlist_for_each_entry(entry, pos, &vis_if_list, list) {-- 1.7.10.1
On 05/06/2012 10:14 PM, Simon Wunderlich wrote:
Your patch does the trick, although I think this ugly function could use a rewrite. First counting bytes and then allocating this size to do exactly the same thing again is not really good style. If you would like to volunteer to do this job (or plan to work more on vis), please tell me, otherwise I'll put it in on my TODO list.
While I'am already at it, I guess I can also volunteer to do some more vis work :)
Besides cleanup, are there more ideas about the vis? A nice feature I can think about would be adding some freely chosen identification string (for example the hostname) to the vis data, this could make big graphs much more readable. I wonder though if this would be possible without breaking compatiblity.
I have some questions about the code though:
- Is there any reason vis_seq_print_text() allocates a buffer at all instead of just printing the data directy into the seq_file? Looking at the seq_printf implementation, there doesn't seem to be a problem calling it while holding the lock. - In many places in the vis code hlist_for_each_entry_rcu() is used to iterate over the hash lists, even though all access to vis_hash is guarded by the vis_hash_lock, so it seems to be okay to just use hlist_for_each_entry(). In some functions, vis_seq_print_text() being one of them, rcu_read_lock/unlock pairs could be removed as well with this change. Do I overlook something?
I'll also drop by the batman IRC channel.
Matthias
On Monday, May 07, 2012 12:35:31 Matthias Schiffer wrote:
On 05/06/2012 10:14 PM, Simon Wunderlich wrote:
Your patch does the trick, although I think this ugly function could use a rewrite. First counting bytes and then allocating this size to do exactly the same thing again is not really good style. If you would like to volunteer to do this job (or plan to work more on vis), please tell me, otherwise I'll put it in on my TODO list.
While I'am already at it, I guess I can also volunteer to do some more vis work :)
Great! The vis code will profit from some love. :)
Besides cleanup, are there more ideas about the vis? A nice feature I can think about would be adding some freely chosen identification string (for example the hostname) to the vis data, this could make big graphs much more readable. I wonder though if this would be possible without breaking compatiblity.
If you wish to replace the mac addresses with readable name you can simply use a bat-hosts file to tell batctl which mac address should be replaced. Check the bat-hosts section in our batctl online manpage: http://downloads.open-mesh.org/batman/manpages/batctl.8.html
- Is there any reason vis_seq_print_text() allocates a buffer at all
instead of just printing the data directy into the seq_file? Looking at the seq_printf implementation, there doesn't seem to be a problem calling it while holding the lock.
The output can directly printed only as long as no spinlock is held. Spinlocks are not allowed to sleep. All other *_print_text() functions have been converted to directly call seq_printf() after we converted the hash to use RCU locking instead of spinlocks. For this to work in the vis code as well the vis_hash_lock spinlock has to be removed ..
- In many places in the vis code hlist_for_each_entry_rcu() is used to
iterate over the hash lists, even though all access to vis_hash is guarded by the vis_hash_lock, so it seems to be okay to just use hlist_for_each_entry(). In some functions, vis_seq_print_text() being one of them, rcu_read_lock/unlock pairs could be removed as well with this change. Do I overlook something?
The vis code is only half way through the spinlock to RCU conversion. Before you remove the vis_hash_lock anywhere you have to ensure that the data structures are properly protected by other means. If half of the code uses one lock while the rest uses another we'll certainly run into trouble.
If you ask me what could/should be done, I'd say: * Replacing vis_hash_lock with RCU locking. * The vis code should be reviewed to remove code duplication. It implements quite a number of things already present in the main batman-adv code (for example its packet queue). * Boring code cleanups like using newly added macros / functions to make the code more readable.
I'll also drop by the batman IRC channel.
See you there!
Regards, Marek
On 05/07/2012 08:40 AM, Marek Lindner wrote:
If you wish to replace the mac addresses with readable name you can simply use a bat-hosts file to tell batctl which mac address should be replaced. Check the bat-hosts section in our batctl online manpage: http://downloads.open-mesh.org/batman/manpages/batctl.8.html
I know about bat-hosts, but this only works when the host displaying the data knows all the MAC address/hostname mappings. For community meshs like Freifunk networks it would be much nicer to allow each host to supply its own hostname for vis in my opinion. I think one possibility to add this without breaking compatiblity would be adding a hostname record after the neighbor entries in the vis packets.
It would be nice to convert the reserved field in the vis packet stucture to a flags field for things like this, but AFAICS the current code never sets this to zero, but this is a byte of uninitialized memory that is sent over the network - but again, I might be overlooking things as I'm not yet familar with the code.
Matthias
On Monday, May 07, 2012 01:10:19 PM Matthias Schiffer wrote:
On 05/07/2012 08:40 AM, Marek Lindner wrote:
If you wish to replace the mac addresses with readable name you can simply use a bat-hosts file to tell batctl which mac address should be replaced. Check the bat-hosts section in our batctl online manpage: http://downloads.open-mesh.org/batman/manpages/batctl.8.html
I know about bat-hosts, but this only works when the host displaying the data knows all the MAC address/hostname mappings. For community meshs like Freifunk networks it would be much nicer to allow each host to supply its own hostname for vis in my opinion. I think one possibility to add this without breaking compatiblity would be adding a hostname record after the neighbor entries in the vis packets.
It would be nice to convert the reserved field in the vis packet stucture to a flags field for things like this, but AFAICS the current code never sets this to zero, but this is a byte of uninitialized memory that is sent over the network - but again, I might be overlooking things as I'm not yet familar with the code.
Please don't think too much about the current packet types... there will be changes anyway [1,2]
And maybe you could also use the DHT implementation [3] for your distributed dns [4] (when you don't want to use already available stuff [5])
Kind regards, Sven
[1] http://www.open-mesh.org/wiki/batman-adv/BackwardsCompatibility [2] http://www.open-mesh.org/wiki/batman-adv/Packet-types [3] http://www.open-mesh.org/wiki/batman-adv/DistributedArpTable [4] https://projects.universe-factory.net/projects/ngmrp/wiki [5] http://www.multicastdns.org/
On Monday, May 07, 2012 19:10:19 Matthias Schiffer wrote:
On 05/07/2012 08:40 AM, Marek Lindner wrote:
If you wish to replace the mac addresses with readable name you can simply use a bat-hosts file to tell batctl which mac address should be replaced. Check the bat-hosts section in our batctl online manpage: http://downloads.open-mesh.org/batman/manpages/batctl.8.html
I know about bat-hosts, but this only works when the host displaying the data knows all the MAC address/hostname mappings. For community meshs like Freifunk networks it would be much nicer to allow each host to supply its own hostname for vis in my opinion. I think one possibility to add this without breaking compatiblity would be adding a hostname record after the neighbor entries in the vis packets.
Flooding the network with arbitrary data like hostnames, service announcements, weather info, etc comes up from time to time but it often boils down to the question: Why does it have to be in the kernel ? There are several ways to implement this feature in user space today without changing the kernel code.
Regards, Marek
On 05/08/2012 08:04 AM, Marek Lindner wrote:
On Monday, May 07, 2012 19:10:19 Matthias Schiffer wrote:
On 05/07/2012 08:40 AM, Marek Lindner wrote:
If you wish to replace the mac addresses with readable name you can simply use a bat-hosts file to tell batctl which mac address should be replaced. Check the bat-hosts section in our batctl online manpage: http://downloads.open-mesh.org/batman/manpages/batctl.8.html
I know about bat-hosts, but this only works when the host displaying the data knows all the MAC address/hostname mappings. For community meshs like Freifunk networks it would be much nicer to allow each host to supply its own hostname for vis in my opinion. I think one possibility to add this without breaking compatiblity would be adding a hostname record after the neighbor entries in the vis packets.
Flooding the network with arbitrary data like hostnames, service announcements, weather info, etc comes up from time to time but it often boils down to the question: Why does it have to be in the kernel ? There are several ways to implement this feature in user space today without changing the kernel code.
Regards, Marek
I see your point, but then one could also ask why the visualization has to be in the kernel at all...
Matthias
supply its own hostname for vis in my opinion. I think one possibility to add this without breaking compatiblity would be adding a hostname record after the neighbor entries in the vis packets.
Flooding the network with arbitrary data like hostnames, service announcements, weather info, etc comes up from time to time but it often boils down to the question: Why does it have to be in the kernel ? There are several ways to implement this feature in user space today without changing the kernel code.
Regards, Marek
I see your point, but then one could also ask why the visualization has to be in the kernel at all...
visualization just means collecting information batman is already handling. neighbour information is essential to normal working, and generating a "vd dot" just exports that info. hostnames, (ip addresses, etc) on the other hand, are not of interest to current code
That said, I would also love to find a solution to this, since maintaining the /etc/bat-hosts in the nodes simply doesn't escalate in my opinion, and Marek' suggestion about "several ways to implement this feature in user space today" is not entirely clear to me: mDNS solutions like avahi translate between hostnames and ips, while batman visualization deals with MACs, and those cannot necessarily be obtained from IPs (in some cases, ARP will help, but won't work in interfaces without IPs)
maybe i'm ignoring some MAC discovery protocol?
Cheers,
Guido
On Wednesday, May 09, 2012 04:52:18 Guido Iribarren wrote:
I see your point, but then one could also ask why the visualization has to be in the kernel at all...
That is a valid question and has been debated several times already. The vis server is a borderline case - the consensus is/was that it's more convenient to have it in the kernel together with the routing protocol, so that it could easily tap into the routing data and make good use of it. Obviously, that also could be done in user space while all relevant data is be exported but it also creates enough burden to outweigh the "kernel bloat".
visualization just means collecting information batman is already handling. neighbour information is essential to normal working, and generating a "vd dot" just exports that info. hostnames, (ip addresses, etc) on the other hand, are not of interest to current code
Correct.
That said, I would also love to find a solution to this, since maintaining the /etc/bat-hosts in the nodes simply doesn't escalate in my opinion, and Marek' suggestion about "several ways to implement this feature in user space today" is not entirely clear to me: mDNS solutions like avahi translate between hostnames and ips, while batman visualization deals with MACs, and those cannot necessarily be obtained from IPs (in some cases, ARP will help, but won't work in interfaces without IPs)
I don't want to endorse a specific solution. Batman-adv creates a single ethernet broadcast domain. Every simple broadcasting tool will work across the entire mesh network. You could even write your own simple script that just broadcasts the hostname / ip address / $whatever. On the other end you have a script that listens to the broadcasts to populate your bat-hosts file / database / etc.
Regards, Marek
Hi Guido,
On 05/08/2012 10:52 PM, Guido Iribarren wrote:
mDNS solutions like avahi translate between hostnames and ips, while batman visualization deals with MACs, and those cannot necessarily be obtained from IPs (in some cases, ARP will help, but won't work in interfaces without IPs)
maybe i'm ignoring some MAC discovery protocol?
While reading about TLV's on wikipedia[1], I came across a MAC discovery protocol[2], LLDP, which eventually lead me to OpenLLDP[3]. If this also works on 802.11, I think we have a winner :)
Happy hacking!
[1] http://en.wikipedia.org/wiki/Type-length-value [2] http://en.wikipedia.org/wiki/Link_Layer_Discovery_Protocol [3] http://openlldp.sourceforge.net/
On 05/09/2012 06:10 PM, Martin Hundebøll wrote:
While reading about TLV's on wikipedia[1], I came across a MAC discovery protocol[2], LLDP, which eventually lead me to OpenLLDP[3]. If this also works on 802.11, I think we have a winner :)
Okay, so I tested 4 open-source LLDP implementations yesterday [1-4]... while LLDP works fine over batman-adv, I didn't find it very useful.
1st, implementations [1-3] aren't very configurable, and [4], an implementation by Intel, is hard to configure and *huge* (stripped binary ~400KB). Most of the implementations don't support announcing IPv6 addresses (I'm not sure which anymore), although that isn't really an important feature for me, as I'd use it mostly to announce hostnames for the MAC addresses, but I consider software that only supports IPv4 as deprecated. Also, most of these projects seem to be dead for years.
2nd, I think without batman-adv specific extensions LLDP isn't very useful, as the LLDP daemon only cares about the bat0/bridge MAC addresses, and not the hardif MAC addresses which are visible in the vis. The softif MAC addresses are shown in the vis as TT records, of course, but as one can't really discern between the adresses of the node itself and those of other clients on the bridge, it's rather useless in my opinion.
Matthias
[1] http://openlldp.sourceforge.net/ [2] https://code.google.com/p/ladvd/ [3] https://github.com/vincentbernat/lldpd/wiki [4] http://www.open-lldp.org/open-lldp
On Friday, May 11, 2012 03:47:30 Matthias Schiffer wrote:
1st, implementations [1-3] aren't very configurable, and [4], an implementation by Intel, is hard to configure and *huge* (stripped binary ~400KB). Most of the implementations don't support announcing IPv6 addresses (I'm not sure which anymore), although that isn't really an important feature for me, as I'd use it mostly to announce hostnames for the MAC addresses, but I consider software that only supports IPv4 as deprecated. Also, most of these projects seem to be dead for years.
2nd, I think without batman-adv specific extensions LLDP isn't very useful, as the LLDP daemon only cares about the bat0/bridge MAC addresses, and not the hardif MAC addresses which are visible in the vis. The softif MAC addresses are shown in the vis as TT records, of course, but as one can't really discern between the adresses of the node itself and those of other clients on the bridge, it's rather useless in my opinion.
I understand all reason except for the last one. What are you trying to achieve that you need to auto-discover all batman-adv interfaces ? Are you trying to replace vis ? As far as I understood Martin was trying to point you to a protocol which broadcasts arbitrary data on layer2 (similar to mDNS on layer3).
Regards, Marek
On 05/10/2012 10:19 PM, Marek Lindner wrote:
On Friday, May 11, 2012 03:47:30 Matthias Schiffer wrote:
2nd, I think without batman-adv specific extensions LLDP isn't very useful, as the LLDP daemon only cares about the bat0/bridge MAC addresses, and not the hardif MAC addresses which are visible in the vis. The softif MAC addresses are shown in the vis as TT records, of course, but as one can't really discern between the adresses of the node itself and those of other clients on the bridge, it's rather useless in my opinion.
I understand all reason except for the last one. What are you trying to achieve that you need to auto-discover all batman-adv interfaces ? Are you trying to replace vis ? As far as I understood Martin was trying to point you to a protocol which broadcasts arbitrary data on layer2 (similar to mDNS on layer3).
I'm trying to merge vis data with other data, so I can create a map which contains node information as well as link qualify information et cetera. To archive this, I need a reliable way to relate batman-adv vis nodes with LLDP nodes.
Matthias
On Monday, May 07, 2012 06:35:31 AM Matthias Schiffer wrote: [....]
I have some questions about the code though:
- Is there any reason vis_seq_print_text() allocates a buffer at all instead
of just printing the data directy into the seq_file? Looking at the seq_printf implementation, there doesn't seem to be a problem calling it while holding the lock.
This is something which came from an old... old... old implementation. It didn't use debugfs and seq_printf and therefore stupid tricks had to be used. Actually, the current implementation is broken and has to be changed (but no one wanted to touch the vis code).
- In many places in the vis code
hlist_for_each_entry_rcu() is used to iterate over the hash lists, even though all access to vis_hash is guarded by the vis_hash_lock, so it seems to be okay to just use hlist_for_each_entry(). In some functions, vis_seq_print_text() being one of them, rcu_read_lock/unlock pairs could be removed as well with this change. Do I overlook something?
I think it would be better to reduce the spin-locking and change the code to use rcu_read_lock. But maybe we have to think a lot about the data structures to generate consistent output... so maybe it is not possible (when also wanting it implemented in an efficient way).
Kind regards, Sven
On Saturday, May 05, 2012 23:51:53 Matthias Schiffer wrote:
The primary entry and the corresponding secondary entries are missing when there are no neighbors on the primary interface. This also causes the TT entries to miss and makes nodes with multiply secondary interface fall apart since there is no way to see they are related without a primary entry.
Fix this by always emitting a primary entry.
Applied in revision 73cc762.
Thanks, Marek
b.a.t.m.a.n@lists.open-mesh.org
-
Guido Iribarren -
Marek Lindner -
Martin Hundebøll -
Matthias Schiffer -
Simon Wunderlich -
Sven Eckelmann