Wireshark · Ethereal-dev: [Ethereal-dev] H323 Analysis cleanup

[Lars Roland <lars.roland@xxxxxxx>]

Hello all,

Having a closer look at the new and very useful H323 Call Analysis 
feature, I have found some bugs and unnecessarily complicated code for 
managing the registration of the tap listeners. So I decided to rewrite 
this part of the source code. This part of the code is much smaller now. 
Unnecessary and wrong calls of register_ethereal_tap() and 
register_tap_listener_xxx() have been removed or replaced.

I also fixed a bug with RAS Messages.

Please check in.

H323 Call Analysis works currently only with direct calls, support for 
calls involving a gatekeeper is missing.
The mechanism for matching h323 packets to a specific call is currently 
based on ip addresses and port numbers. At least for h225 packets we 
should use the call id instead.

BTW the usage of register_ethereal_tap() in the rtp taps needs to be 
fixed, too.

Regards,
Lars


Index: gtk/h323_analysis.c
===================================================================
--- gtk/h323_analysis.c	(revision 12146)
+++ gtk/h323_analysis.c	(working copy)
@@ -41,7 +41,6 @@
 
 #include "util.h"
 #include <epan/tap.h>
-#include "register.h"
 
 #include <epan/dissectors/packet-h225.h>
 #include <epan/dissectors/packet-h245.h>
Index: gtk/h323_conversations_dlg.c
===================================================================
--- gtk/h323_conversations_dlg.c	(revision 12146)
+++ gtk/h323_conversations_dlg.c	(working copy)
@@ -31,6 +31,8 @@
 #  include <config.h>
 #endif
 
+#include "register.h"
+
 #include "h323_conversations_dlg.h"
 #include "h323_conversations.h"
 #include "h323_analysis.h"
@@ -528,28 +530,38 @@
 	}
 }
 
-
-/****************************************************************************/
-/* entry point when called via the GTK menu */
-void h323conversations_launch(GtkWidget *w _U_, gpointer data _U_)
+/* init function for tap */
+static void
+h323conversations_init_tap(char *dummy _U_)
 {
 	/* Register the tap listener */
-	register_tap_listener_h225_conversations();
-	register_tap_listener_h245_conversations();
+	h225conversations_init_tap();
+	h245conversations_init_tap();
 
 	/* Scan for H323 conversations conversationss (redissect all packets) */
-	h323conversations_scan();
+	retap_packets(&cfile);
 
 	/* Show the dialog box with the list of conversationss */
 	h323conversations_dlg_show(h323conversations_get_info()->strinfo_list);
 
 	/* Tap listener will be removed and cleaned up in h323conversations_on_destroy */
+	
 }
 
+
 /****************************************************************************/
+/* entry point when called via the GTK menu */
+void h323conversations_launch(GtkWidget *w _U_, gpointer data _U_)
+{
+	h323conversations_init_tap("");
+}
+
+/****************************************************************************/
 void
 register_tap_listener_h323_conversations_dlg(void)
 {
+	register_ethereal_tap("h323,conv",h323conversations_init_tap);
+	
 	register_tap_menu_item("H.323/Show All H323 Conversations...", REGISTER_TAP_GROUP_NONE,
 	    h323conversations_launch, NULL, NULL, NULL);
 }
Index: gtk/Makefile.common
===================================================================
--- gtk/Makefile.common	(revision 12146)
+++ gtk/Makefile.common	(working copy)
@@ -53,6 +53,7 @@
 	goto_dlg.c	\
 	gtk_stat_util.c	\
 	gui_prefs.c	\
+	h323_analysis.c	\
 	h323_conversations.c	\
 	help_dlg.c	\
 	hostlist_table.c \
@@ -104,7 +105,6 @@
 	gsm_map_summary.c	\
 	h225_counter.c	\
 	h225_ras_srt.c	\
-	h323_analysis.c	\
 	h323_conversations_dlg.c	\
 	hostlist_eth.c \
 	hostlist_fc.c \
