Wireshark-dev: [Wireshark-dev] When to use tree != NULL check?

Date: Mon, 10 Sep 2012 22:37:45 -0400 (EDT)
In working on bug 3534, one of the review comments was that a "tree != NULL check" placed near the start of the dissector should be removed as it prevented some of the COL_INFO/expert info data from being populated if tree was NULL.  While I agree the "tree != NULL" check should probably be moved to just after setting the COL_PROTOCOL column (and clearing the COL_INFO column), I didn't think it should be removed entirely.  This prompted the following response from Jeff Morris:

"Yes, the tree==NULL check is for faster processing.  Some people care very much
about speed [probably because they often work with larger files].  Wireshark
with no coloring rules (how I normally work) or tshark (without "-V") both
stand to benefit from if(tree) checks.

But it's all a question of effort vs. reward.  If you've got a function with
100 proto_tree_add_item()s that don't affect the columns or expert info then
you could easily trade one if(tree) for 100 function calls which then end up
taking the shortcut out when tree==NULL.  OTOH if you've got column updates and
expert infos hiding in there (or if you're tracking the offset into the tvb)
then you're probably better off without the if(tree) check."
 
 
 
I guess I've always used the rule that simple [1] dissectors (no matter how large) should all have the tree != NULL check before any dissection really takes place.  Most of the "expert info" I've seen is attached to "tree items" along the lines of "field validation" (command/value not supported/recognized, length incorrect, etc).  Without the tree, they don't seem very useful.   I've also seen dissectors that appear to be more geared towards tshark (lots of data in COL_INFO) than Wireshark, as I assume that's the developer's preferred "output".  I guess I'm looking for more hard and fast rules to apply that will equally benefit Wireshark and tshark users (minimizing effort, maximizing reward).  Is the "tree != NULL" check something I should probably just avoid altogether?  For "not simple" dissectors, I definitely agree having lots of "tree != NULL" checks isn't worth the effort.
 
 
 
[1] I consider a dissector without things like conversation data, fragmentation, "internal" states, or the need for pinfo->fd.visited, etc. to be "simple".