[v2] 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 |   21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)
  

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 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
  

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