Wireshark · Wireshark-dev: Re: [Wireshark-dev] proto_tree_add_item() with range_string

[Sebastien Tandel <sebastien@xxxxxxxxx>]

Hi,

thx for the advice, I indeed didn't notice it. Modification done and
patch attached (FT_BOOLEAN not working) but ...

As I was not totally satisfied by my solution, I'm not totally with your
neither. With VALS and TFS (and, in fact, also NULL :)), as you said,
this field 'display' is usually defined to know which base to use (dec,
hex, dec_hex, ...) to display a value. If you want a range_string
displayed with base dec, hex, ... you have to duplicate
BASE_RANGE_STRING values as for example BASE_RANGE_STRING_DEC,
BASE_RANGE_STRING_HEX, ... VALS and TFS are independent from the base
chosen (or in general display field) and RVALS won't be.

What do you think of an hybrid solution between mine and your defining
an additional field (i.e. convertfield_type) in head_register_info which
identifies which type of structure is in CONVERTFIELD (strings)?
- It would bring the independence  from the base avoiding us to
duplicate the fields (BASE_RANGE_STRING_DEC, BASE_RANGE_STRING_HEX, ...)
- this field could be hidden by the HFILL macro when you don't have to
use it (and use another, HFILL2(?), when you have to use this field)
- it would be scalable for the current and future implementation if you
create another structure à la range_string without changing anything in
the code of the existing dissectors.
- would be possible to use FT_BOOLEAN as well
- it's clean (from the syntax point of view and from the semantic as
well) :)


Regards,

Sebastien Tandel

Guy Harris wrote:
> Sebastien Tandel wrote:
>
>   
>> If you want something cleaner ... one of the solution may be a
>> intermediate structure which is mandatory with well-defined values to
>> determine which structure it encapsulates. => need changes then in the
>> macros VALS, TFS and even in the dissectors using VALS, TFS
>>     
>
> That's one way to do it.
>
> Another is to note that there's a field "display" in the 
> header_field_info structure.  It currently just holds a base indication 
> for numbers, but it could be treated as a general "how to display this 
> field" indicator, with additional information such as, for numbers, 
> "display this as a numerical value"/"display this as a numerical value 
> and a symbolic interpretation using a value_string table"/"display this 
> as a numerical value with a range-based description"/etc..
>
> Unfortunately, that won't work for FT_BOOLEAN, as, for Boolean 
> bitfields, "display" holds the value of the bigger item in which the 
> bitfield is packed.
> _______________________________________________
> Wireshark-dev mailing list
> Wireshark-dev@xxxxxxxxxxxxx
> http://www.wireshark.org/mailman/listinfo/wireshark-dev
>   

Index: epan/proto.c
===================================================================
--- epan/proto.c	(révision 20350)
+++ epan/proto.c	(copie de travail)
@@ -4028,9 +4028,15 @@
 	value = fvalue_get_integer(&fi->value);
 
 	/* Fill in the textual info */
-	ret = g_snprintf(label_str, ITEM_LABEL_LENGTH,
+	if (hfinfo->display == BASE_RANGE_STRING) {
+	  ret = g_snprintf(label_str, ITEM_LABEL_LENGTH, 
 			format,  hfinfo->name,
+			rval_to_str(value, hfinfo->strings, "Unknown"), value);
+	} else {
+	  ret = g_snprintf(label_str, ITEM_LABEL_LENGTH,
+			format,  hfinfo->name,
 			val_to_str(value, cVALS(hfinfo->strings), "Unknown"), value);
+	}
 	if ((ret == -1) || (ret >= ITEM_LABEL_LENGTH))
 		label_str[ITEM_LABEL_LENGTH - 1] = '\0';
 }
@@ -4096,9 +4102,15 @@
 	value = fvalue_get_integer(&fi->value);
 
 	/* Fill in the textual info */
-	ret = g_snprintf(label_str, ITEM_LABEL_LENGTH,
+	if (hfinfo->display == BASE_RANGE_STRING) {
+	  ret = g_snprintf(label_str, ITEM_LABEL_LENGTH, 
 			format,  hfinfo->name,
+			rval_to_str(value, hfinfo->strings, "Unknown"), value);
+	} else {
+	  ret = g_snprintf(label_str, ITEM_LABEL_LENGTH,
+			format,  hfinfo->name,
 			val_to_str(value, cVALS(hfinfo->strings), "Unknown"), value);
+	}
 	if ((ret == -1) || (ret >= ITEM_LABEL_LENGTH))
 		label_str[ITEM_LABEL_LENGTH - 1] = '\0';
 }
@@ -4195,6 +4207,7 @@
 	switch(hfinfo->display) {
 		case BASE_DEC:
 		case BASE_DEC_HEX:
+		case BASE_RANGE_STRING:
 			format = "%s: %s (%u)";
 			break;
 		case BASE_OCT: /* I'm lazy */
@@ -4319,6 +4332,7 @@
 	switch(hfinfo->display) {
 		case BASE_DEC:
 		case BASE_DEC_HEX:
+		case BASE_RANGE_STRING:
 			format = "%s: %s (%d)";
 			break;
 		case BASE_OCT: /* I'm lazy */
Index: epan/proto.h
===================================================================
--- epan/proto.h	(révision 20350)
+++ epan/proto.h	(copie de travail)
@@ -65,6 +65,10 @@
 /** Make a const true_false_string[] look like a _true_false_string pointer, used to set header_field_info.strings */
 #define TFS(x)	(const struct true_false_string*)(x)
 
+/** Make a const value_string[] look like a _value_string pointer, used to set
+ * header_field_info.strings */
+#define RVALS(x) (const struct _value_string*)(x)
+
 struct _protocol;
 
 /** Structure for information about a protocol */
@@ -136,7 +140,8 @@
 	BASE_HEX,	/**< hexadecimal */
 	BASE_OCT,	/**< octal */
 	BASE_DEC_HEX,	/**< decimal (hexadecimal) */
-	BASE_HEX_DEC	/**< hexadecimal (decimal) */
+	BASE_HEX_DEC,	/**< hexadecimal (decimal) */
+	BASE_RANGE_STRING
 } base_display_e;
 
 #define IS_BASE_DUAL(b) ((b)==BASE_DEC_HEX||(b)==BASE_HEX_DEC)