Index: gtk/h323_conversations.c
===================================================================
--- gtk/h323_conversations.c	(revision 12146)
+++ gtk/h323_conversations.c	(working copy)
@@ -37,7 +37,6 @@
 #include "globals.h"
 
 #include <epan/tap.h>
-#include "register.h"
 #include <epan/dissectors/packet-h225.h>
 #include <epan/dissectors/packet-h245.h>
 
@@ -57,7 +56,7 @@
 /****************************************************************************/
 /* the one and only global h323conversations_tapinfo_t structure */
 static h323conversations_tapinfo_t the_tapinfo_struct =
-	{0, NULL, 0, NULL, 0, FALSE, FALSE, 0, 0, 0};
+	{0, NULL, 0, NULL, 0, 0, 0, 0};
 
 /****************************************************************************/
 /* GCompareFunc style comparison function for _h323_conversations_info */
@@ -135,6 +134,10 @@
 	GList* list;
 
 	h225_packet_info *pi = h225info;
+	
+	/* TODO: evaluate RAS Messages. Just ignore them for now*/
+	if(pi->msg_type==H225_RAS)
+		return 0;
 
 	/* gather infos on the conversations this packet is part of */
 	g_memmove(&(tmp_strinfo.src_addr), pinfo->src.data, 4);
@@ -233,27 +236,7 @@
 		return 1;  /* refresh output */
 }
 
