Re: [B.A.T.M.A.N.] [patch] adding subgraph-feature to vis-output in dot-file-format

[Simon Wunderlich <simon.wunderlich@s2003.tu-chemnitz.de>]

[-- Attachment #1: Type: text/plain, Size: 9793 bytes --]

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>

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


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]