Thrift: Add support for expert info on exceptions in sub-dissectors

While working on some sub-dissector, it occurred to me that there was no easy way for sub-dissector to mark the exceptions with specific expert info. In fact, in some of the example sub-dissectors in the wiki, I had to use some workaround to mark the exceptions with expert info.

The first commit is there only to separate the documentation-related changes (comments only) that I saw since last update, I’ll squash everything before the merge.

The second commit is the real deal:

• New field in the public structure thrift_member_t to hold a pointer to possible expert info related to a struct (which exceptions really are).
• New public dissect_thrift_t_exception() function with one more parameter (comparing to struct).
• dissect_thrift_t_struct() implementation becomes just a call to the new function with the last parameter set to NULL.
• In the implementation, we attach the expert info (if provided) to the structure.

The third commit implement the changes suggested by point 3 below.

Now, I still have several points that might be worth discussing regarding the whole thing:

1. The new expert info field is only available for structures but to all structures, including unions. Would it be pertinent to make it available for all types in case the sub-dissector has the use for it?
   • All dissect_thrift_t_<type>() would have to be doubled or rather the new parameter would have to be added everywhere.
   • ❌ I would argue against it and consider the sub-dissectors to use this feature for exceptions only.
2. All existing sub-dissectors will have to update their code to handle this new element when dealing with structure. Do you think it’s safe enough this way?
   • The risk I see is that the uninitialized field leads to a crash if it happens to result in a non-NULL value but only for the sub-dissector which would have to be recompiled.
   • The change remains quite straightforward and is taken care of with :%s/\({ \)\(\.members = \w\+\)\( }\)/\1.s\2, .s.expert_info = NULL\3/g in Vim. It will of course change depending on the editor used but the point is that a global automatic replacement does the work.
   • Note: If we decide to generalize (first question), part of it can be taken care of in the TMFILL definition but containers, binary, string, and structs will still need some changes.
   • ✅ Since the change will only happen in a major version (hopefully 4.1/4.2 but it might be short so probably 4.3/4.4), the sub-dissectors will have to be recompiled anyway.
3. Using dissect_thrift_t_exception() directly does not seem practical, do you think it would be better to keep it internal?
   • Since the exception are always exclusive to each others and to the result, it seems much easier to use them through the definition of a union as demonstrated in the wiki page and the example sub-dissectors.
   • If the sub-dissector wanted to do it anyway, they would have to add a switch on the field id of the reply (available in thrift_opt->reply_field_id as demonstrated in the first TCustom toy example. This switch would have to be done everywhere, which quickly become tedious.
   • If made internal, I would probably rename it (dissect_thrift_t_struct_expert or something like that).
   • ✅ I would think it’s better to keep it internal for simplicity.
4. Since thrift_opt->reply_field_id would no longer be necessary given the new feature, do you think it would be pertinent to remove it in the process?
   • While it would slightly simplify the code in dissect_thrift_common() by removing a block and reduce the size of thrift_option_t, it might break existing sub-dissectors (the TCustom sample does not really count).
   • ❌ I don’t think the gain is enough to break existing sub-dissectors (the change is much bigger than a replace-all).
5. Do you know if there is any merged Thrift-based sub-dissector I should update in the process?
   • ✅ I did not find any while searching for packet-thrift.h in epan and plugin/epan directories so I think it’s fine.
6. Do you have any other remark/question/improvement proposal?

Everything is fine on my side, I started using the new capability in my sub-dissectors.

@AndersBroman @richvdh