-/****************************************************************************/
-/* scan for h323 conversationss */
-void h323conversations_scan(void)
-{
-	gboolean was_h225_registered = the_tapinfo_struct.is_h225_registered;
-	gboolean was_h245_registered = the_tapinfo_struct.is_h245_registered;
 
-	if (!the_tapinfo_struct.is_h225_registered)
-		register_tap_listener_h225_conversations();
-	if (!the_tapinfo_struct.is_h245_registered)
-		register_tap_listener_h245_conversations();
-
-	retap_packets(&cfile);
-
-	if (!was_h225_registered)
-		remove_tap_listener_h225_conversations();
-	if (!was_h245_registered)
-		remove_tap_listener_h245_conversations();
-}
-
-
 /****************************************************************************/
 const h323conversations_tapinfo_t* h323conversations_get_info(void)
 {
@@ -264,44 +247,19 @@
 /****************************************************************************/
 /* TAP INTERFACE */
 /****************************************************************************/
-
+static gboolean have_h225_tap_listener=FALSE;
 /****************************************************************************/
-static void
-h225conversations_init_tap(char *dummy _U_)
-{
-	/* XXX: never called? */
-}
-
-
-/* XXX just copied from gtk/rpc_stat.c */
-void protect_thread_critical_region(void);
-void unprotect_thread_critical_region(void);
-
-/****************************************************************************/
 void
-remove_tap_listener_h225_conversations(void)
+h225conversations_init_tap(void)
 {
-	if (the_tapinfo_struct.is_h225_registered) {
-		protect_thread_critical_region();
-		remove_tap_listener(&the_tapinfo_struct);
-		unprotect_thread_critical_region();
-
-		the_tapinfo_struct.is_h225_registered = FALSE;
-	}
-}
-
-
-/****************************************************************************/
-void
-register_tap_listener_h225_conversations(void)
-{
 	GString *error_string;
+	
+	h225conversations_reset(&the_tapinfo_struct);
 
-	if (!the_tapinfo_struct.is_h225_registered) {
-		register_ethereal_tap("h225", h225conversations_init_tap);
-
-		error_string = register_tap_listener("h225", &the_tapinfo_struct,
-			NULL,
+	if(have_h225_tap_listener==FALSE)
+	{
+		/* don't register tap listener, if we have it already */
+		error_string = register_tap_listener("h225", &the_tapinfo_struct, NULL,
 			(void*)h225conversations_reset, (void*)h225conversations_packet, (void*)h225conversations_draw);
 
 		if (error_string != NULL) {
@@ -310,14 +268,28 @@
 			g_string_free(error_string, TRUE);
 			exit(1);
 		}
-
-		the_tapinfo_struct.is_h225_registered = TRUE;
+		have_h225_tap_listener=TRUE;
 	}
 }
 
 
+/* XXX just copied from gtk/rpc_stat.c */
+void protect_thread_critical_region(void);
+void unprotect_thread_critical_region(void);
 
 /****************************************************************************/
+void
+remove_tap_listener_h225_conversations(void)
+{
+	protect_thread_critical_region();
+	remove_tap_listener(&the_tapinfo_struct);
+	unprotect_thread_critical_region();
+	
+	have_h225_tap_listener=FALSE;
+}
+
+
+/****************************************************************************/
 /* ***************************TAP for h245 **********************************/
 /****************************************************************************/
 
@@ -362,34 +334,16 @@
 }
 
 /****************************************************************************/
-static void
-h245conversations_init_tap(char *dummy _U_)
-{
-	/* XXX: never called? */
-}
+static gboolean have_h245_tap_listener=FALSE;
 
-/****************************************************************************/
 void
-remove_tap_listener_h245_conversations(void)
+h245conversations_init_tap(void)
 {
-	if (the_tapinfo_struct.is_h245_registered) {
-		protect_thread_critical_region();
-		remove_tap_listener(&the_tapinfo_struct);
-		unprotect_thread_critical_region();
-
-		the_tapinfo_struct.is_h245_registered = FALSE;
-	}
-}
-
-
-/****************************************************************************/
-void
-register_tap_listener_h245_conversations(void)
-{
 	GString *error_string;
-	if (!the_tapinfo_struct.is_h245_registered) {
-		register_ethereal_tap("h245", h245conversations_init_tap);
-
+	
+	if(have_h245_tap_listener==FALSE)
+	{ 
+		/* don't register tap listener, if we have it already */
 		error_string = register_tap_listener("h245", &the_tapinfo_struct,
 			NULL,
 			(void*)h245conversations_reset, (void*)h245conversations_packet, (void*)h245conversations_draw);
@@ -400,12 +354,24 @@
 			g_string_free(error_string, TRUE);
 			exit(1);
 		}
-
-		the_tapinfo_struct.is_h245_registered = TRUE;
+		have_h245_tap_listener=TRUE;
 	}
 }
 
+/****************************************************************************/
+void
+remove_tap_listener_h245_conversations(void)
+{
+	protect_thread_critical_region();
+	remove_tap_listener(&the_tapinfo_struct);
+	unprotect_thread_critical_region();
+	
+	have_h245_tap_listener=FALSE;
+}
 
+
+/****************************************************************************/
+
 void h245conversations_reset(h323conversations_tapinfo_t *tapinfo _U_)
 {
 	return;
Index: gtk/h323_conversations.h
===================================================================
--- gtk/h323_conversations.h	(revision 12146)
+++ gtk/h323_conversations.h	(working copy)
@@ -71,8 +71,6 @@
 	int     npackets;       /* total number of h323 packets of all conversationss */
 	h323_conversations_info_t* filter_conversations_fwd;  /* used as filter in some tap modes */
 	guint32 launch_count;   /* number of times the tap has been run */
-	gboolean is_h225_registered; /* if the tap listener is currently registered or not */
-	gboolean is_h245_registered; /* if the tap listener is currently registered or not */
 	int setup_packets;
 	int completed_calls;
 	int rejected_calls;
@@ -89,8 +87,8 @@
 * So whenever h323_conversations.c is added to the list of ETHEREAL_TAP_SRCs, the tap will be registered on startup.
 * If not, it will be registered on demand by the h323_conversationss and h323_analysis functions that need it.
 */
-void register_tap_listener_h225_conversations(void);
-void register_tap_listener_h245_conversations(void);
+void h225conversations_init_tap(void);
+void h245conversations_init_tap(void);
 
 /*
 * Removes the h323_conversationss tap listener (if not already done)
@@ -112,12 +110,6 @@
 void h245conversations_reset(h323conversations_tapinfo_t *tapinfo);
 
 /*
-* Scans all packets for H225 conversationss and updates the H225 conversationss list.
-* (redissects all packets)
-*/
-void h323conversations_scan(void);
-
-/*
 * Marks all packets belonging to conversations.
 * (can be NULL)
 * (redissects all packets)