Refactor NCP Python data so that INFO column can be generated on the fly.

The "old" method of populating the INFO column was to dissect all fields of a function/subfunction, then do a search in the tree to find the hf_ values of interest to then format into something for the INFO column.  This is very expensive and requires "low level" APIs (for tree manipulation) which really shouldn't be used in a dissector.

The "new" method populates the INFO column at the same time a field is parsed, so nothing has to be revisited.

There are still expert infos (and possibly column APIs) under if (tree)s, but with the FAKE_TREE_IS_VISIBLE "hacks" removed, there should be less fear in removing the tree checks.

Change-Id: I847827395fc28704f468df8bc8b47b297dde8479
Reviewed-on: https://code.wireshark.org/review/10572
Petri-Dish: Michael Mann <mmann78@netscape.net>
Tested-by: Petri Dish Buildbot <buildbot-no-reply@wireshark.org>
Reviewed-by: Anders Broman <a.broman58@gmail.com>

2 files changed, 190 insertions, 432 deletions

diff --git a/epan/dissectors/packet-ncp-int.h b/epan/dissectors/packet-ncp-int.h
index 816f467cac..9f6778afa0 100644
--- a/epan/dissectors/packet-ncp-int.h
+++ b/epan/dissectors/packet-ncp-int.h
@@ -34,10 +34,17 @@
 typedef struct _ptvc_record ptvc_record;
 typedef struct _sub_ptvc_record sub_ptvc_record;
 
