diff mbox

adding subgraph-feature to vis-output in dot-file-format

Message ID 20090830195553.GA2410@pandem0nium
State Accepted, archived
Headers show

Commit Message

Simon Wunderlich Aug. 30, 2009, 7:55 p.m. UTC
This is a small review of Linus patch. It contains:
 * minor style adaptions
 * optional compile flag VIS_SUBCLUSTERS_DISABLED to prevent vis parser breakages
 * sanity checks for kmalloc()

Linus, Andrew: please give it a try on your systems. I currently don't have 
machines set up to test, so this is a dry run. :)

Original message:

Pfeh... long night again, but here you go. With this patch,
batman-adv now supports the subgraphing-feature of the
dot-file-format. The vis-output can then be parsed with
"fdp -Tsvg -Gcharset=utf8 /proc/net/batman-adv/vis > test.svg"
for example.

Interfaces belonging to one BATMAN-node can be found
inside a box, the primary interface of a BATMAN-node is
double-circled. The output also got more detailed, in that
connections between nodes are being displayed with the exact
mac address of the used interface instead of replacing one end
with the primary interface's mac (see [0] for the problems I had
posted before).

Cheers, Linus

PS: Please check if I got that kmalloc stuff right.
PPS: I had to introduce a src-field in the vis-packet-struct,
therefore the compatibility version had to be increased as well.

[0] https://lists.open-mesh.net/pipermail/b.a.t.m.a.n/2009-August/002832.html

Signed-off-by: Simon Wunderlich <simon.wunderlich@s2003.tu-chemnitz.de>
From: Linus Lüssing <linus.luessing@web.de>

Comments

Andrew Lunn Aug. 30, 2009, 8:15 p.m. UTC | #1
> Linus, Andrew: please give it a try on your systems. I currently don't have 
> machines set up to test, so this is a dry run. :)

I don't yet know if i can. I'm off to Finland for two weeks, working
in the Finnish branch of the company i work for. I don't know how well
the internal FI-CH network works. I might not have easy access to our
test system. 

At least i can run checkpatch on it for coding style issues.

    Andrew
Linus Lüssing Aug. 30, 2009, 9:47 p.m. UTC | #2
Yep, works fine for me, compiled and run it on our three virtual
machines and looks the same as with my original patch. I also had
a quick look at the json output and I think the patch should not
have broken anything of that either, but a json-expert should have
a look at it again. Also it might make sense to introduce
something comparable to the subgraphing-feature of the
dot-file-format to the json output, wouldn't it? Otherwise you
will still see seperated graphs with json output if nodes are
having multiple interfaces. So someone might have to write and
commit a patch for that sometime in the future as well.

Thanks for reviewing the patch.

Cheers, Linus
Andrew Lunn Aug. 31, 2009, 5:28 a.m. UTC | #3
Hi Linus, Simon

One of the things on the TODO list for mainline is to strip out all
the graphvis/JSON formatting done by the kernel and put it into user
space, probably batctl, or another standalone tool, or maybe even a
little library...

Why:

1) No other file in /proc does any special formatting.
2) What happens when one tool wants graphvis and another JSON, at the same time?
3) What happens when you want a third, forth, fifth format etc...

Are either of you interested on working on this? 

I guess the first step is to design a file format for vis in proc.  I
would suggest space separated values. Maybe one line per
vis_into_entry? I would also suggest the quality value is 0-255, not a
real number. User space, which has float support, can then turn it
into a float if need be.

     Andrew
Linus Lüssing Sept. 1, 2009, 8:47 p.m. UTC | #4
Ok, now with a larger setup I found two smaller problems. I didn't
have the time to look at the source code again yet, but I'll just post
the things we noticed here.
First of all, not all interfaces get added to a subgraph. As an example
vis-output I have this [0] file, but unfortunately the lines in brackets
are not there, but should to avoid splitted graphs.
Secondly, it seems that a vis-client is just sending to the
vis-server its prefered interface plus quality to a neighbour, not
all possible links plus qualities to the neighbour. As a
network-administrator I'd like to have these information as well
to be able to check if an interface might be broken for example.

Cheers, Linus

