Wireshark-dev: [Wireshark-dev] Duplicate protocols in dissector tables

From: Michael Mann <mmann78@xxxxxxxxxxxx>
Date: Thu, 29 Oct 2015 12:01:45 -0400
I wrote a patch (https://code.wireshark.org/review/1405/) that may require discussion, so I thought I'd do it with a broader audience (because it impacts many dissectors whose individual authorship doesn't need to be added to a single Gerrit review)
 
The patch fixes bug 3949 (https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=3949) by enforcing that dissector tables that are used by Decode As can't have the same "protocol" (name) in multiple entries (exception - FT_STRING because the string value is used in the Decode As dialog, not the protocol name).  I've made a few patches in the past that fixed some of the duplicates by visual inspection, but this patch allows the code to do the work to ensure nothing was missed.
 
The part I felt was more "up for debate" was that I "defaulted" dissector tables to not allow duplicates.  As a test, I made all dissector tables not allow duplicates, then used printf and TShark to see how many duplicates there were.  If the dissector table wasn't used for Decode As, I would switch it to allowing duplicates.  Is that the way to go?  Should I "default" FT_STRING dissector tables to always allow duplicates?
 
The problem is that I'm limited to the dissector source in Wireshark and if there are dissector tables with known/intentional third-party dissectors with duplicate protocol registration, they will end up getting flagged.  I'd like to limit the number of follow-up commits with dissectors being corrected for allowing duplicates.  If you have specific dissector tables that you think should allow duplicates (or didn't like my "duplicate replacement"), please post in the review or email me privately.  The dialog I'm going for in the mailing list is for more general approaches (like all ASN.1 dissector tables should allow duplicates or you think the "default" should be to allow duplicates for all dissector tables)
 
I also plan to backport this to 2.0 (because I want the API change in there before it can't be changed).  Opinions on that welcome as well.