+typedef struct {
+	int			*hf_ptr;
+	const char		*first_string;
+	const char		*repeat_string;
+} info_string_t;
+
 struct _ptvc_record {
 	int			*hf_ptr;
 	gint			length;
 	const sub_ptvc_record	*sub_ptvc_rec;
+	const info_string_t	*req_info_str;
 	unsigned int	endianness;
 	unsigned int	var_index	: 2;
 	unsigned int	repeat_index	: 2;
@@ -80,13 +87,6 @@ typedef struct {
 	struct epan_dfilter	*dfilter;
 } conditional_record;
 
-typedef struct {
-	int			*hf_ptr;
-	const char		*first_string;
-	const char		*repeat_string;
-} info_string_t;
-
-
 struct novell_tap {
 	int stat;
 	int hdr;
@@ -113,7 +113,6 @@ typedef struct _ncp_record {
 	const error_equivalency	*errors;
 	const int		*req_cond_indexes;
 	unsigned int		req_cond_size_type;
-	const info_string_t	*req_info_str;
 	ncp_expert_handler  *expert_handler_func;
 } ncp_record;
 
diff --git a/epan/dissectors/packet-ncp2222.inc b/epan/dissectors/packet-ncp2222.inc
index a7bf95d958..c338e5d5c3 100644
--- a/epan/dissectors/packet-ncp2222.inc
+++ b/epan/dissectors/packet-ncp2222.inc
@@ -1,11 +1,3 @@
-#define FAKE_TREE_IS_VISIBLE 1
-/* fake that the tree is visible so that proto.c will not try to
- "fake" generation of finfo values/nodes.
-  Thus this means netware dissector is slower than it has to, but it wont crash.
-  once the dissector is refactored to avoid calling fvalue_ functions directly this faking of whether the tree is visible or not can be removed.
-*/
-
-
 /* packet-ncp2222.inc
  *
  * Routines for NetWare Core Protocol. This C code gets #include'd
@@ -38,8 +30,6 @@
  * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
  */
 
-#define PROTO_LENGTH_UNTIL_END -1
-
 gboolean    nds_defragment  = TRUE;
 gboolean    nds_echo_eid    = TRUE;
 gboolean    ncp_echo_err    = TRUE;
@@ -2117,49 +2107,7 @@ ncp_record_find(guint8 func, guint8 subfunc)
     return NULL;
 }
 
-
-/* Given a proto_item*, assume it contains an integer value
- * and return a guint from it. */
-static guint
-get_finfo_value_integer(field_info *finfo)
-{
-    /* XXX the fvalue functions are no longer safe to call directly
-       since we sometimes fake the entries to speed things up.
-       this dissector should not call fvalue_ functions directly.
-    */
-    if(!finfo->value.ftype->get_value_uinteger){
-        return 0;
-    }
-    return fvalue_get_uinteger(&finfo->value);
-}
-static guint
-get_item_value(proto_item *item)
-{
-    return get_finfo_value_integer(PITEM_FINFO(item));
-}
-
-
-static guint
-get_finfo_length(field_info *finfo)
-{
-    return fvalue_length(&finfo->value);
-}
-
-
-static char *
-get_finfo_value_string(field_info *finfo)
-{
-    return (char *)fvalue_get(&finfo->value);
-}
-
-static const char *
-get_item_name(proto_item *item)
-{
-    return PITEM_FINFO(item)->hfinfo->name;
-}
-
-
-typedef proto_item* (*padd_func_t)(ptvcursor_t*, const ptvc_record*);
+typedef proto_item* (*padd_func_t)(packet_info* pinfo, ptvcursor_t*, const ptvc_record*, gboolean request, gboolean repeat, guint* ret_value);
 
 /*
  * XXX - are these just DOS-format dates and times?
@@ -2287,51 +2235,179 @@ uint_to_nwtime(guint data, nw_time_t *nwtime)
 }
 
 static proto_item*
-padd_normal(ptvcursor_t *ptvc, const ptvc_record *rec)
+padd_normal(packet_info* pinfo, ptvcursor_t *ptvc, const ptvc_record *rec, gboolean request, gboolean repeat, guint* ret_value)
 {
-    return
-        ptvcursor_add(ptvc, *rec->hf_ptr,
+    header_field_info* hinfo = proto_registrar_get_nth(*rec->hf_ptr);
+
+    if (!repeat && request && rec->req_info_str)
+        col_set_str(pinfo->cinfo, COL_INFO, "C ");
+
+    /* We may want integer values regardless of whether or not the value is displayed in INFO column */
+    if (IS_FT_UINT(hinfo->type) || IS_FT_INT(hinfo->type)) {
+        guint32 value32 = 0;
+        guint64 value64 = 0;
+
+        switch(rec->length)
+        {
+        case 1:
+            value32 = tvb_get_guint8(ptvcursor_tvbuff(ptvc), ptvcursor_current_offset(ptvc));
+            *ret_value = value32;
+            break;
+        case 2:
+            value32 = tvb_get_guint16(ptvcursor_tvbuff(ptvc), ptvcursor_current_offset(ptvc), rec->endianness);
+            *ret_value = value32;
+            break;
+        case 3:
+            value32 = tvb_get_guint24(ptvcursor_tvbuff(ptvc), ptvcursor_current_offset(ptvc), rec->endianness);
+            *ret_value = value32;
+            break;
+        case 4:
+            value32 = tvb_get_guint32(ptvcursor_tvbuff(ptvc), ptvcursor_current_offset(ptvc), rec->endianness);
+            *ret_value = value32;
+            break;
+        case 5:
+            value64 = tvb_get_guint40(ptvcursor_tvbuff(ptvc), ptvcursor_current_offset(ptvc), rec->endianness);
+            break;
+        case 6:
+            value64 = tvb_get_guint48(ptvcursor_tvbuff(ptvc), ptvcursor_current_offset(ptvc), rec->endianness);
+            break;
+        case 7:
+            value64 = tvb_get_guint56(ptvcursor_tvbuff(ptvc), ptvcursor_current_offset(ptvc), rec->endianness);
+            break;
+        case 8:
+            value64 = tvb_get_guint64(ptvcursor_tvbuff(ptvc), ptvcursor_current_offset(ptvc), rec->endianness);
+            break;
+        default:
+            DISSECTOR_ASSERT(FALSE);
+            break;
+        }
+
+        if (request && rec->req_info_str) {
+            if (rec->length <= 4) {
+                col_append_fstr(pinfo->cinfo, COL_INFO,
+                        (const gchar*)(repeat ? rec->req_info_str->repeat_string : rec->req_info_str->first_string),
+                        value32);
+            } else {
+                col_append_fstr(pinfo->cinfo, COL_INFO,
+                        (const gchar*)(repeat ? rec->req_info_str->repeat_string : rec->req_info_str->first_string),
+                        value64);
+
+            }
+        }
+    } else if (request && rec->req_info_str) {
+        if (hinfo->type == FT_STRING) {
+            col_append_fstr(pinfo->cinfo, COL_INFO,
+                                (const gchar*)(repeat ? rec->req_info_str->repeat_string : rec->req_info_str->first_string),
+                                 tvb_get_string_enc(wmem_packet_scope(), ptvcursor_tvbuff(ptvc),
+                                     ptvcursor_current_offset(ptvc), rec->length, ENC_ASCII));
+        } else if (hinfo->type == FT_STRINGZ) {
+            gint length;
+            col_append_fstr(pinfo->cinfo, COL_INFO,
+                                (const gchar*)(repeat ? rec->req_info_str->repeat_string : rec->req_info_str->first_string),
+                                tvb_get_stringz_enc(wmem_packet_scope(), ptvcursor_tvbuff(ptvc),
+                                    ptvcursor_current_offset(ptvc), &length, ENC_ASCII));
+        } else if (hinfo->type == FT_UINT_STRING) {
+            guint32 length = 0;
+
+            switch(rec->length)
+            {
+            case 1:
+                length = tvb_get_guint8(ptvcursor_tvbuff(ptvc), ptvcursor_current_offset(ptvc));
+                break;
+            case 2:
+                length = tvb_get_guint16(ptvcursor_tvbuff(ptvc), ptvcursor_current_offset(ptvc), rec->endianness);
+                break;
+            case 3:
+                length = tvb_get_guint24(ptvcursor_tvbuff(ptvc), ptvcursor_current_offset(ptvc), rec->endianness);
+                break;
+            case 4:
+                length = tvb_get_guint32(ptvcursor_tvbuff(ptvc), ptvcursor_current_offset(ptvc), rec->endianness);
+                break;
+            default:
+                DISSECTOR_ASSERT(FALSE);
+                break;
+            }
+
+            col_append_fstr(pinfo->cinfo, COL_INFO,
+                                (const gchar*)(repeat ? rec->req_info_str->repeat_string : rec->req_info_str->first_string),
+                                tvb_get_string_enc(wmem_packet_scope(), ptvcursor_tvbuff(ptvc),
+                                    ptvcursor_current_offset(ptvc)+rec->length, length, ENC_ASCII));
+        } else if (hinfo->type == FT_BYTES) {
+            col_append_fstr(pinfo->cinfo, COL_INFO,
+                                (const gchar*)(repeat ? rec->req_info_str->repeat_string : rec->req_info_str->first_string),
+                                tvb_bytes_to_str(wmem_packet_scope(), ptvcursor_tvbuff(ptvc),
+                                    ptvcursor_current_offset(ptvc), rec->length));
+        }
+    }
+
+    return ptvcursor_add(ptvc, *rec->hf_ptr,
                       rec->length,
                       rec->endianness);
 }
 
