Wireshark · Wireshark-dev: [Wireshark-dev] [RFC] bug 6797 : proto: Add proto_tree_add_split_{uint, int} helpers for non contigu

[Sylvain Munaut <246tnt@xxxxxxxxx>]

Hi,

Anders Broman suggested I post here to get more feedback.

----
This adds a helper in proto.c to display fields where you have to go grab bits
at several places (within a 32 bits word) to make the value.

This doesn't recover the value of the field itself, it just takes care of the
formating and displaying of 'bit fields' in the common format to display such
things.

(thing like  ....110...1100...1 = 0xd9)

This will be used in the GMR-1 RR dissector among others where some fields have
been split into small values (i.e. MSB at one place, LSB at another).

If you don't think this belongs in proto and should be somewhere else, I can
prefix it with gmr1_ and place it in gmr1_common.c (it does need to be exported
in the global namespace tough because it's used in several gmr1 files).
----

And the feedback already in the bug tracker:


> We would need a write up of the function in README.developer as well.

Ok, will do.


> You should probably post to the developers list to get more feedback.

Done :)


> Would it be feasible to have proto_tree_add_split_item() instead/as well
> and have all the magic done internally?

There is two issues there:
 - First the caller code can often decode the real value much more
easily than having to reconstruct it bit by bit.
 - Some time to get the value you have to take the bits in a weird
"order". Like if you get something like

...3210...7654  (dots are unused bits, numbers are bit position).

The caller knows he can just do (val & 0xf) << 4 | ((val & 0xf00) >> 8)
But passing the info on which bit to take when is not trivial.

Here I'm aiming to provide visual feedback about which bits where used
rather than how they were used.


> Would it be better to go for 64bits?

I used 32 bits because all the display "magic" is done by a
pre-existing helper decode_bitfield_value and it only handles 32 bits.


> Come to think of it would proto_tree_add_split_bits_item(tvb,tree, start_bit_offset,num_of_bits,num_skip_bits,num_of_remaining_bits,encoding) make more sense?

See above: Sometimes the bits don't have to be taken "in order" ...


Cheers,

    Sylvain

From 656fbda551f07b8d5bd1c4af412f257692628e9c Mon Sep 17 00:00:00 2001
From: Sylvain Munaut <tnt@xxxxxxxxxx>
Date: Sun, 18 Dec 2011 13:22:10 +0100
Subject: [PATCH] proto: Add proto_tree_add_split_{uint,int} helpers for non contiguous int fields

Signed-off-by: Sylvain Munaut <tnt@xxxxxxxxxx>
---
 epan/proto.c |   94 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 epan/proto.h |   32 ++++++++++++++++++++
 2 files changed, 126 insertions(+), 0 deletions(-)

diff --git a/epan/proto.c b/epan/proto.c
index 7c3a214..01b2162 100644
--- a/epan/proto.c
+++ b/epan/proto.c
@@ -2932,6 +2932,53 @@ proto_tree_add_uint(proto_tree *tree, int hfindex, tvbuff_t *tvb, gint start,
 	return pi;
 }
 
