[Cvetan Ivanov <zezo@xxxxxxxxx>]

Hi,


This patch is not intended for addition to the source yet, I've just 
included it as illustration at this point. Would like to get some 
comments first.


What's changed:

1) added few more Cisco q931+ messages/IEs.
This leads to the question if dissecting q931+ should be made optional 
as those messages are invalid for normal q931, and marking them as 
unknow may be preferable if used with normal isdn trace
So should we have option for this?

2) fixed support for NFAS interfaces
The octet variable was overwritten in case that
 if (octet & Q931_INTERFACE_IDENTIFIED)
and then the next check fails and disecting the channel id is skipped

2.5) renamed Q931_NOT_BASIC_CHANNEL to Q931_NOT_BASIC_INTERFACE - the 
bit being called "Interface type"

3) Added basic dissecting of the channel ID in case its identified by 
number.

If it's bitmap the length depends on wether the interface is E1 or T1
So we could try to derive it from the total IE length (dissect as bytes 
to the end of the IE)?

Doing this I've also added the channel as protocol field, which may or 
may not have much sense - it can't be used to correlate call messages, 
but might be useful in other cases, like trying to identify faulty hardware

4) Interface ID:

as for the comment:

    * XXX - do we want to strip off the 8th bit on the
    * last octet of the interface identifier?

- we should. after all it's indicator bit, not part of the encoded data, 
but what's the correct way to do it:

- Interpret it as int, and add it as field, so we could flter on that?
- Just copy the bytes to a buffer and strip the bit, then add to the tree?

Any idea about how long that field is in real world implementations?
The Interface ID is too loosely defined in q931:

Interface identifier (octet 3.1)
        Binary code assigned to the interface at subscription time. At 
subscription time, the binary code for the interface will specify the 
number of octets to be used and the content of each octet.


Best regards,

Cvetan

Index: packet-q931.c
===================================================================
--- packet-q931.c	(revision 14415)
+++ packet-q931.c	(working copy)
@@ -94,6 +94,7 @@
 static int hf_q931_segment_too_long_segment = -1;
 static int hf_q931_segment_error = -1;
 static int hf_q931_reassembled_in = -1; 
+static int hf_q931_channel = -1;
 
 static gint ett_q931 					= -1;
 static gint ett_q931_ie 				= -1;
@@ -145,6 +146,7 @@
 #define	Q931_CONNECT		0x07
 #define	Q931_CONNECT_ACK	0x0F
 #define	Q931_PROGRESS		0x03
+#define	Q931_COT_CONF		0x04
 #define	Q931_SETUP		0x05
 #define	Q931_SETUP_ACK		0x0D
 #define	Q931_HOLD		0x24
@@ -186,6 +188,7 @@
 	{ Q931_CONNECT,			"CONNECT" },
 	{ Q931_CONNECT_ACK,		"CONNECT ACKNOWLEDGE" },
 	{ Q931_PROGRESS,		"PROGRESS" },
+	{ Q931_COT_CONF,		"COT CONF" },
 	{ Q931_SETUP,			"SETUP" },
 	{ Q931_SETUP_ACK,		"SETUP ACKNOWLEDGE" },
 	{ Q931_HOLD,			"HOLD" },
@@ -306,6 +309,11 @@
 #define	Q931_IE_CUG			0x47	/* Closed user group */
 #define	Q931_IE_REVERSE_CHARGE_IND	0x4A	/* Reverse charging indication */
 #define	Q931_IE_CONNECTED_NUMBER_DEFAULT        0x4C	/* Connected Number */
+#define	Q931_IE_COT_OPERATION		0x61	/* q931+ COT Operation */
+#define	Q931_IE_COT_DURATION		0x62	/* q931+ COT Duration */
+#define	Q931_IE_COT_RESULT		0x63	/* q931+ COT Result */
+#define	Q931_IE_COT_IN_TONE		0x64	/* q931+ COT In-Tone */
+#define	Q931_IE_COT_OUT_TONE		0x65	/* q931+ COT Out-Tone */
 #define	Q931_IE_INTERFACE_SERVICE	0x66	/* q931+ Interface Service */
 #define	Q931_IE_CHANNEL_STATUS		0x67	/* q931+ Channel Status */
 #define	Q931_IE_VERSION_INFO		0x68	/* q931+ Version Info */
@@ -388,6 +396,11 @@
 	{ Q931_IE_REVERSE_CHARGE_IND,		"Reverse charging indication" },
 	{ Q931_IE_CONNECTED_NUMBER_DEFAULT,     "Connected number" },
 	{ Q931_IE_INTERFACE_SERVICE,		"Interface Service" },
+	{ Q931_IE_COT_OPERATION,		"COT Operation" },
+	{ Q931_IE_COT_DURATION,			"COT Duration" },
+	{ Q931_IE_COT_RESULT,			"COT Result" },
+	{ Q931_IE_COT_IN_TONE,			"COT In-Tone" },
+	{ Q931_IE_COT_OUT_TONE,			"COT Out-Tone" },
 	{ Q931_IE_CHANNEL_STATUS,		"Channel Status" },
 	{ Q931_IE_VERSION_INFO,			"Version Info" },
 	{ Q931_IE_CALLING_PARTY_NUMBER,		"Calling party number" },