+/* XXX - This could be a BASE_CUSTOM extension to the hf_ fields that use it */
 static proto_item*
-padd_date(ptvcursor_t *ptvc, const ptvc_record *rec)
+padd_date(packet_info* pinfo _U_, ptvcursor_t *ptvc, const ptvc_record *rec, gboolean request _U_, gboolean repeat _U_, guint* ret_value)
 {
     proto_item  *item;
     nw_date_t   nw_date;
 
-    item = ptvcursor_add(ptvc, *rec->hf_ptr,
-                         rec->length,
-                         rec->endianness);
+    switch(rec->length)
+    {
+    case 1:
+        *ret_value = tvb_get_guint8(ptvcursor_tvbuff(ptvc), ptvcursor_current_offset(ptvc));
+        break;
+    case 2:
+        *ret_value = tvb_get_guint16(ptvcursor_tvbuff(ptvc), ptvcursor_current_offset(ptvc), rec->endianness);
+        break;
+    case 3:
+        *ret_value = tvb_get_guint24(ptvcursor_tvbuff(ptvc), ptvcursor_current_offset(ptvc), rec->endianness);
+        break;
+    case 4:
+        *ret_value = tvb_get_guint32(ptvcursor_tvbuff(ptvc), ptvcursor_current_offset(ptvc), rec->endianness);
+        break;
+    default:
+        DISSECTOR_ASSERT(FALSE);
+    }
 
-    if (item) {
-        uint_to_nwdate(get_item_value(item), &nw_date);
+    uint_to_nwdate(*ret_value, &nw_date);
+    item = proto_tree_add_uint_format_value(ptvcursor_tree(ptvc), *rec->hf_ptr, ptvcursor_tvbuff(ptvc), ptvcursor_current_offset(ptvc),
+                                            rec->length, *ret_value, "%04u/%02u/%02u", nw_date.year, nw_date.month, nw_date.day);
+    ptvcursor_advance(ptvc, rec->length);
 
-        proto_item_set_text(item, "%s", get_item_name(item));
-        proto_item_append_text(item, ": %04u/%02u/%02u",
-                               nw_date.year, nw_date.month, nw_date.day);
-    }
     return item;
 }
 
+/* XXX - This could be a BASE_CUSTOM extension to the hf_ fields that use it */
 static proto_item*
-padd_time(ptvcursor_t *ptvc, const ptvc_record *rec)
+padd_time(packet_info* pinfo _U_, ptvcursor_t *ptvc, const ptvc_record *rec, gboolean request _U_, gboolean repeat _U_, guint* ret_value)
 {
     proto_item  *item;
     nw_time_t   nw_time;
 
-    item = ptvcursor_add(ptvc, *rec->hf_ptr,
-                         rec->length,
-                         rec->endianness);
+    switch(rec->length)
+    {
+    case 1:
+        *ret_value = tvb_get_guint8(ptvcursor_tvbuff(ptvc), ptvcursor_current_offset(ptvc));
+        break;
+    case 2:
+        *ret_value = tvb_get_guint16(ptvcursor_tvbuff(ptvc), ptvcursor_current_offset(ptvc), rec->endianness);
+        break;
+    case 3:
+        *ret_value = tvb_get_guint24(ptvcursor_tvbuff(ptvc), ptvcursor_current_offset(ptvc), rec->endianness);
+        break;
+    case 4:
+        *ret_value = tvb_get_guint32(ptvcursor_tvbuff(ptvc), ptvcursor_current_offset(ptvc), rec->endianness);
+        break;
+    default:
+        DISSECTOR_ASSERT(FALSE);
+    }
 
-    if (item) {
-        uint_to_nwtime(get_item_value(item), &nw_time);
+    uint_to_nwtime(*ret_value, &nw_time);
+    item = proto_tree_add_uint_format_value(ptvcursor_tree(ptvc), *rec->hf_ptr, ptvcursor_tvbuff(ptvc), ptvcursor_current_offset(ptvc),
+                                            rec->length, *ret_value, "%02u:%02u:%02u", nw_time.hour, nw_time.minute, nw_time.second);
+    ptvcursor_advance(ptvc, rec->length);
 
-        proto_item_set_text(item, "%s", get_item_name(item));
-        proto_item_append_text(item, ": %02u:%02u:%02u",
-                               nw_time.hour, nw_time.minute, nw_time.second);
-    }
     return item;
 }
 
@@ -2341,7 +2417,7 @@ padd_time(ptvcursor_t *ptvc, const ptvc_record *rec)
    result returned. */
 /* XXX This prints the proto_item name, but not its value. */
 static proto_item*