[0]:
--------
digraph {
        "00:50:bf:d6:fd:b0" -> "00:22:b0:44:94:02" [label="1.0"]
        "ca:5f:0b:67:03:c7" -> "00:ff:4d:f1:04:c5" [label="4.553"]
        "00:50:bf:d6:fd:b0" -> "00:ff:82:74:9c:eb" [label="HNA"]
        subgraph "cluster_00:50:bf:d6:fd:b0" {
                "00:50:bf:d6:fd:b0" [peripheries=2]
                "ca:5f:0b:67:03:c7"
        }
        "00:22:b0:98:87:de" -> "00:22:b0:98:87:fa" [label="1.15"]
        "06:22:b0:98:87:dd" -> "06:22:b0:44:94:5d" [label="1.7"]
        "00:22:b0:98:87:de" -> "00:e0:29:39:34:d7" [label="1.15"]
        "06:22:b0:98:87:dd" -> "00:16:cb:ba:4a:f4" [label="HNA"]
        "06:22:b0:98:87:dd" -> "00:19:d2:7a:ba:21" [label="HNA"]
        "06:22:b0:98:87:dd" -> "00:ff:ca:4d:a5:e7" [label="HNA"]
        "06:22:b0:98:87:dd" -> "00:22:b0:98:87:dd" [label="HNA"]
        "06:22:b0:98:87:dd" -> "00:19:7e:6a:8a:27" [label="HNA"]
        subgraph "cluster_06:22:b0:98:87:dd" {
              ( "00:22:b0:98:87:de" )
                "06:22:b0:98:87:dd" [peripheries=2]
        }
        "00:22:b0:98:87:fa" -> "00:22:b0:98:87:de" [label="1.15"]
        "06:22:b0:98:87:f9" -> "06:22:b0:44:c6:59" [label="1.3"]
        "06:22:b0:98:87:f9" -> "06:22:b0:98:85:5b" [label="1.0"]
        "06:22:b0:98:87:f9" -> "06:22:b0:98:8c:33" [label="1.0"]
        "06:22:b0:98:87:f9" -> "00:22:b0:98:87:f9" [label="HNA"]
        "06:22:b0:98:87:f9" -> "00:ff:1b:01:51:12" [label="HNA"]
        subgraph "cluster_06:22:b0:98:87:f9" {
              ( "00:22:b0:98:87:fa" )
                "06:22:b0:98:87:f9" [peripheries=2]
        }
        "06:22:b0:44:c6:59" -> "06:22:b0:98:87:f9" [label="1.15"]
        "00:22:b0:44:c6:5a" -> "00:22:b0:98:8c:34" [label="1.85"]
        "06:22:b0:44:c6:59" -> "06:22:b0:98:85:5b" [label="1.0"]
        "06:22:b0:44:c6:59" -> "06:22:b0:98:8c:33" [label="1.32"]
        "06:22:b0:44:c6:59" -> "00:22:b0:44:c6:59" [label="HNA"]
        "06:22:b0:44:c6:59" -> "00:ff:d8:85:d7:fa" [label="HNA"]
        subgraph "cluster_06:22:b0:44:c6:59" {
                "06:22:b0:44:c6:59" [peripheries=2]
                "00:22:b0:44:c6:5a"
        }
        "00:22:b0:44:94:02" -> "00:50:bf:d6:fd:b0" [label="1.15"]
        "06:22:b0:44:94:01" -> "00:22:b0:44:94:01" [label="HNA"]
        "06:22:b0:44:94:01" -> "00:ff:af:cb:d6:64" [label="HNA"]
        subgraph "cluster_06:22:b0:44:94:01" {
              ( "00:22:b0:44:94:02" )
                "06:22:b0:44:94:01" [peripheries=2]
        }
        "06:22:b0:98:85:5b" -> "06:22:b0:44:c6:59" [label="1.15"]
        "06:22:b0:98:85:5b" -> "06:22:b0:98:8c:33" [label="1.62"]
        "06:22:b0:98:85:5b" -> "00:ff:00:02:cb:82" [label="HNA"]
        "06:22:b0:98:85:5b" -> "00:22:b0:98:85:5b" [label="HNA"]
        subgraph "cluster_06:22:b0:98:85:5b" {
                "06:22:b0:98:85:5b" [peripheries=2]
        }
}
diff mbox

Patch

Index: vis.c
===================================================================
--- vis.c	(revision 1418)
+++ vis.c	(working copy)
@@ -87,7 +87,7 @@ 
 	struct vis_info *d1, *d2;
 	d1 = data1;
 	d2 = data2;
-	return memcmp(d1->packet.vis_orig, d2->packet.vis_orig, ETH_ALEN) == 0;
+	return compare_orig(d1->packet.vis_orig, d2->packet.vis_orig);
 }
 
 /* hash function to choose an entry in a hash table of given size */
