["David Aggeler" <david_aggeler@xxxxxxxxxx>]

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);


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? 
With the currently implementation I cannot disable "SIP over TCP", but keep "SIP over UDP" on.

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)

>> 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.
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. 

Regards
David

-----Original Message-----
From: Wireshark-dev <wireshark-dev-bounces@xxxxxxxxxxxxx> On Behalf Of Peter Wu
Sent: Friday, March 23, 2018 00:26
To: Developer support list for Wireshark <wireshark-dev@xxxxxxxxxxxxx>
Subject: Re: [Wireshark-dev] heur_dissector_add()

On Thu, Mar 22, 2018 at 10:33:26PM +0100, David Aggeler wrote:
> Hi Peter,
> 
> Wow. How counter intuitive can things get.

Yes, I must admit that by first search attempt was "heuristic" which gave only one result in the dialog.

> >> The one of DICOM appears to be named "dicom_tcp" ("DICOM over TCP").
> 
> For me 'Enable/Disable Protocol' meant exactly that. Enable/Disable 
> the full protocol dissection But indeed, having some TCP ports configured for DICOM and the 'child protocol' deselected, disables the 'search on any port'.
> 
> The name "DICOM over TCP" is what was suggested to call the hook log time back, and I see many others do the same. In the new Dialog/Context that is plain wrong.
> So I guess hardly any of the dissectors that supported a 'search on any port' were converted to follow the new model. 

What do you mean by "the new model"? The move from the protocol preferences to the Enabled Protocols dialog? There were a lot more protocols that got converted from preference to the dialog thing, see commit 21e5a950ade6a20260b63b5f5c055c52ac07b599

> Is now the idea, that that level should be called 'dicom_any_tcp_port' / "DICOM on an TCP Port (heuristic)". That what I now intend to do. I.e.
> 
> From:    heur_dissector_add("tcp", dissect_dcm_heuristic, "DICOM over TCP", "dicom_tcp", proto_dcm, HEURISTIC_ENABLE);
> To:         heur_dissector_add("tcp", dissect_dcm_heuristic, "DICOM on any TCP port (heuristic)", "dicom_any_tcp_port", proto_dcm, HEURISTIC_ENABLE);
> 
> That would mean the 265 other places HEURISTIC_ENABLE is used, would need similar adaption, no?

"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".

Do you think it is sufficient to append "(heuristic)" automatically in the GUI after each description?

Kind regards,
Peter

> Regards
> David
> 
> -----Original Message-----
> From: Wireshark-dev <wireshark-dev-bounces@xxxxxxxxxxxxx> On Behalf Of 
> Peter Wu
> Sent: Thursday, March 22, 2018 19:51
> To: Developer support list for Wireshark <wireshark-dev@xxxxxxxxxxxxx>
> Subject: Re: [Wireshark-dev] heur_dissector_add()
> 
> Hey David,
> 
> On Thu, Mar 22, 2018 at 06:32:08PM +0100, David Aggeler wrote:
> > Hi Peter,
> > 
> > Thanks for the hint.  Ok. I just debugged and apparently the DICOM 
> > one as many others is DISSECTOR_TYPE_SIMPLE. Not sure how to change, 
> > but also not sure whether it is that relevant. However, I return 0 
> > when it does not match (more like new style)
> > 
> > If the dissectors are combined like this, basically old and new need to follow the same interface, otherwise every parent of call_dissector_through_handle() would need a type selector, and  I've not seen that anywhere else. 
> > 
> > Also from this comment & code, I'd conclude that the retuned number is the number of processed bytes.
> > In that sense, for me, 0 maps to FALSE in the heuristic and >0 maps to TRUE. 
> > 
> > I've now coded according to the README.heuristic. To me that reflects 0/FALSE, >0 TRUE.
> > 
> > >> you can enable or disable heuristics dissectors it in the Enabled Protocols menu.
> > 
> > I'm blind. All I can see there is Ethernet PW (CW heuristic). That one?
> 
> The one of DICOM appears to be named "dicom_tcp" ("DICOM over TCP").
> 
> Kind regards,
> Peter
> 
> > Regards
> > David
> > 
> > -----Original Message-----
> > From: Wireshark-dev <wireshark-dev-bounces@xxxxxxxxxxxxx> On Behalf 
> > Of Peter Wu
> > Sent: Thursday, March 22, 2018 17:29
> > To: Developer support list for Wireshark 
> > <wireshark-dev@xxxxxxxxxxxxx>
> > Subject: Re: [Wireshark-dev] heur_dissector_add()
> > 
> > Hi David,
> > 
> > On Thu, Mar 22, 2018 at 11:50:07AM +0100, David Aggeler wrote:
> > >  
> > > 
> > > I'm intending to re-enable the heuristic part in the DICOM dissector. 
> > > So I read though the updates readme and some other dissector, and 
> > > to my surprise, the return value of the heuristic still is 
> > > supposed to be boolean, where the static one returns int.
> > > 
> > >  
> > > 
> > > Implementation wise, by now I kind of only see 'return 
> > > tvb_captured_length(tvb)'. Wasn't this consumed bytes or needed 
> > > bytes at some point? I used to return the same int also in 
> > > heuristic part and never had an issue, but it looks wrong.
> > 
> > There are only two conventions for the integer return value, see the comment for call_dissector_through_handle in epan/packet.c:
> > 
> >     /* This function will return
> >      * old style dissector :
> >      *   length of the payload or 1 of the payload is empty
> >      * new dissector :
> >      *   >0  this protocol was successfully dissected and this was this protocol.
> >      *   0   this packet did not match this protocol.
> >      *
> >      * The only time this function will return 0 is if it is a new style dissector
> >      * and if the dissector rejected the packet.
> >      */
> > 
> > When I tried to change this such that the returned value is actually significant (https://www.wireshark.org/lists/wireshark-dev/201406/msg00221.html), I found that it would be quite risky to use the return value to signal reassembly.
> > 
> > > I did not understand that 8 years back, and I still don't. Does it 
> > > mean a heuristic can't re-assemble?
> > 
> > Heuristics can request reassembly, but then they must not reject the data (return true). But only do this when you are sure it is your protocol. To ensure that your normal (not heuristics) dissector is called in the future, you can use conversation_set_dissector.
> > 
> > > The other part that seems to have changed are the settings for this. 
> > > Is it not desired anymore, that the use can select at dissector 
> > > level, whether it shall do the heuristic math or not?
> > 
> > Rather than having a preference at every dissector, it is now taken of by in the core (see function "heur_dissector_add"). In the GUI, you can enable or disable heuristics dissectors it in the Enabled Protocols menu.
> > --
> > Kind regards,
> > Peter Wu
> > https://lekensteyn.nl
___________________________________________________________________________
Sent via:    Wireshark-dev mailing list <wireshark-dev@xxxxxxxxxxxxx>
Archives:    https://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
             mailto:wireshark-dev-request@xxxxxxxxxxxxx?subject=unsubscribe