Re: heur_dissector_add()

[Peter Wu <peter () lekensteyn nl>]

On Fri, Mar 23, 2018 at 09:07:58AM +0100, David Aggeler wrote:
> Hi Peter,
>
> > > "DICOM on any TCP port" sounds more logical for a single protocol, but there are actually protocols that run on 
> > > other 
> > > transports. One example is SIP which has "SIP over SCTP", "SIP over TURN", "SIP over TCP" and "SIP over UDO".
>
> "SIP over TCP" / "SIP over UDP" are done the same ways as the DICOM is done. So it's not all bindings I agree, but 
> many.
> Therefore let's focus on IP based protocols for a moment. To me
>
> protocolx_over_tcp_static             should be different to 
> protocolx_over_tcp_heuristic
>
> A) In my understanding, the TCP static binding is done with the following line. Here I cannot specify a name.
> This is borderline questionable, because it’s a UI preference function.
>
>       dissector_add_uint_range_with_preference("tcp.port", DICOM_DEFAULT_RANGE, dcm_handle);

Seems correct, the protocol name is taken from the handle which was
returned by register_dissector or something like that.

> B) The TCP heuristic binding is done with
>
>       heur_dissector_add() 
>
>
> Is my understanding of A) correct? If yes, I'd prefer it more explicit. If no, what am I missing on the static 
> assignment? 

Explicit how? Dissectors do not always have to be registered with a
table such as tcp.port. For example, the "data" dissector is looked up
by dissectors using find_dissector("data") and then used directly.
That's likely why registration of dissectors is separate from linking
them to something like tcp.port.

> With the currently implementation I cannot disable "SIP over TCP", but keep "SIP over UDP" on.

Are you sure? sip_tcp and sip_udp are different heuristics dissectors, I
presume that they can be disabled via the GUI as well.

> Form a tree perspective, the following would make sense. And toggling the parent should toggle the children.
>
> [ ] ProtocolX 
>       [ ] ProtocolX over TCP (static)
>       [ ] ProtocolX over TCP (heuristic)

I agree it would make more sense. For single protocols, there would just
be a single item (which could be just heuristics or "static"). For
protocols with multiple registrations, a tree would be visible.

Perhaps it could even append a comment to the protocol ("1 heuristics
dissector hidden") in case a search query is active. Or maybe even
display all subtree items since you are probably looking for a protocol
rather than a specific instance.

> > > Do you think it is sufficient to append "(heuristic)" automatically in the GUI after each description?
>
> If my understanding of A) is correct, then yes, this comes pretty close to the original idea in the 2015 discussion 
> thread.

I guess you are referring to this "Enabling/disabling ANY heuristic
dissector" discussion?
https://www.wireshark.org/lists/wireshark-dev/201507/msg00027.html

> It's not a single flag but better than nothing. Two buttons to 'Enable/Disable all heuristic at once'  would pretty 
> much be that part. Or better 'Enable/Disable Visible'. Then with keyword, one can filter first and disable / enable 
> that subset.

So when would you like to enable all heuristics dissectors, for all
protocols, but not enable the "static" dissector (as done by
Enable/Disable All)? "Enable/Disable all visible" sounds reasonable, but
perhaps we can do better. Do you have a use case in mind?

Kind regards,
Peter
___________________________________________________________________________
Sent via:    Wireshark-dev mailing list <wireshark-dev () wireshark org>
Archives:    https://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
             mailto:wireshark-dev-request () wireshark org?subject=unsubscribe