-padd_uni(ptvcursor_t *ptvc, const ptvc_record *rec)
+padd_uni(packet_info* pinfo _U_, ptvcursor_t *ptvc, const ptvc_record *rec, gboolean request _U_, gboolean repeat _U_, guint* ret_value _U_)
 {
     proto_item      *item;
     /* nw_uni_t        nw_uni; */
@@ -2352,12 +2428,12 @@ padd_uni(ptvcursor_t *ptvc, const ptvc_record *rec)
                          rec->length,
                          rec->endianness);
 
-    if (item) {
+/*  if (item) {
         proto_item_set_text(item, "%s", get_item_name(item));
-        /* proto_item_append_text(item, " %s",
-           nw_uni.buffer); */
+           proto_item_append_text(item, " %s",
+           nw_uni.buffer);
     }
-
+*/
     return item;
 }
 
@@ -2428,7 +2504,7 @@ process_struct_sub_ptvc_record(ptvcursor_t *ptvc, packet_info *pinfo, const ptvc
         ett = *rec->sub_ptvc_rec->ett;
         old_tree = ptvcursor_tree(ptvc);
         offset = ptvcursor_current_offset(ptvc);
-        new_tree = proto_tree_add_subtree(old_tree, ptvcursor_tvbuff(ptvc), offset, PROTO_LENGTH_UNTIL_END,
+        new_tree = proto_tree_add_subtree(old_tree, ptvcursor_tvbuff(ptvc), offset, -1,
 						ett, &item, rec->sub_ptvc_rec->descr);
         ptvcursor_set_tree(ptvc, new_tree);
     }
@@ -2455,9 +2531,12 @@ _process_ptvc_record(ptvcursor_t *ptvc, packet_info *pinfo, const ptvc_record *r
                      gboolean *req_cond_results, gboolean really_decode,
                      const ncp_record *ncp_rec, gboolean request)
 {
-    proto_item  *item;
-    guint       i, repeat_count;
+    guint       i, repeat_count, repeat_value;
     padd_func_t func = NULL;
+    guint repeat_vars[NUM_REPEAT_VARS];
+
+    /* Clear the repeat_vars array. */
+    memset(repeat_vars, 0, sizeof(repeat_vars));
 
     if (rec->sub_ptvc_rec) {
         /* Repeat this? */
@@ -2509,11 +2588,12 @@ _process_ptvc_record(ptvcursor_t *ptvc, packet_info *pinfo, const ptvc_record *r
                 default:
                     DISSECTOR_ASSERT_NOT_REACHED();
                 }
-                item = func(ptvc, rec);
+                repeat_value = 0;
+                func(pinfo, ptvc, rec, request, FALSE, &repeat_value);
 
                 /* Set the value as a 'var' ? */
                 if (rec->var_index != NO_VAR) {
-                    repeat_vars[rec->var_index] = get_item_value(item);
+                    repeat_vars[rec->var_index] = repeat_value;
                 }
             }
             else {
@@ -2553,7 +2633,7 @@ _process_ptvc_record(ptvcursor_t *ptvc, packet_info *pinfo, const ptvc_record *r
                     DISSECTOR_ASSERT_NOT_REACHED();
                 }
                 for (i = 0; i < repeat_count; i++ ) {
-                    func(ptvc, rec);
+                    func(pinfo, ptvc, rec, request, i != 0, &repeat_value);
                 }
             }
             else {
@@ -2607,20 +2687,6 @@ process_ptvc_record(ptvcursor_t *ptvc, packet_info *pinfo, const ptvc_record *re
     }
 }
 
-
-
-/* Clear the repeat_vars array. */
-static void
-clear_repeat_vars(void)
-{
-    guint i;
-
-    for (i = 0 ; i < NUM_REPEAT_VARS; i++ ) {
-        repeat_vars[i] = 0;
-    }
-}
-
-
 /* Given an error_equivalency table and a completion code, return
  * the string representing the error. */
 static const char*
@@ -2639,27 +2705,19 @@ ncp_error_string(const error_equivalency *errors, guint8 completion_code)
 
 static const ncp_record ncp1111_request =
 { 0x1, 0x00, NO_SUBFUNC, "Create Connection Service", NCP_GROUP_CONNECTION,
-  NULL, NULL, ncp_0x2_errors, NULL, NO_REQ_COND_SIZE, NULL, NULL };
+  NULL, NULL, ncp_0x2_errors, NULL, NO_REQ_COND_SIZE, NULL };
 
 static const ncp_record ncp5555_request =
 { 0x5, 0x00, NO_SUBFUNC, "Destroy Connection Service", NCP_GROUP_CONNECTION,
-  NULL, NULL, ncp_0x2_errors, NULL, NO_REQ_COND_SIZE, NULL, NULL };
+  NULL, NULL, ncp_0x2_errors, NULL, NO_REQ_COND_SIZE, NULL };
 
 static const ncp_record ncpbbbb_request =
 { 0xb, 0x00, NO_SUBFUNC, "Server Broadcast Message", NCP_GROUP_CONNECTION,
-  NULL, NULL, ncp_0x2_errors, NULL, NO_REQ_COND_SIZE, NULL, NULL };
+  NULL, NULL, ncp_0x2_errors, NULL, NO_REQ_COND_SIZE, NULL };
 
 static const ncp_record ncplip_echo =
 { 0x1f, 0x00, NO_SUBFUNC, "LIP Echo Packet", NCP_GROUP_CONNECTION,
-  NULL, NULL, ncp_0x2_errors, NULL, NO_REQ_COND_SIZE, NULL, NULL };
-
-/* Wrapper around proto_tree_free() */
-static void free_proto_tree(void *tree)
-{
-    if (tree) {
-        proto_tree_free((proto_tree*) tree);
-    }
-}
+  NULL, NULL, ncp_0x2_errors, NULL, NO_REQ_COND_SIZE, NULL };
 
 static guint32
 align_4(tvbuff_t *tvb, guint32 aoffset)
