Wireshark · Wireshark-dev: [Wireshark-dev] Suggested enhancements for WireShark

[Alexey Neyman <avn@xxxxxxxxxxx>]

Hi All,

Suggest 2 enhancements to WireShark:

1. (format_value.diff)

Make the proto_tree_add_*_format_value() functions prepend the bitmask 
indicators (those dots, zeros and ones) to bitfields. This makes it 
easier to format bitfields where only the value need to be formatted in 
some special way.

Currently, there are no uses of proto_tree_add_(int|uint|
boolean)_format_value() with bitfields, with one exception. The exception 
is in the packet-bvlc.c file, and the usage is weird: there, a bitmask of 
0xFFFF is applied to a 16-bit integer - which is obviously pointless. 
Thus, no existing usage of these functions will be harmed.

2. (add_bitmask_text.diff)

Provide somewhat more flexible tool for bitmask dissection. The new 
function, proto_tree_add_bitmask_text() is described in the patch, which 
updates doc/README.developer as well.

Regards,
Alexey.

Index: doc/README.developer
===================================================================
--- doc/README.developer	(revision 21223)
+++ doc/README.developer	(working copy)
@@ -1998,6 +1998,10 @@
 	proto_tree_add_bitmask(tree, tvb, start, header, ett, **fields,
 	    little_endian);
 
+	proto_item *
+	proto_tree_add_bitmask_text(tree, tvb, start, length, name,
+	    fallback, ett, **fields, little_endian, flags);
+
 The 'tree' argument is the tree to which the item is to be added.  The
 'tvb' argument is the tvbuff from which the item's value is being
 extracted; the 'start' argument is the offset from the beginning of that
@@ -2351,6 +2355,7 @@
 tree.
 
 proto_tree_add_bitmask()
+proto_tree_add_bitmask_text()
 ---------------------
 This function provides an easy to use and convenient helper function
 to manage many types of common bitmasks that occur in protocols.
@@ -2397,7 +2402,24 @@
 
 Which provides very pretty dissection of this one byte bitmask.
 
+The proto_tree_add_bitmask_text() function is similar to
+proto_tree_add_bitmask(), but is somewhat more flexible. First, it
+allows for bitfields where one cannot provide a good description of the
+whole bitfield (i.e. where the members are not logically connected with
+each other). In this case, the 'name' argument should be supplied as NULL
+and the top-level item's name will be made of the sub-fields that compose
+it. Second, for a collection of flag bits, a 'fallback' text can be
+provided that will be appended to the top-level item in case that none
+of the flags is set.
 
+The 'flags' argument allows to specify which values shall be appended to
+the top-level item's text: BMT_NO_INT prevents adding of numeric integers,
+BMT_NO_TFS prevents it from expanding the 'true_false_string' structures
+associated with boolean fields, BMT_NO_FALSE prevents appending values
+for boolean fields that are not set. The proto_tree_add_bitmask()'s
+behavior is described as (BMT_NO_INT|BMT_NO_TFS).
+
+
 1.7 Utility routines.
 
 1.7.1 match_strval and val_to_str.
Index: epan/proto.c
===================================================================
--- epan/proto.c	(revision 21223)
+++ epan/proto.c	(working copy)
@@ -172,6 +172,9 @@
 proto_tree_set_uint64(field_info *fi, guint64 value);
 static void
 proto_tree_set_uint64_tvb(field_info *fi, tvbuff_t *tvb, gint start, gboolean little_endian);
+static gboolean
+proto_item_add_bitmask_tree(proto_item *item, tvbuff_t *tvb, int offset, int len, gint ett,
+	const gint **fields, gboolean little_endian, int flags, gboolean first);
 
 static int proto_register_field_init(header_field_info *hfinfo, int parent);
 
@@ -5323,80 +5341,45 @@
 }
 
 
-/* This function will dissect a sequence of bytes that describe a
- * bitmask.
- * hf_hdr is a 8/16/24/32 bit integer that describes the bitmask to be dissected.
- * This field will form an expansion under which the individual fields of the
- * bitmask is dissected and displayed.
- * This field must be of the type FT_[U]INT{8|16|24|32}.
- *
- * fields is an array of pointers to int that lists all the fields of the
- * bitmask. These fields can be either of the type FT_BOOLEAN for flags
- * or another integer of the same type/size as hf_hdr with a mask specified.
- * This array is terminated by a NULL entry.
- *
- * FT_BOOLEAN bits that are set to 1 will have the name added to the expansion.
- * FT_integer fields that have a value_string attached will have the
- * matched string displayed on the expansion line.
+/* This function is common code for both proto_tree_add_bitmask() and
+ *  proto_tree_add_bitmask_text() functions.
  */