+proto_item*
+proto_tree_add_split_uint(proto_tree *tree, const int hf_index,
+                          tvbuff_t *tvb, const gint start, const gint len,
+                          guint32 true_value, guint32 mask, const guint enc)
+{
+	char *bf_str, *lbl_str;
+	proto_item* pi;
+	guint32 v;
+
+	if (!tree)
+		return NULL;
+
+	bf_str = ep_alloc(64);
+	lbl_str = ep_alloc(ITEM_LABEL_LENGTH+1);
+
+	switch (len) {
+	case 1:
+		v = tvb_get_guint8(tvb, start);
+		break;
+
+	case 2:
+		v = (enc == ENC_BIG_ENDIAN) ?
+			tvb_get_ntohs(tvb, start) :
+			tvb_get_letohs(tvb, start);
+		break;
+
+	case 4:
+		v = (enc == ENC_BIG_ENDIAN) ?
+			tvb_get_ntohl(tvb, start) :
+			tvb_get_letohl(tvb, start);
+		break;
+
+	default:
+			DISSECTOR_ASSERT_NOT_REACHED();
+	}
+
+	decode_bitfield_value(bf_str, v, mask, len<<3);
+
+	pi = proto_tree_add_uint(tree, hf_index, tvb, start, len, true_value);
+
+	proto_item_fill_label(PITEM_FINFO(pi), lbl_str);
+
+	proto_item_set_text(pi, "%s%s", bf_str, lbl_str);
+
+	return pi;
+}
+
 proto_item *
 proto_tree_add_uint_format_value(proto_tree *tree, int hfindex, tvbuff_t *tvb,
 				 gint start, gint length, guint32 value,
@@ -3098,6 +3145,53 @@ proto_tree_add_int(proto_tree *tree, int hfindex, tvbuff_t *tvb, gint start,
 	return pi;
 }
 
+proto_item*
+proto_tree_add_split_int(proto_tree *tree, const int hf_index,
+                         tvbuff_t *tvb, const gint start, const gint len,
+                         gint32 true_value, guint32 mask, const guint enc)
+{
+	char *bf_str, *lbl_str;
+	proto_item* pi;
+	guint32 v;
+
+	if (!tree)
+		return NULL;
+
+	bf_str = ep_alloc(64);
+	lbl_str = ep_alloc(ITEM_LABEL_LENGTH+1);
+
+	switch (len) {
+	case 1:
+		v = tvb_get_guint8(tvb, start);
+		break;
+
+	case 2:
+		v = (enc == ENC_BIG_ENDIAN) ?
+			tvb_get_ntohs(tvb, start) :
+			tvb_get_letohs(tvb, start);
+		break;
+
+	case 4:
+		v = (enc == ENC_BIG_ENDIAN) ?
+			tvb_get_ntohl(tvb, start) :
+			tvb_get_letohl(tvb, start);
+		break;
+
+	default:
+			DISSECTOR_ASSERT_NOT_REACHED();
+	}
+
+	decode_bitfield_value(bf_str, v, mask, len<<3);
+
+	pi = proto_tree_add_int(tree, hf_index, tvb, start, len, true_value);
+
+	proto_item_fill_label(PITEM_FINFO(pi), lbl_str);
+
+	proto_item_set_text(pi, "%s%s", bf_str, lbl_str);
+
+	return pi;
+}
+
 proto_item *
 proto_tree_add_int_format_value(proto_tree *tree, int hfindex, tvbuff_t *tvb,
 				gint start, gint length, gint32 value,
diff --git a/epan/proto.h b/epan/proto.h
index 6859d48..0051211 100644
--- a/epan/proto.h
+++ b/epan/proto.h
@@ -1320,6 +1320,22 @@ extern proto_item *
 proto_tree_add_uint(proto_tree *tree, int hfindex, tvbuff_t *tvb, gint start,
 	gint length, guint32 value);
 
+/** Add one FT_UINT8, FT_UINT16, FT_UINT24 or FT_UINT32 to a proto_tree
+ *  when it's contructed from non contiguous bits
+ @param tree the tree to append this item to
+ @param hfindex field index
+ @param tvb the tv buffer of the current data
+ @param start start of data in tvb
+ @param length length of data in tvb
+ @param true_value data to display and set
+ @param mask mask of bits belonging to the value (used to generate string)
+ @param enc encoding endianness
+ @return the newly created item */
+extern proto_item*
+proto_tree_add_split_uint(proto_tree *tree, const int hf_index,
+	tvbuff_t *tvb, const gint start, const gint len,
+	guint32 true_value, guint32 mask, const guint enc);
+
 /** Add a formatted FT_UINT8, FT_UINT16, FT_UINT24 or FT_UINT32 to a proto_tree,
     with the format generating the string for the value and with the field
     name being included automatically.
@@ -1409,6 +1425,22 @@ extern proto_item *
 proto_tree_add_int(proto_tree *tree, int hfindex, tvbuff_t *tvb, gint start,
 	gint length, gint32 value);
 
+/** Add one FT_INT8, FT_INT16, FT_INT24 or FT_INT32 to a proto_tree
+ *  when it's contructed from non contiguous bits
+ @param tree the tree to append this item to
+ @param hfindex field index
+ @param tvb the tv buffer of the current data
+ @param start start of data in tvb
+ @param length length of data in tvb
+ @param true_value data to display and set
+ @param mask mask of bits belonging to the value (used to generate string)
+ @param enc encoding endianness
+ @return the newly created item */
+extern proto_item*
+proto_tree_add_split_int(proto_tree *tree, const int hf_index,
+	tvbuff_t *tvb, const gint start, const gint len,
+	gint32 true_value, guint32 mask, const guint enc);
+
 /** Add a formatted FT_INT8, FT_INT16, FT_INT24 or FT_INT32 to a proto_tree,
     with the format generating the string for the value and with the field
     name being included automatically.
-- 
1.7.3.4