@@ -2734,83 +2792,6 @@ get_string(tvbuff_t* tvb, guint offset, guint str_length)
 }
 
 static void
-uni_to_string(char * data, guint32 str_length, char *dest_buf)
-{
-    gint i;
-    guint16 c_char;
-    gint length_remaining = 0;
-
-    length_remaining = str_length;
-    dest_buf[0] = '\0';
-    if(str_length == 0)
-    {
-        return;
-    }
-    for ( i = 0; i < (gint) str_length; i++ )
-    {
-        c_char = data[i];
-        if (c_char<0x20 || c_char>0x7e)
-        {
-            if (c_char != 0x00)
-            {
-                c_char = '.';
-                dest_buf[i] = c_char & 0xff;
-            }
-            else
-            {
-                i--;
-                str_length--;
-            }
-        }
-        else
-        {
-            dest_buf[i] = c_char & 0xff;
-        }
-        length_remaining--;
-
-        if(length_remaining==0)
-        {
-            if (i+1 < (gint)str_length){
-                dest_buf[i+1] = '\0';
-            } else {
-                dest_buf[i] = '\0';
-            }
-            return;
-        }
-    }
-    if (i < 0) {
-        i = 0;
-    }
-    dest_buf[i] = '\0';
-    return;
-}
-
-/*************************************
- * Return based on % format in request
- * %d = integer in decimal format = 0
- * %x = integer in hex format = 1
- * %s = string = 2
- **************************************/
-static int
-get_info_type(const gchar* check_string)
-{
-    guint length;
-    guint i;
-
-    length =  (guint) strlen(check_string);
-
-    for (i = 0 ; i < length-1 ; i++ ) {
-        if (check_string[i] == 0x25 && check_string[i+1] == 0x64) {   /* %d Digits*/
-            return 0;
-        }
-        if ( check_string[i] == 0x78 && check_string[i+1] == 0x25 && check_string[i+2] == 0x73) {  /* x%s Bytes*/
-            return 1;
-        }
-    }
-    return 2; /* Normal String */
-}
-
-static void
 process_bitfield(proto_tree *ncp_tree, tvbuff_t *tvb, nds_val *values)
 {
     gchar        flags_str[512];
@@ -7187,13 +7168,12 @@ dissect_ncp_request(tvbuff_t *tvb, packet_info *pinfo,
     const ncp_record        *volatile ncp_rec = NULL;
     conversation_t          *conversation;
     ptvcursor_t             *volatile ptvc = NULL;
-    proto_tree              *temp_tree = NULL;
     volatile gboolean       run_req_cond = FALSE;
-    volatile gboolean       run_info_str = FALSE;
     guint32                 length_remaining;
     guint32                 testvar;
     volatile unsigned long  exception_code;
     const char              *volatile message;
+    proto_item              *ti;
 
 
     /* We're dissecting an ncp2222 request; Compile the dfilters (if not yet done). */
@@ -7266,11 +7246,6 @@ dissect_ncp_request(tvbuff_t *tvb, packet_info *pinfo,
     /* Fill in the INFO column. */
     if (ncp_rec) {
         col_add_fstr(pinfo->cinfo, COL_INFO, "C %s", ncp_rec->name);
-        if (ncp_rec->req_info_str) {
-            /* We want to add more stuff to the Info
-                column. */
-            run_info_str = TRUE;
-        }
     }
     else {
         if (requires_subfunc) {
@@ -7320,32 +7295,10 @@ dissect_ncp_request(tvbuff_t *tvb, packet_info *pinfo,
         }
     }
 
-    /* If we have to handle a request condition, or have to
-       add to the Info column, we need to construct a protocol
-       tree.  If we already have a proto_tree, then wonderful.
-       If we don't, we need to build one. */
-    if ((run_info_str || run_req_cond) && !ncp_tree) {
-        proto_item *ti;
-
-        temp_tree = proto_tree_create_root(pinfo);
-        proto_tree_set_visible(temp_tree, FALSE);
-        ti = proto_tree_add_item(temp_tree, proto_ncp, tvb, 0, -1, ENC_NA);
-        ncp_tree = proto_item_add_subtree(ti, ett_ncp);
-    }
+    ti = proto_tree_add_item(ncp_tree, proto_ncp, tvb, 0, -1, ENC_NA);
+    ncp_tree = proto_item_add_subtree(ti, ett_ncp);
 
     if (ncp_tree) {
-        /* If the dissection throws an exception, be sure to free
-         * the temporary proto_tree that was created. Because of the
-         * way the CLEANUP_PUSH macro works, we can't put it in an 'if'
-         * block; it has to be in the same scope as the terminating
-         * CLEANUP_POP or CLEANUP_POP_AND_ALLOC. So, we always
-         * call CLEANUP_POP and friends, but the value of temp_tree is
-         * NULL if no cleanup is needed, and non-null if cleanup is needed. */
-        CLEANUP_PUSH_PFX(xx,free_proto_tree, temp_tree);
-
-#ifdef FAKE_TREE_IS_VISIBLE
-        PTREE_DATA(ncp_tree)->visible=1;
-#endif
 
         /* Before the dissection, if we're saving data for a request
          * condition, we have to prime the proto tree using the
@@ -7364,12 +7317,6 @@ dissect_ncp_request(tvbuff_t *tvb, packet_info *pinfo,
             }
         }
 
-        /* Before the dissection, if we need a field for the info_str,
-         * prime the tree. */
-        if (run_info_str) {
-            proto_tree_prime_hfid(ncp_tree, *ncp_rec->req_info_str->hf_ptr);
-        }
-
         switch (type) {
         case NCP_BROADCAST_SLOT:
             ; /* nothing */
@@ -7410,13 +7357,13 @@ dissect_ncp_request(tvbuff_t *tvb, packet_info *pinfo,
         /* The group is not part of the packet, but it's useful
          * information to display anyway. Put it in the tree for filtering and tap use*/
         if (ncp_rec) {
-            proto_tree_add_uint_format_value(ncp_tree, hf_ncp_group, tvb, 0, 0, ncp_rec->group, "%s", ncp_groups[ncp_rec->group]);
+            ti = proto_tree_add_uint_format_value(ncp_tree, hf_ncp_group, tvb, 0, 0, ncp_rec->group, "%s", ncp_groups[ncp_rec->group]);
+            PROTO_ITEM_SET_GENERATED(ti);
         }
 
         exception_code = 0;
         message = NULL;
         if (ncp_rec && ncp_rec->request_ptvc) {
-            clear_repeat_vars();
             /*
              * We need to remember the results even if we
              * throw an exception dissecting this request,
@@ -7459,112 +7406,7 @@ dissect_ncp_request(tvbuff_t *tvb, packet_info *pinfo,
              * get to them. */
             request_value->req_cond_results = results;
         }
-        /* Construct the info string if necessary */
-        if (run_info_str) {
-            GPtrArray *parray;
-            char*   byte_string;
-            char    non_uni_string[1024];
-            int i, len;
-            field_info *finfo;
-            int info_type;
-
-            if (!request_value)
-            {
-                conversation = find_conversation(pinfo->fd->num, &pinfo->src, &pinfo->dst,
-                                                 PT_NCP, nw_connection, nw_connection, 0);
-                if (conversation != NULL) {
-                    /* find the record telling us the request made that caused
-                       this reply */
-                    request_value = ncp_hash_lookup(conversation, sequence, pinfo->fd->num);
-                }
-                if (!conversation || !request_value)
-                {
-                    return;
-                }
-            }
-
-            parray = proto_get_finfo_ptr_array(ncp_tree,
-                                               *ncp_rec->req_info_str->hf_ptr);
-            len = g_ptr_array_len(parray);
-
-            if (len > 0) {
-
-                col_set_str(pinfo->cinfo, COL_INFO, "C ");
-
-                finfo = (field_info *)g_ptr_array_index(parray, 0);
 
-                info_type = get_info_type((const gchar*) ncp_rec->req_info_str->first_string);
-
-                if (info_type != 0) {    /* Is this  a string or not? */
-
-                    if (info_type == 1) {   /* Is this bytes? */
-                        byte_string = bytes_to_str(wmem_packet_scope(), get_finfo_value_string(finfo), get_finfo_length(finfo));
-                        col_append_fstr(pinfo->cinfo, COL_INFO,
-                                        (const gchar*) ncp_rec->req_info_str->first_string,
-                                        byte_string);
-                    }
-                    else
-                    {
-                        if (info_type == 2) {   /* Is this a String? */
-                            uni_to_string(get_finfo_value_string(finfo), get_finfo_length(finfo), non_uni_string);
-                            col_append_fstr(pinfo->cinfo, COL_INFO,
-                                            (const gchar*) ncp_rec->req_info_str->first_string,
-                                            non_uni_string);
-                        }
-                        else
-                        {
-                            col_append_fstr(pinfo->cinfo, COL_INFO,
-                                            (const gchar*) ncp_rec->req_info_str->first_string,
-                                            get_finfo_value_string(finfo));
-                        }
-                    }
-                }
-                else
-                {
-                    col_append_fstr(pinfo->cinfo, COL_INFO,
-                                    (const gchar*) ncp_rec->req_info_str->first_string,
-                                    get_finfo_value_integer(finfo));
-                }
-            }
-            if (len > 1) {
-                for (i = 1; i < len; i++) {
-                    non_uni_string[0]='\0';
-                    finfo = (field_info *)g_ptr_array_index(parray, i);
-                    info_type = get_info_type((const gchar*) ncp_rec->req_info_str->repeat_string);
-
-                    if (info_type != 0) {    /* Is this  a string or not? */
-                        if (info_type == 1)
-                        {   /* Is this bytes? */
-                            byte_string = bytes_to_str(wmem_packet_scope(), get_finfo_value_string(finfo), get_finfo_length(finfo));
-                            col_append_fstr(pinfo->cinfo, COL_INFO,
-                                            (const gchar*) ncp_rec->req_info_str->repeat_string,
-                                            byte_string);
-                        }
-                        else
-                        {
-                            if (info_type == 2) {   /* Is this a String? */
-                                uni_to_string(get_finfo_value_string(finfo), get_finfo_length(finfo), non_uni_string);
-                                col_append_fstr(pinfo->cinfo, COL_INFO,
-                                                (const gchar*) ncp_rec->req_info_str->repeat_string,
-                                                non_uni_string);
-                            }
-                            else
-                            {
-                                col_append_fstr(pinfo->cinfo, COL_INFO,
-                                                (const gchar*) ncp_rec->req_info_str->repeat_string,
-                                                get_finfo_value_string(finfo));
-                            }
-                        }
-                    }
-                    else
-                    {
-                        col_append_fstr(pinfo->cinfo, COL_INFO,
-                                        (const gchar*) ncp_rec->req_info_str->repeat_string,
-                                        get_finfo_value_integer(finfo));
-                    }
-                }
-            }
-        }
         if (!request_value)
         {
              conversation = find_conversation(pinfo->fd->num, &pinfo->src, &pinfo->dst,
@@ -7610,8 +7452,6 @@ dissect_ncp_request(tvbuff_t *tvb, packet_info *pinfo,
                 dissect_ncp_8x20req(tvb, ncp_tree, 28, ncp_rec->func);
             }
         }
-        /* Free the temporary proto_tree */
-        CLEANUP_CALL_AND_POP_PFX(xx);
 
         /* Re-throw any exception. */
         if (exception_code != 0)
@@ -8454,12 +8294,6 @@ dissect_ncp_reply(tvbuff_t *tvb, packet_info *pinfo,
     guint8               conn_stat;
 
 
-#ifdef FAKE_TREE_IS_VISIBLE
-    if (ncp_tree) {
-        PTREE_DATA(ncp_tree)->visible=1;
-    }
-#endif
-
     if (!pinfo->fd->flags.visited) {
         /* Find the conversation whence the request would have come. */
         conversation = find_conversation(pinfo->fd->num, &pinfo->src, &pinfo->dst,
@@ -8700,7 +8534,6 @@ dissect_ncp_reply(tvbuff_t *tvb, packet_info *pinfo,
             else {
                 req_cond_results = NULL;
             }
-            clear_repeat_vars();
             ptvc = ptvcursor_new(ncp_tree, tvb, 8);
             process_ptvc_record(ptvc, pinfo, ncp_rec->reply_ptvc,
                                 req_cond_results, TRUE, ncp_rec, FALSE);
@@ -8749,9 +8582,6 @@ dissect_nds_request(tvbuff_t *tvb, packet_info *pinfo,
     const ncp_record        *ncp_rec = NULL;
     conversation_t          *conversation;
     ptvcursor_t             *ptvc = NULL;
-    proto_tree              *temp_tree = NULL;
-    gboolean                 run_req_cond = FALSE;
-    gboolean                 run_info_str = FALSE;
     guint8                   nds_verb = 0;
     const char              *verb_string = "";
     guint32                  nds_frag = 0;
@@ -8764,6 +8594,7 @@ dissect_nds_request(tvbuff_t *tvb, packet_info *pinfo,
     gboolean                 resolve_eid=FALSE;
     guint32                  global_flags=0, nds_prot_flags=0;
     int                      i;
+    proto_item              *ti;
 
     for (i = 0; i < 9; i++) {
         pvalues[i].vtype = 0;
@@ -8808,32 +8639,12 @@ dissect_nds_request(tvbuff_t *tvb, packet_info *pinfo,
         request_value->req_frame_num = pinfo->fd->num;
         request_value->req_frame_time=pinfo->fd->abs_ts;
 
-        /* If this is the first time we're examining the packet,
-         * check to see if this NCP type uses a "request condition".
-         * If so, we have to build a proto_tree because request conditions
-         * use display filters to work, and without a proto_tree,
-         * display filters can't possibly work. If we already have
-         * a proto_tree, then wonderful. If we don't, we need to build
-         * one. */
-        if (ncp_rec && !ncp_tree) {
-            run_req_cond = TRUE;
-        }
         /* Keep track of the Fragment number in the request for defrag logic */
         request_value->nds_frag_num = nds_frag;
     }
 
-    /* If we have to handle a request condition, or have to
-       add to the Info column, we need to construct a protocol
-       tree.  If we already have a proto_tree, then wonderful.
-       If we don't, we need to build one. */
-    if ((run_info_str || run_req_cond) && !ncp_tree) {
-        proto_item *ti;
-
-        temp_tree = proto_tree_create_root(pinfo);
-        proto_tree_set_visible(temp_tree, FALSE);
-        ti = proto_tree_add_item(temp_tree, proto_ncp, tvb, 0, -1, ENC_NA);
-        ncp_tree = proto_item_add_subtree(ti, ett_ncp);
-    }
+    ti = proto_tree_add_item(ncp_tree, proto_ncp, tvb, 0, -1, ENC_NA);
+    ncp_tree = proto_item_add_subtree(ti, ett_ncp);
 
     /* Get NDS Verb */
     if (nds_frag == 0xffffffff) {
@@ -11135,7 +10946,6 @@ dissect_nds_request(tvbuff_t *tvb, packet_info *pinfo,
         else {
             col_add_fstr(pinfo->cinfo, COL_INFO, "C NDS %s", verb_string);
         }
-        run_info_str = TRUE;
     }
     else {
         col_add_fstr(pinfo->cinfo, COL_INFO,
@@ -11144,20 +10954,6 @@ dissect_nds_request(tvbuff_t *tvb, packet_info *pinfo,
     }
 
     if (ncp_tree) {
-        /* If the dissection throws an exception, be sure to free
-         * the temporary proto_tree that was created. Because of the
-         * way the CLEANUP_PUSH macro works, we can't put it in an 'if'
-         * block; it has to be in the same scope as the terminating
-         * CLEANUP_POP or CLEANUP_POP_AND_ALLOC. So, we always
-         * call CLEANUP_POP and friends, but the value of temp_tree is
-         * NULL if no cleanup is needed, and non-null if cleanup is needed.
-         */
-        CLEANUP_PUSH(free_proto_tree, temp_tree);
-
-#ifdef FAKE_TREE_IS_VISIBLE
-        PTREE_DATA(ncp_tree)->visible=1;
-#endif
-
         request_value = ncp_hash_lookup(conversation, sequence, pinfo->fd->num);
         switch (type) {
         case NCP_BROADCAST_SLOT:
@@ -11169,7 +10965,6 @@ dissect_nds_request(tvbuff_t *tvb, packet_info *pinfo,
             ptvc = ptvcursor_new(ncp_tree, tvb, 7);
             if (ncp_rec && ncp_rec->request_ptvc)
             {
-                clear_repeat_vars();
                 process_ptvc_record(ptvc, pinfo, ncp_rec->request_ptvc, NULL, TRUE, ncp_rec, TRUE);
             }
             proto_tree_add_uint_format_value(ncp_tree, hf_ncp_func, tvb, 6, 1,
@@ -11313,9 +11108,6 @@ dissect_nds_request(tvbuff_t *tvb, packet_info *pinfo,
             ; /* nothing */
             break;
         }
-
-        /* Free the temporary proto_tree */
-        CLEANUP_CALL_AND_POP;
     }
 }
 
@@ -11335,16 +11127,10 @@ dissect_ping_req(tvbuff_t *tvb, packet_info *pinfo,
     const ncp_record    *ncp_rec = NULL;
     conversation_t      *conversation;
     ptvcursor_t         *ptvc = NULL;
-    proto_tree          *temp_tree = NULL;
     gint                 length_remaining = 0;
     guint32              nds_flags;
     guint32              ping_version;
-
-#ifdef FAKE_TREE_IS_VISIBLE
-    if (ncp_tree) {
-        PTREE_DATA(ncp_tree)->visible=1;
-    }
-#endif
+    proto_item          *ti;
 
     func = tvb_get_guint8(tvb, 6);
     subfunc = tvb_get_guint8(tvb, 7);
@@ -11384,35 +11170,12 @@ dissect_ping_req(tvbuff_t *tvb, packet_info *pinfo,
         request_value = ncp_hash_insert(conversation, sequence, ncp_rec, pinfo->fd->num);
         request_value->req_frame_num = pinfo->fd->num;
         request_value->req_frame_time=pinfo->fd->abs_ts;
-
-        /* If this is the first time we're examining the packet,
-         * check to see if this NCP type uses a "request condition".
-         * If so, we have to build a proto_tree because request conditions
-         * use display filters to work, and without a proto_tree,
-         * display filters can't possibly work. If we already have
-         * a proto_tree, then wonderful. If we don't, we need to build
-         * one. */
-        if (ncp_rec && !ncp_tree) {
-            proto_item *ti;
-
-            temp_tree = proto_tree_create_root(pinfo);
-            proto_tree_set_visible(temp_tree, FALSE);
-            ti = proto_tree_add_item(temp_tree, proto_ncp, tvb, 0, -1, ENC_NA);
-            ncp_tree = proto_item_add_subtree(ti, ett_ncp);
-        }
     }
 
-    if (ncp_tree) {
-        /* If the dissection throws an exception, be sure to free
-         * the temporary proto_tree that was created. Because of the
-         * way the CLEANUP_PUSH macro works, we can't put it in an 'if'
-         * block; it has to be in the same scope as the terminating
-         * CLEANUP_POP or CLEANUP_POP_AND_ALLOC. So, we always
-         * call CLEANUP_POP and friends, but the value of temp_tree is
-         * NULL if no cleanup is needed, and non-null if cleanup is needed.
-         */
-        CLEANUP_PUSH(free_proto_tree, temp_tree);
+    ti = proto_tree_add_item(ncp_tree, proto_ncp, tvb, 0, -1, ENC_NA);
+    ncp_tree = proto_item_add_subtree(ti, ett_ncp);
 
+    if (ncp_tree) {
         switch (type) {
 
         case NCP_BROADCAST_SLOT:
@@ -11454,13 +11217,9 @@ dissect_ping_req(tvbuff_t *tvb, packet_info *pinfo,
         }
         ptvc = ptvcursor_new(ncp_tree, tvb, 7);
         if (ncp_rec && ncp_rec->request_ptvc) {
-            clear_repeat_vars();
             process_ptvc_record(ptvc, pinfo, ncp_rec->request_ptvc, NULL, TRUE, ncp_rec, TRUE);
         }
         ptvcursor_free(ptvc);
-
-        /* Free the temporary proto_tree */
-        CLEANUP_CALL_AND_POP;
     }
 }