> On Jul 5, 2015, at 7:02 PM, mmann78@xxxxxxxxxxxx wrote:
>
> I uploaded a patch to Gerrit that allows enabling/disabling of any heuristic dissector (https://code.wireshark.org/review/9508/).
>
> Some comments about the patch (others are welcome to add more):
> 1. Not sure how to best express the relationship between the "name" of the heuristic dissector and its "parent"/table name. For example, the AD-win Config protocol has a heuristic dissector that goes on top of TCP and UDP. Each instance (TCP or UDP) can be enabled/disabled separately. Requiring an individual name for each heuristic dissector sounds like a bit too much to ask. Right now they are slash (/) delimited in the GUI and comma delimited in the "disabled_heuristics" file.
I think the slash delimiter in the GUI is ok - not great, but I can’t think of anything more clear. Perhaps have the Column title also be slash delimited, like “Heuristic Protocol/Underlying Protocol”? (or some better word for “Underlying”)
> 2. These "preferences" are read right after the enable/disable protocols are read/applied. This seemed like a logical place for it (since I added support for enable/disable heuristics in disabled_protos.c), but I'm not sure how that effects the "general" protocol preferences. Both manipulate the same heuristic dissector list, so "last one would win”.
I think all the individual preferences to enable/disable a heuristic should be removed. Are you talking about some other “general" protocol preference?
One note: this would mean we can’t ship the Qt-based GUI until the Enabled Protocols dialog box is available in it.
> 3. I'd like to remove any individual dissector preferences that enable/disable a heuristic dissector (future patch) using the same logic/justification as Decode As. Not sure how to handle heuristic dissectors that should be off by default. heur_dissector_set_enabled(..., FALSE) can be used, but that won't be "overridden" (enabled) by the fact that the heuristic dissector ISN'T in the disabled heuristics file.
I think we should change all the heur_dissector_add() function calls to take a boolean for whether it is enable by default or not; that way developers of future heuristic functions have to think about it and can’t forget. The heur_dissector_set_enabled() should be removed.
> 4. I understand the "feature" of enabling/disabling a (heuristic dissector) preference from the context menu, and that could be justification/argument for keeping it (the preference). Maybe just "appending" the context menu preferences for any protocol that has a heuristic dissector would be a good compromise?
You can currently disable a protocol itself with the context menu, without them having a preference for doing so, right? So I suggest we just follow that same behavior for heuristics, whatever that is. :)
-hadriel