-proto_item *
-proto_tree_add_bitmask(proto_tree *parent_tree, tvbuff_t *tvb, int offset, int hf_hdr, gint ett, const int **fields, gboolean little_endian)
+static gboolean
+proto_item_add_bitmask_tree(proto_item *item, tvbuff_t *tvb, int offset, int len, gint ett,
+	const int **fields, gboolean little_endian, int flags, gboolean first)
 {
-	proto_tree *tree=NULL;
-	proto_item *item=NULL;
-	header_field_info *hf_info;
-	int len=0;
-	guint32 value=0;
+	guint32 value = 0, tmpval;
+	proto_tree *tree = NULL;
+	header_field_info *hf;
+	const char *fmt;
 
-	hf_info=proto_registrar_get_nth(hf_hdr);
-	switch(hf_info->type){
-	case FT_INT8:
-	case FT_UINT8:
-		len=1;
-		value=tvb_get_guint8(tvb, offset);
+	switch (len) {
+	case 1:
+		value = tvb_get_guint8(tvb, offset);
 		break;
-	case FT_INT16:
-	case FT_UINT16:
-		len=2;
-		if(little_endian){
-			value=tvb_get_letohs(tvb, offset);
-		} else {
-			value=tvb_get_ntohs(tvb, offset);
-		}
+	case 2:
+		value = little_endian ? tvb_get_letohs(tvb, offset) :
+			tvb_get_ntohs(tvb, offset);
 		break;
-	case FT_INT24:
-	case FT_UINT24:
-		len=3;
-		if(little_endian){
-			value=tvb_get_letoh24(tvb, offset);
-		} else {
-			value=tvb_get_ntoh24(tvb, offset);
-		}
+	case 3:
+		value = little_endian ? tvb_get_letoh24(tvb, offset) :
+			tvb_get_ntoh24(tvb, offset);
 		break;
-	case FT_INT32:
-	case FT_UINT32:
-		len=4;
-		if(little_endian){
-			value=tvb_get_letohl(tvb, offset);
-		} else {
-			value=tvb_get_ntohl(tvb, offset);
-		}
+	case 4:
+		value = little_endian ? tvb_get_letohl(tvb, offset) :
+			tvb_get_ntohl(tvb, offset);
 		break;
 	default:
 		g_assert_not_reached();
 	}
 
-	if(parent_tree){
-		item=proto_tree_add_item(parent_tree, hf_hdr, tvb, offset, len, little_endian);
-		tree=proto_item_add_subtree(item, ett);
-	}
+	tree = proto_item_add_subtree(item, ett);
+	while (*fields) {
+		hf = proto_registrar_get_nth(**fields);
+		DISSECTOR_ASSERT(hf->bitmask != 0);
+		tmpval = (value & hf->bitmask) >> hf->bitshift;
 
-	while(*fields){
-		header_field_info *hf_field;
-		guint32 tmpval, tmpmask;
-
-		hf_field=proto_registrar_get_nth(**fields);
-		switch(hf_field->type){
+		switch (hf->type) {
 		case FT_INT8:
 		case FT_UINT8:
 		case FT_INT16:
@@ -5405,29 +5388,51 @@
 		case FT_UINT24:
 		case FT_INT32:
 		case FT_UINT32:
+			DISSECTOR_ASSERT(len == ftype_length(hf->type));
 			proto_tree_add_item(tree, **fields, tvb, offset, len, little_endian);
 
-			/* Mask and shift out the value */
-			tmpmask=hf_field->bitmask;
-			tmpval=value;
-			if(tmpmask){
-				tmpval&=tmpmask;
-				while(!(tmpmask&0x00000001)){
-					tmpval>>=1;
-					tmpmask>>=1;
+			if (hf->strings) {
+				proto_item_append_text(item, "%s%s: %s", first ? "" : ", ",
+						hf->name, val_to_str(tmpval, hf->strings, "Unknown"));
+				first = FALSE;
+			} else if (!(flags & BMT_NO_INT)) {
+				if (!first) {
+					proto_item_append_text(item, ", ");
 				}
+
+				fmt = IS_FT_INT(hf->type) ? hfinfo_int_format(hf) : hfinfo_uint_format(hf);
+				if (IS_BASE_DUAL(hf->display)) {
+					proto_item_append_text(item, fmt, hf->name, tmpval, tmpval);
+				} else {
+					proto_item_append_text(item, fmt, hf->name, tmpval);
+				}
+				first = FALSE;
 			}
-			/* Show the value_string content (if there is one) */
-			if(hf_field->strings){
-				proto_item_append_text(item, ",  %s", val_to_str(tmpval, hf_field->strings, "Unknown"));
-			}
 
 			break;
 		case FT_BOOLEAN:
+			DISSECTOR_ASSERT(len * 8 == hf->display);
 			proto_tree_add_item(tree, **fields, tvb, offset, len, little_endian);
-			/* if the flag is set, show the name */
-			if(hf_field->bitmask&value){
-				proto_item_append_text(item, ",  %s", hf_field->name);
+
+			if (hf->strings && !(flags & BMT_NO_TFS)) {
+				/* If we have true/false strings, emit full - otherwise messages
+				   might look weird */
+				const struct true_false_string *tfs =
+					(const struct true_false_string *)hf->strings;
+
+				if (tmpval) {
+					proto_item_append_text(item, "%s%s: %s", first ? "" : ", ",
+							hf->name, tfs->true_string);
+					first = FALSE;
+				} else if (!(flags & BMT_NO_FALSE)) {
+					proto_item_append_text(item, "%s%s: %s", first ? "" : ", ",
+							hf->name, tfs->false_string);
+					first = FALSE;
+				}
+			} else if (tmpval) {
+				/* If the flag is set, show the name */
+				proto_item_append_text(item, "%s%s", first ? "" : ", ", hf->name);
+				first = FALSE;
 			}
 			break;
 		default:
@@ -5437,6 +5442,62 @@
 		fields++;
 	}
 
+	return first;
+}
+
+/* This function will dissect a sequence of bytes that describe a
+ * bitmask.
+ * hf_hdr is a 8/16/24/32 bit integer that describes the bitmask to be dissected.
+ * This field will form an expansion under which the individual fields of the
+ * bitmask is dissected and displayed.
+ * This field must be of the type FT_[U]INT{8|16|24|32}.
+ *
+ * fields is an array of pointers to int that lists all the fields of the
+ * bitmask. These fields can be either of the type FT_BOOLEAN for flags
+ * or another integer of the same type/size as hf_hdr with a mask specified.
+ * This array is terminated by a NULL entry.
+ *
+ * FT_BOOLEAN bits that are set to 1 will have the name added to the expansion.
+ * FT_integer fields that have a value_string attached will have the 
+ * matched string displayed on the expansion line.
+ */
+proto_item *
+proto_tree_add_bitmask(proto_tree *parent_tree, tvbuff_t *tvb, guint offset, int hf_hdr,
+		gint ett, const int **fields, gboolean little_endian)
+{
+	proto_item *item = NULL;
+	header_field_info *hf;
+	int len;
+
+	hf = proto_registrar_get_nth(hf_hdr);
+	DISSECTOR_ASSERT(IS_FT_INT(hf->type) || IS_FT_UINT(hf->type));
+	len = ftype_length(hf->type);
+
+	if (parent_tree) {
+		item = proto_tree_add_item(parent_tree, hf_hdr, tvb, offset, len, little_endian);
+		proto_item_add_bitmask_tree(item, tvb, offset, len, ett, fields, little_endian,
+				BMT_NO_INT|BMT_NO_TFS, FALSE);
+	}
+
 	return item;
 }
 
+/* The same as proto_tree_add_bitmask(), but using an arbitrary text as a top-level item */
+proto_item *
+proto_tree_add_bitmask_text(proto_tree *parent_tree, tvbuff_t *tvb, guint offset, guint len,
+		const char *name, const char *fallback,
+		gint ett, const int **fields, gboolean little_endian, int flags)
+{
+	proto_item *item = NULL;
+
+	if (parent_tree) {
+		item = proto_tree_add_text(parent_tree, tvb, offset, len, "%s", name ? name : "");
+		if (proto_item_add_bitmask_tree(item, tvb, offset, len, ett, fields, little_endian,
+					flags, TRUE) && fallback) {
+			/* Still at first item - append 'fallback' text if any */
+			proto_item_append_text(item, "%s", fallback);
+		}
+	}
+
+	return item;
+}
Index: epan/proto.h
===================================================================
--- epan/proto.h	(revision 21223)
+++ epan/proto.h	(working copy)
@@ -1568,10 +1568,38 @@
 extern field_info*
 proto_find_field_from_offset(proto_tree *tree, guint offset, tvbuff_t *tvb);
 
+/** Add an integer with a subtree of bitfields.
+ @param tree the tree to append this item to
+ @param tvb the tv buffer of the current data
+ @param offset start of data in tvb
+ @param hf_hdr top-level field index
+ @param ett subtree index
+ @param fields NULL-terminated array of bitfield indexes
+ @param little_endian big or little endian byte representation
+ @return the newly created item */
+extern proto_item *
+proto_tree_add_bitmask(proto_tree *tree, tvbuff_t *tvb, guint offset, int hf_hdr,
+		gint ett, const int **fields, gboolean little_endian);
 
+/** Add a text with a subtree of bitfields.
+ @param tree the tree to append this item to
+ @param tvb the tv buffer of the current data
+ @param offset start of data in tvb
+ @param name field name (NULL if bitfield contents should be used)
+ @param fallback field name if none of bitfields were usable
+ @param ett subtree index
+ @param fields NULL-terminated array of bitfield indexes
+ @param little_endian big or little endian byte representation
+ @return the newly created item */
 extern proto_item *
-proto_tree_add_bitmask(proto_tree *tree, tvbuff_t *tvb, int offset, int hf_hdr, gint ett, const int **fields, gboolean little_endian);
+proto_tree_add_bitmask_text(proto_tree *tree, tvbuff_t *tvb, guint offset, guint len,
+		const char *name, const char *fallback,
+		gint ett, const int **fields, gboolean little_endian, int flags);
 
+#define BMT_NO_INT	0x01
+#define BMT_NO_FALSE	0x02
+#define BMT_NO_TFS	0x04
+
 #ifdef __cplusplus
 }
 #endif /* __cplusplus */

Index: epan/proto.c
===================================================================
--- epan/proto.c	(revision 21223)
+++ epan/proto.c	(working copy)
@@ -2903,12 +2906,27 @@
 	int	ret;	/*tmp return value */
 	int	replen;
 	field_info *fi = PITEM_FINFO(pi);
+	header_field_info *hf = fi->hfinfo;
 
 	if (!PROTO_ITEM_IS_HIDDEN(pi)) {
 		ITEM_LABEL_NEW(fi->rep);
 		replen = 0;
-		ret = g_snprintf(fi->rep->representation, ITEM_LABEL_LENGTH,
-		    "%s: ", fi->hfinfo->name);
+		if (hf->bitmask && (hf->type == FT_BOOLEAN ||
+				IS_FT_UINT(hf->type) || IS_FT_INT(hf->type))) {
+			char tmpbuf[64];
+			guint32 val;
+
+			val = fvalue_get_uinteger(&fi->value);
+			if (hf->bitshift > 0) {
+				val <<= hf->bitshift;
+			}
+			decode_bitfield_value(tmpbuf, val, hf->bitmask, hfinfo_bitwidth(hf));
+			ret = g_snprintf(fi->rep->representation, ITEM_LABEL_LENGTH,
+			    "%s%s: ", tmpbuf, fi->hfinfo->name);
+		} else {
+			ret = g_snprintf(fi->rep->representation, ITEM_LABEL_LENGTH,
+			    "%s: ", fi->hfinfo->name);
+		}
 		if ((ret == -1) || (ret >= ITEM_LABEL_LENGTH)) {
 			/* That's all we can put in the representation. */
 			fi->rep->representation[ITEM_LABEL_LENGTH - 1] = '\0';
Index: epan/dissectors/packet-bvlc.c
===================================================================
--- epan/dissectors/packet-bvlc.c	(revision 21223)
+++ epan/dissectors/packet-bvlc.c	(working copy)
@@ -325,11 +325,9 @@
 			FT_UINT16, BASE_DEC, NULL, 0,
 			"Length of BVLC", HFILL }
 		},
-		/* We should bitmask the result correctly when we have a
-		 * packet to dissect */
 		{ &hf_bvlc_result,
 			{ "Result",           "bvlc.result",
-			FT_UINT16, BASE_HEX, NULL, 0xffff,
+			FT_UINT16, BASE_HEX, NULL, 0,
 			"Result Code", HFILL }
 		},
 		{ &hf_bvlc_bdt_ip,