Re: Wireshark-commits: [Wireshark-commits] rev 44161: /trunk/epan/dissectors/ /trunk/epan/dissectors/: packet-gmr1_bcch.c packet-gmr1_common.c packet-gmr1_rr.c

[mmann78 () netscape net]

I'll revert the GMR-1 display filter name changes and update checkdisplayfilter.pl to accept periods in the "base" 
filter name, but I will note their existance in another "FILE LIST" grouping in case other dissectors don't have a 
valuable reason to do so.


-----Original Message-----
From: Pascal Quantin <pascal.quantin () gmail com>
To: Developer support list for Wireshark <wireshark-dev () wireshark org>
Sent: Tue, Aug 21, 2012 5:21 pm
Subject: Re: [Wireshark-dev] Wireshark-commits: [Wireshark-commits] rev 44161: /trunk/epan/dissectors/ 
/trunk/epan/dissectors/: packet-gmr1_bcch.c packet-gmr1_common.c packet-gmr1_rr.c


2012/8/20  <mmann78 () netscape net>



Sylvain,
 
The checkdisplayfilter.pl script reflects my interpretation of the desired display filter format, and since there 
hasn't been that much feedback on the outputted results (with Pascal's comments on the GSM dissectors being the 
exception), I continue to plod along manually checking and possibly updating dissectors as they show up in the list.  
It's not set in stone, but lacking any officially documented rules, I thought it was turning into the defacto standard. 
 I'm more than willing to adapt the script, I just want the consistency expressed in bug 2794.  The problem may be 
defining "consistent".
 
I definitely struggled with the GMR-1 filters (whether they or the script should be changed), and that's one of the 
main reasons I intentionally made the change its own revision (makes reverting a little easier).  I got the impression 
that the GMR-1 protocols were more a "grouping" of protocols (like the ZigBee or SCSI protocols), than "subdissectors", 
which is why I went with the underscore separator over the period.  I don't see where most users would notice it 
because you shouldn't see much of a difference in the "autocompletion" when typing in a display filter since the 
"subdissectors" of common / rr / cc would still be at the top of the list.  The CCCH stuff (which appears to be an 
obvious mistake) came from either another similar dissector architecture, the "protocol filter name" or the naming of 
the hf_ variables in the registration array (where I found a common theme using their names).
 
The GMR-1 protocol follows one of the biggest reasons for the script - ensuring display filter names start with the 
"protocol filter name", followed by a period.  The problem I have is that I don't like the idea of having a period in 
the "protocol filter name" itself.  This check hasn't been added to the script yet (maybe even to the protocol 
registration code itself), but I have certainly considered it.  To me a period in the "protocol filter name" adds some 
confusion to what's being "autocompleted" and also suggests that a protocol may have been architected with multiple 
dissectors to (unnecessarily) break the code up into multiple source modules (for strictly reasons of size).  Multiple 
source modules for a protocol is somewhat discouraged as there are already 1000+ dissector files (with some larger than 
the totality of the GMR-1 code).  If the GMR-1 protocol was implemented in a single dissector/file, the 
checkdisplayfilter.pl script wouldn't have complained about the "subfilters" of common / rr / cc.




GMR-1 dissectors, like GSM-A dissectors, can be seen either as separate protocols belonging to the same family, or as a 
single protocol with sub dissectors:it depends a bit on your point of view :) Personnally I see them as a single one 
(like Sylvain) and was rather happy with the '.' separator instead of '_' one.
For example at the beginning in Wireshark there was a single packet-gsm_a.c file that became huge and was split into 
several files in r25915 and r25917.
When looking at checkfiltername.pl script, it looks like there are already variants allowed for PROTOABBREV ('-' vs '_' 
for example). Would allowing '.' vs '_' really be an issue? If think it would make sense for big protocols like GSM or 
GMR-1 (and there is maybe a few other ones). If accepted, we should be careful when reviewing patches to ensure that 
this capability is not used without any valuable reason.

Regards,
Pascal.


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

 

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