@@ -326,13 +326,14 @@ 
 
 	while (NULL != (hashit = hash_iterate(orig_hash, hashit))) {
 		orig_node = hashit->bucket->data;
-		if (orig_node->router != NULL &&
-		    memcmp(orig_node->router->addr,
-			   orig_node->orig, ETH_ALEN) == 0 &&
-		    orig_node->router->tq_avg > 0) {
+		if (orig_node->router != NULL
+			&& compare_orig(orig_node->router->addr, orig_node->orig)
+			&& orig_node->batman_if
+		    && orig_node->router->tq_avg > 0) {
 
 			/* fill one entry into buffer. */
 			entry = &entry_array[info->packet.entries];
+			memcpy(entry->src, orig_node->batman_if->net_dev->dev_addr, ETH_ALEN);
 			memcpy(entry->dest, orig_node->orig, ETH_ALEN);
 			entry->quality = orig_node->router->tq_avg;
 			info->packet.entries++;
@@ -351,6 +352,7 @@ 
 	while (NULL != (hashit = hash_iterate(hna_local_hash, hashit))) {
 		hna_local_entry = hashit->bucket->data;
 		entry = &entry_array[info->packet.entries];
+		memset(entry->src, 0, ETH_ALEN);
 		memcpy(entry->dest, hna_local_entry->addr, ETH_ALEN);
 		entry->quality = 0; /* 0 means HNA */
 		info->packet.entries++;
@@ -556,3 +558,4 @@ 
 	queue_delayed_work(bat_event_workqueue, &vis_timer_wq,
 			   (atomic_read(&vis_interval)/1000) * HZ);
 }
+
Index: vis.h
===================================================================
--- vis.h	(revision 1418)
+++ vis.h	(working copy)
@@ -33,6 +33,7 @@ 
 } __attribute__((packed));
 
 struct vis_info_entry {
+	uint8_t  src[ETH_ALEN];
 	uint8_t  dest[ETH_ALEN];
 	uint8_t  quality;	/* quality = 0 means HNA */
 } __attribute__((packed));
Index: packet.h
===================================================================
--- packet.h	(revision 1418)
+++ packet.h	(working copy)
@@ -26,7 +26,7 @@ 
 #define BAT_VIS       0x05
 
 /* this file is included by batctl which needs these defines */
-#define COMPAT_VERSION 7
+#define COMPAT_VERSION 8
 #define DIRECTLINK 0x40
 #define VIS_SERVER 0x20
 
Index: proc.c
===================================================================
--- proc.c	(revision 1418)
+++ proc.c	(working copy)
@@ -403,17 +403,59 @@ 
 	return single_open(file, proc_transt_global_read, NULL);
 }
 
+/* insert interface to the list of interfaces of one originator */
+
+static void proc_vis_insert_interface(const uint8_t *interface, 
+				      struct vis_if_list **if_entry, 
+				      bool primary)
+{
+	/* Did we get an empty list? (then insert imediately) */
+	if(*if_entry == NULL) {
+		*if_entry = kmalloc(sizeof(struct vis_if_list), GFP_KERNEL);
+		if (*if_entry == NULL)
+			return;
+
+		(*if_entry)->primary = primary;
+		(*if_entry)->next = NULL;
+		memcpy((*if_entry)->addr, interface, ETH_ALEN);
+	} else {
+		/* Do we already have this interface in our list? */
+		while (!compare_orig((*if_entry)->addr, (void *)interface)) {
+
+			/* Or did we reach the end (then append the interface) */
+			if ((*if_entry)->next == NULL) {
+				(*if_entry)->next = kmalloc(sizeof(struct vis_if_list), GFP_KERNEL);
+				if ((*if_entry)->next == NULL)
+					return;
+
+				memcpy((*if_entry)->next->addr, interface, ETH_ALEN);
+				(*if_entry)->next->primary = primary;
+				(*if_entry)->next->next = NULL;
+				break;
+			}
+			*if_entry = (*if_entry)->next;
+		}
+	}
+}
+/* read an entry  */
+
 static void proc_vis_read_entry(struct seq_file *seq,
 				struct vis_info_entry *entry,
-				char *from,
+				struct vis_if_list **if_entry,
+				uint8_t *vis_orig,
 				uint8_t current_format,
 				uint8_t first_line)
 {
+	char from[40];
 	char to[40];
 	int int_part, frac_part;
 
 	addr_to_string(to, entry->dest);
 	if (entry->quality == 0) {
+#ifndef VIS_SUBCLUSTERS_DISABLED
+		proc_vis_insert_interface(vis_orig, if_entry, true);
+#endif /* VIS_SUBCLUSTERS_DISABLED */
+		addr_to_string(from, vis_orig);
 		if (current_format == DOT_DRAW) {
 			seq_printf(seq, "\t\"%s\" -> \"%s\" [label=\"HNA\"]\n",
 				   from, to);
@@ -423,10 +465,17 @@ 
 				   (first_line ? "" : ",\n"), from, to);
 		}
 	} else {
+#ifndef VIS_SUBCLUSTERS_DISABLED
+		proc_vis_insert_interface(entry->src, if_entry, compare_orig(entry->src, vis_orig));
+#endif /* VIS_SUBCLUSTERS_DISABLED */
+		addr_to_string(from, entry->src);
+
 		/* kernel has no printf-support for %f? it'd be better to return
 		 * this in float. */
+
 		int_part = TQ_MAX_VALUE / entry->quality;
 		frac_part = 1000 * TQ_MAX_VALUE / entry->quality - int_part * 1000;
+
 		if (current_format == DOT_DRAW) {
 			seq_printf(seq,
 				   "\t\"%s\" -> \"%s\" [label=\"%d.%d\"]\n",
@@ -439,14 +488,19 @@ 
 	}
 }
 
+
 static int proc_vis_read(struct seq_file *seq, void *offset)
 {
 	struct hash_it_t *hashit = NULL;
 	struct vis_info *info;
 	struct vis_info_entry *entries;
-	char from[40];
+	struct vis_if_list *if_entries = NULL;
 	int i;
 	uint8_t current_format, first_line = 1;
+#ifndef VIS_SUBCLUSTERS_DISABLED
+	char tmp_addr_str[ETH_STR_LEN];
+	struct vis_if_list *tmp_if_next;
+#endif /* VIS_SUBCLUSTERS_DISABLED */
 
 	current_format = vis_format;
 
@@ -468,14 +522,37 @@ 
 		info = hashit->bucket->data;
 		entries = (struct vis_info_entry *)
 			((char *)info + sizeof(struct vis_info));
-		addr_to_string(from, info->packet.vis_orig);
 
 		for (i = 0; i < info->packet.entries; i++) {
-			proc_vis_read_entry(seq, &entries[i], from,
+			proc_vis_read_entry(seq, &entries[i], &if_entries,
+					    info->packet.vis_orig,
 					    current_format, first_line);
 			if (first_line)
 				first_line = 0;
 		}
+
+#ifndef VIS_SUBCLUSTERS_DISABLED
+		/* Generate subgraphs from the collected items */
+		if (current_format == DOT_DRAW) {
+
+			addr_to_string(tmp_addr_str, info->packet.vis_orig);
+			seq_printf(seq, "\tsubgraph \"cluster_%s\" \{\n", tmp_addr_str);
+			while (if_entries != NULL) {
+
+				addr_to_string(tmp_addr_str, if_entries->addr);
+				if (if_entries->primary)
+					seq_printf(seq, "\t\t\"%s\" [peripheries=2]\n", tmp_addr_str);
+				else
+					seq_printf(seq, "\t\t\"%s\"\n", tmp_addr_str);
+
+				/* ... and empty the list while doing this */
+				tmp_if_next = if_entries->next;
+				kfree(if_entries);
+				if_entries = tmp_if_next;
+			}
+			seq_printf(seq, "\t}\n");
+		}
+#endif /* VIS_SUBCLUSTERS_DISABLED */
 	}
 	spin_unlock(&vis_hash_lock);
 
Index: proc.h
===================================================================
--- proc.h	(revision 1418)
+++ proc.h	(working copy)
@@ -35,3 +35,13 @@ 
 
 void cleanup_procfs(void);
 int setup_procfs(void);
+
+/* While scanning for vis-entries of a particular vis-originator
+ * this list collects its interfaces to create a subgraph/cluster 
+ * out of them later
+ */
+struct vis_if_list {
+	uint8_t addr[ETH_ALEN];
+	bool primary;
+	struct vis_if_list *next;
+};
Index: main.c
===================================================================
--- main.c	(revision 1418)
+++ main.c	(working copy)
@@ -224,6 +224,8 @@ 
 		       addr[0], addr[1], addr[2], addr[3], addr[4], addr[5]);
 }
 
+/* returns 1 if they are the same originator */
+
 int compare_orig(void *data1, void *data2)
 {
 	return (memcmp(data1, data2, ETH_ALEN) == 0 ? 1 : 0);
Index: main.h
===================================================================
--- main.h	(revision 1418)
+++ main.h	(working copy)
@@ -25,6 +25,10 @@ 
 #define DRIVER_DEVICE "batman-adv"
 
 #define SOURCE_VERSION "0.2-beta"
+
+
+/* B.A.T.M.A.N. parameters */
+
 #define TQ_MAX_VALUE 255
 #define JITTER 20
 #define TTL 50		          /* Time To Live of broadcast messages */
@@ -82,6 +86,16 @@ 
 #define LOG_TYPE_BATMAN_NAME	"batman"
 #define LOG_TYPE_ROUTES_NAME	"routes"
 
+/*
+ *  Vis
+ */
+
+/* #define VIS_SUBCLUSTERS_DISABLED */
+
+/* 
+ * Kernel headers
+ */
+
 #include <linux/mutex.h>	/* mutex */
 #include <linux/module.h>	/* needed by all modules */
 #include <linux/netdevice.h>	/* netdevice */