@@ -1404,7 +1417,7 @@
  * Dissect a Channel identification information element.
  */
 #define	Q931_INTERFACE_IDENTIFIED	0x40
-#define	Q931_NOT_BASIC_CHANNEL		0x20
+#define	Q931_NOT_BASIC_INTERFACE		0x20
 
 static const value_string q931_basic_channel_selection_vals[] = {
 	{ 0x00, "No channel" },
@@ -1435,6 +1448,7 @@
 dissect_q931_channel_identification_ie(tvbuff_t *tvb, int offset, int len,
     proto_tree *tree)
 {
+	guint8 channel_flags;
 	guint8 octet;
 	int identifier_offset;
 	int identifier_len;
@@ -1442,34 +1456,34 @@
 
 	if (len == 0)
 		return;
-	octet = tvb_get_guint8(tvb, offset);
+	channel_flags = tvb_get_guint8(tvb, offset);
 	proto_tree_add_text(tree, tvb, offset, 1,
 	    "Interface %s identified",
-	    (octet & Q931_INTERFACE_IDENTIFIED) ? "explicitly" : "implicitly");
+	    (channel_flags & Q931_INTERFACE_IDENTIFIED) ? "explicitly" : "implicitly");
 	proto_tree_add_text(tree, tvb, offset, 1,
 	    "%s interface",
-	    (octet & Q931_NOT_BASIC_CHANNEL) ? "Not basic" : "Basic");
+	    (channel_flags & Q931_NOT_BASIC_INTERFACE) ? "Not basic" : "Basic");
 	proto_tree_add_text(tree, tvb, offset, 1,
 	    "Indicated channel is %s",
-	    (octet & 0x08) ? "required" : "preferred");
+	    (channel_flags & 0x08) ? "required" : "preferred");
 	proto_tree_add_text(tree, tvb, offset, 1,
 	    "Indicated channel is %sthe D-channel",
-	    (octet & 0x04) ? "" : "not ");
-	if (octet & Q931_NOT_BASIC_CHANNEL) {
+	    (channel_flags & 0x04) ? "" : "not ");
+	if (channel_flags & Q931_NOT_BASIC_INTERFACE) {
 		proto_tree_add_text(tree, tvb, offset, 1,
 		    "Channel selection: %s",
-		    val_to_str(octet & 0x03, q931_not_basic_channel_selection_vals,
+		    val_to_str(channel_flags & 0x03, q931_not_basic_channel_selection_vals,
 		      "Unknown (0x%X)"));
 	} else {
 		proto_tree_add_text(tree, tvb, offset, 1,
 		    "Channel selection: %s",
-		    val_to_str(octet & 0x03, q931_basic_channel_selection_vals,
+		    val_to_str(channel_flags & 0x03, q931_basic_channel_selection_vals,
 		      "Unknown (0x%02X)"));
 	}
 	offset += 1;
 	len -= 1;
 
-	if (octet & Q931_INTERFACE_IDENTIFIED) {
+	if (channel_flags & Q931_INTERFACE_IDENTIFIED) {
 		identifier_offset = offset;
 		identifier_len = 0;
 		do {
@@ -1494,7 +1508,7 @@
 		}
 	}
 
-	if (octet & Q931_NOT_BASIC_CHANNEL) {
+	if (channel_flags & Q931_NOT_BASIC_INTERFACE) {
 		if (len == 0)
 			return;
 		octet = tvb_get_guint8(tvb, offset);
@@ -1519,9 +1533,24 @@
 		    (octet & Q931_IS_SLOT_MAP) ? "Map element" : "Channel",
 		    val_to_str(octet & 0x0F, q931_element_type_vals,
 		        "Unknown (0x%02X)"));
+		
+	        offset += 1;
+                len -= 1;
+		if (len == 0)
+	 	    return;
 
+		if (!(octet & Q931_IS_SLOT_MAP)) {
+		    proto_tree_add_uint(tree, hf_q931_channel, tvb, offset, 1, tvb_get_guint8(tvb, offset) & 0x7f);
+		    /* 
+		     * this octet is not expected to be extended, but (at least) Cisco
+		     * interprets the high bit as ext. indicator, and sets it to 1
+		     */
+		}
+
 		/*
-		 * XXX - dump the channel number or slot map.
+		 * XXX - else dump the slot map. but this depends on wether the interface
+		 * is E1 or T1 (3- or 4-byte map). probably there should be an option to 
+		 * indicate this
 		 */
 	}
 }
@@ -3220,6 +3249,10 @@
 		{ &hf_q931_reassembled_in,
 		  { "Reassembled Q.931 in frame", "q931.reassembled_in", FT_FRAMENUM, BASE_NONE, NULL, 0x0,
 			"This Q.931 message is reassembled in this frame", HFILL}}, 
+
+		{ &hf_q931_channel,
+		  { "Channel", "q931.channel", FT_UINT8, BASE_DEC, NULL, 0x0,
+			"", HFILL}}, 
 	};
 	static gint *ett[] = {
 		&ett_q931,