Re: [Wireshark-dev] tools/check[hf|APIs|filtername].pl need updating?
tools/checkhf.pl still needs an update. Its current logic only matches "g?int hf_ = -1;" (find_remove_hf_defs) and then looks for missing entries in the hf array (using find_remove_hf_array_entries). To make it work for the new API, some code must be added to: 1. Detect the "header_field_info" definitions. 2. Find the array of "header_field_info" items. 3. Diff those. tools/checkfiltername.pl tries to check the filter name (matching it against the file name). This tool does not seem as essential as the others although it would be nice to update it eventually I guess. Let's keep the API, it does have some benefits (less boilerplate like "gint hf_blablabla = -1;") while the disadvantages are not that bad (less tool support, less commonly used). I am considering moving the (D)TLS dissectors to use the new API. Due to common fields shared between DTLS and TLS, it is currently implemented as an ugly big macro in epan/dissectors/packet-tls-utils.h. Adding new fields requires (1) adding the field variable as struct member, (2) adding the field definition to a big macro and (3) adding -1 to another macro. Alexis noticed that merging concurrent changes is complicated by (3) as manual action is required to correctly merge them. Note that due to this macro approach, existing tools already fail to check these dissectors. If the preprocessor expands the contents first, in theory it should be able to handle it after fixups for HFILL. Kind regards, Peter On Mon, Sep 24, 2018 at 06:03:33PM -0400, Jeff Morriss wrote: > [For completeness of this thread] Peter took care of checkAPIs in > https://code.wireshark.org/review/#/c/29754/ . > > On Thu, Sep 20, 2018 at 11:03 AM Maynard, Chris > wrote: > > > I'm not sure if anyone is waiting for my feedback, but just in case ... > > > > I'm not against Jakub's changes. There are benefits as he mentioned, > > particularly with the idea of auto-registration. The current problem as I > > see it is that in its current state, the check*.pl tools won't catch > > problems (such as the one I illustrated) like they used to be able to do. > > > > It would be great if all dissectors were converted to use the new API and > > then fields were auto-registered and then all #ifndef > > HAVE_HFI_SECTION_INIT ... #endif blocks could be removed. Of course that's > > a large task, so in the interim, maybe it's possible for the check*.pl > > tools to be updated to catch missing/duplicate/etc. entries in the hfi[] > > arrays? Otherwise, the more dissectors that are written this way, the > > greater the chance of errors being introduced but not being caught by the > > tools. > > > > Thoughts? > > - Chris > > > > -Original Message- > > From: Wireshark-dev [mailto:wireshark-dev-boun...@wireshark.org] On > > Behalf Of Michal Labedzki > > Sent: Wednesday, September 19, 2018 8:39 AM > > To: Alexis La Goutte ; Developer support list > > for Wireshark > > Subject: Re: [Wireshark-dev] tools/check[hf|APIs|filtername].pl need > > updating? > > > > I want to convert all Bluetooth dissectors to new proto tree API. Is it a > > good idea? > > > > wt., 18 wrz 2018 o 18:23 Alexis La Goutte > > napisał(a): > > > > > > Thanks Jakub for historic > > > > > > I think a good idea is revert to use "standard" API or write a tools > > > for convert old dissector to new API... > > > > > > Cheers > > > > > > On Tue, Sep 18, 2018 at 6:05 PM Jakub Zawadzki < > > darkjames...@darkjames.pl> wrote: > > >> > > >> Hi, > > >> > > >> W dniu 2018-09-18 16:56, Maynard, Chris napisał(a): > > >> > While investigating the transum-related crash, I had suspected some > > >> > unregistered hf's and ran the various tools like checkhf.pl. I > > >> > then noticed that a number of dissectors seemed to have changed a > > >> > bit from what I was used to before (...) > > >> > > >> These changes are quite old. For udp I did it in Aug 2013 > > >> (88eaebaedf2e19c493ea696f633463e4f2a9a757). > > >> > > >> some wireshark-dev threads: > > >> - https://www.wireshark.org/lists/wireshark-dev/201307/msg00222.html > > >> - thread continuation: > > >> https://www.wireshark.org/lists/wireshark-dev/201308/msg00035.html > > >> > > >> Nobody stopped me that time. > > >> > > >> > And I guess I missed the reasoning behind the restructuring, but > > >> > what is the purpose/benefit of this restructuring &
Re: [Wireshark-dev] tools/check[hf|APIs|filtername].pl need updating?
[For completeness of this thread] Peter took care of checkAPIs in https://code.wireshark.org/review/#/c/29754/ . On Thu, Sep 20, 2018 at 11:03 AM Maynard, Chris wrote: > I'm not sure if anyone is waiting for my feedback, but just in case ... > > I'm not against Jakub's changes. There are benefits as he mentioned, > particularly with the idea of auto-registration. The current problem as I > see it is that in its current state, the check*.pl tools won't catch > problems (such as the one I illustrated) like they used to be able to do. > > It would be great if all dissectors were converted to use the new API and > then fields were auto-registered and then all #ifndef > HAVE_HFI_SECTION_INIT ... #endif blocks could be removed. Of course that's > a large task, so in the interim, maybe it's possible for the check*.pl > tools to be updated to catch missing/duplicate/etc. entries in the hfi[] > arrays? Otherwise, the more dissectors that are written this way, the > greater the chance of errors being introduced but not being caught by the > tools. > > Thoughts? > - Chris > > -Original Message- > From: Wireshark-dev [mailto:wireshark-dev-boun...@wireshark.org] On > Behalf Of Michal Labedzki > Sent: Wednesday, September 19, 2018 8:39 AM > To: Alexis La Goutte ; Developer support list > for Wireshark > Subject: Re: [Wireshark-dev] tools/check[hf|APIs|filtername].pl need > updating? > > I want to convert all Bluetooth dissectors to new proto tree API. Is it a > good idea? > > wt., 18 wrz 2018 o 18:23 Alexis La Goutte > napisał(a): > > > > Thanks Jakub for historic > > > > I think a good idea is revert to use "standard" API or write a tools > > for convert old dissector to new API... > > > > Cheers > > > > On Tue, Sep 18, 2018 at 6:05 PM Jakub Zawadzki < > darkjames...@darkjames.pl> wrote: > >> > >> Hi, > >> > >> W dniu 2018-09-18 16:56, Maynard, Chris napisał(a): > >> > While investigating the transum-related crash, I had suspected some > >> > unregistered hf's and ran the various tools like checkhf.pl. I > >> > then noticed that a number of dissectors seemed to have changed a > >> > bit from what I was used to before (...) > >> > >> These changes are quite old. For udp I did it in Aug 2013 > >> (88eaebaedf2e19c493ea696f633463e4f2a9a757). > >> > >> some wireshark-dev threads: > >> - https://www.wireshark.org/lists/wireshark-dev/201307/msg00222.html > >> - thread continuation: > >> https://www.wireshark.org/lists/wireshark-dev/201308/msg00035.html > >> > >> Nobody stopped me that time. > >> > >> > And I guess I missed the reasoning behind the restructuring, but > >> > what is the purpose/benefit of this restructuring > >> > >> To sum up: > >> > >> Restructuring idea was to remove usage of int hf_foo, so you would > >> need only to declare header_field_info hfi_foo (unfortunate, you > >> still need to do it on top of file). > >> > >> Benefit is no more ints, so: > >> - proto_tree_ api checks if you passed header_field_info structure, > >> - You don't need to declare int hf_foo = -1; (bonus: binary size > >> smaller 4 bytes per hf), > >> - no need for table lookup in proto_tree_add_* > >> > >> > and use of HAVE_HFI_SECTION_INIT? > >> > >> Idea was that HFI_INIT(proto_bar) would put all protocol hfi's into > >> single binary section. This way wireshark could auto-register these > >> fields without need of some indirect array (bonus: binary size > >> smaller by sizeof(void *) per hfi). > >> > >> > >> After 5 years simple grep shows that only 36 dissectors are using > >> NEW_PROTO_TREE_API, so it seems that this API is not well known or > >> not liked. > >> If it makes problem I would suggest to drop it. > >> > >> Alexis suggested the same in 2015: > >> https://www.wireshark.org/lists/wireshark-dev/201508/msg00087.html > >> > >> > >> Jakub. > > > > > > > > > > > > > > > > > > > > > > CONFIDENTIALITY NOTICE: This message is the property of International Game > Technology PLC and/or its subsidiaries and may contain proprietary, > confidential or trade secret information. This message is intended solely > for the use of the addressee. If you are not the intended recipient and > have received this message in error, please delete this message from your > system
Re: [Wireshark-dev] tools/check[hf|APIs|filtername].pl need updating?
I'm not sure if anyone is waiting for my feedback, but just in case ... I'm not against Jakub's changes. There are benefits as he mentioned, particularly with the idea of auto-registration. The current problem as I see it is that in its current state, the check*.pl tools won't catch problems (such as the one I illustrated) like they used to be able to do. It would be great if all dissectors were converted to use the new API and then fields were auto-registered and then all #ifndef HAVE_HFI_SECTION_INIT ... #endif blocks could be removed. Of course that's a large task, so in the interim, maybe it's possible for the check*.pl tools to be updated to catch missing/duplicate/etc. entries in the hfi[] arrays? Otherwise, the more dissectors that are written this way, the greater the chance of errors being introduced but not being caught by the tools. Thoughts? - Chris -Original Message- From: Wireshark-dev [mailto:wireshark-dev-boun...@wireshark.org] On Behalf Of Michal Labedzki Sent: Wednesday, September 19, 2018 8:39 AM To: Alexis La Goutte ; Developer support list for Wireshark Subject: Re: [Wireshark-dev] tools/check[hf|APIs|filtername].pl need updating? I want to convert all Bluetooth dissectors to new proto tree API. Is it a good idea? wt., 18 wrz 2018 o 18:23 Alexis La Goutte napisał(a): > > Thanks Jakub for historic > > I think a good idea is revert to use "standard" API or write a tools > for convert old dissector to new API... > > Cheers > > On Tue, Sep 18, 2018 at 6:05 PM Jakub Zawadzki > wrote: >> >> Hi, >> >> W dniu 2018-09-18 16:56, Maynard, Chris napisał(a): >> > While investigating the transum-related crash, I had suspected some >> > unregistered hf's and ran the various tools like checkhf.pl. I >> > then noticed that a number of dissectors seemed to have changed a >> > bit from what I was used to before (...) >> >> These changes are quite old. For udp I did it in Aug 2013 >> (88eaebaedf2e19c493ea696f633463e4f2a9a757). >> >> some wireshark-dev threads: >> - https://www.wireshark.org/lists/wireshark-dev/201307/msg00222.html >> - thread continuation: >> https://www.wireshark.org/lists/wireshark-dev/201308/msg00035.html >> >> Nobody stopped me that time. >> >> > And I guess I missed the reasoning behind the restructuring, but >> > what is the purpose/benefit of this restructuring >> >> To sum up: >> >> Restructuring idea was to remove usage of int hf_foo, so you would >> need only to declare header_field_info hfi_foo (unfortunate, you >> still need to do it on top of file). >> >> Benefit is no more ints, so: >> - proto_tree_ api checks if you passed header_field_info structure, >> - You don't need to declare int hf_foo = -1; (bonus: binary size >> smaller 4 bytes per hf), >> - no need for table lookup in proto_tree_add_* >> >> > and use of HAVE_HFI_SECTION_INIT? >> >> Idea was that HFI_INIT(proto_bar) would put all protocol hfi's into >> single binary section. This way wireshark could auto-register these >> fields without need of some indirect array (bonus: binary size >> smaller by sizeof(void *) per hfi). >> >> >> After 5 years simple grep shows that only 36 dissectors are using >> NEW_PROTO_TREE_API, so it seems that this API is not well known or >> not liked. >> If it makes problem I would suggest to drop it. >> >> Alexis suggested the same in 2015: >> https://www.wireshark.org/lists/wireshark-dev/201508/msg00087.html >> >> >> Jakub. CONFIDENTIALITY NOTICE: This message is the property of International Game Technology PLC and/or its subsidiaries and may contain proprietary, confidential or trade secret information. This message is intended solely for the use of the addressee. If you are not the intended recipient and have received this message in error, please delete this message from your system. Any unauthorized reading, distribution, copying, or other use of this message or its attachments is strictly prohibited. ___ Sent via:Wireshark-dev mailing list Archives:https://www.wireshark.org/lists/wireshark-dev Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe
Re: [Wireshark-dev] tools/check[hf|APIs|filtername].pl need updating?
I want to convert all Bluetooth dissectors to new proto tree API. Is it a good idea? wt., 18 wrz 2018 o 18:23 Alexis La Goutte napisał(a): > > Thanks Jakub for historic > > I think a good idea is revert to use "standard" API > or write a tools for convert old dissector to new API... > > Cheers > > On Tue, Sep 18, 2018 at 6:05 PM Jakub Zawadzki > wrote: >> >> Hi, >> >> W dniu 2018-09-18 16:56, Maynard, Chris napisał(a): >> > While investigating the transum-related crash, I had suspected some >> > unregistered hf's and ran the various tools like checkhf.pl. I then >> > noticed that a number of dissectors seemed to have changed a bit from >> > what I was used to before (...) >> >> These changes are quite old. For udp I did it in Aug 2013 >> (88eaebaedf2e19c493ea696f633463e4f2a9a757). >> >> some wireshark-dev threads: >> - https://www.wireshark.org/lists/wireshark-dev/201307/msg00222.html >> - thread continuation: >> https://www.wireshark.org/lists/wireshark-dev/201308/msg00035.html >> >> Nobody stopped me that time. >> >> > And I guess I missed the reasoning behind the restructuring, but what >> > is the purpose/benefit of this restructuring >> >> To sum up: >> >> Restructuring idea was to remove usage of int hf_foo, so you would need >> only to declare header_field_info hfi_foo (unfortunate, you still need >> to do it on top of file). >> >> Benefit is no more ints, so: >> - proto_tree_ api checks if you passed header_field_info structure, >> - You don't need to declare int hf_foo = -1; (bonus: binary size >> smaller 4 bytes per hf), >> - no need for table lookup in proto_tree_add_* >> >> > and use of HAVE_HFI_SECTION_INIT? >> >> Idea was that HFI_INIT(proto_bar) would put all protocol hfi's into >> single binary section. This way wireshark could auto-register these >> fields >> without need of some indirect array (bonus: binary size smaller by >> sizeof(void *) per hfi). >> >> >> After 5 years simple grep shows that only 36 dissectors are using >> NEW_PROTO_TREE_API, so it seems that this API is not well known or not >> liked. >> If it makes problem I would suggest to drop it. >> >> Alexis suggested the same in 2015: >> https://www.wireshark.org/lists/wireshark-dev/201508/msg00087.html >> >> >> Jakub. >> ___ >> Sent via:Wireshark-dev mailing list >> Archives:https://www.wireshark.org/lists/wireshark-dev >> Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev >> mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe > > ___ > Sent via:Wireshark-dev mailing list > Archives:https://www.wireshark.org/lists/wireshark-dev > Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev > mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe -- Michał Łabędzki ___ Sent via:Wireshark-dev mailing list Archives:https://www.wireshark.org/lists/wireshark-dev Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe
Re: [Wireshark-dev] tools/check[hf|APIs|filtername].pl need updating?
Thanks Jakub for historic I think a good idea is revert to use "standard" API or write a tools for convert old dissector to new API... Cheers On Tue, Sep 18, 2018 at 6:05 PM Jakub Zawadzki wrote: > Hi, > > W dniu 2018-09-18 16:56, Maynard, Chris napisał(a): > > While investigating the transum-related crash, I had suspected some > > unregistered hf's and ran the various tools like checkhf.pl. I then > > noticed that a number of dissectors seemed to have changed a bit from > > what I was used to before (...) > > These changes are quite old. For udp I did it in Aug 2013 > (88eaebaedf2e19c493ea696f633463e4f2a9a757). > > some wireshark-dev threads: > - https://www.wireshark.org/lists/wireshark-dev/201307/msg00222.html > - thread continuation: > https://www.wireshark.org/lists/wireshark-dev/201308/msg00035.html > > Nobody stopped me that time. > > > And I guess I missed the reasoning behind the restructuring, but what > > is the purpose/benefit of this restructuring > > To sum up: > > Restructuring idea was to remove usage of int hf_foo, so you would need > only to declare header_field_info hfi_foo (unfortunate, you still need > to do it on top of file). > > Benefit is no more ints, so: > - proto_tree_ api checks if you passed header_field_info structure, > - You don't need to declare int hf_foo = -1; (bonus: binary size > smaller 4 bytes per hf), > - no need for table lookup in proto_tree_add_* > > > and use of HAVE_HFI_SECTION_INIT? > > Idea was that HFI_INIT(proto_bar) would put all protocol hfi's into > single binary section. This way wireshark could auto-register these > fields > without need of some indirect array (bonus: binary size smaller by > sizeof(void *) per hfi). > > > After 5 years simple grep shows that only 36 dissectors are using > NEW_PROTO_TREE_API, so it seems that this API is not well known or not > liked. > If it makes problem I would suggest to drop it. > > Alexis suggested the same in 2015: > https://www.wireshark.org/lists/wireshark-dev/201508/msg00087.html > > > Jakub. > ___ > Sent via:Wireshark-dev mailing list > Archives:https://www.wireshark.org/lists/wireshark-dev > Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev > mailto:wireshark-dev-requ...@wireshark.org > ?subject=unsubscribe ___ Sent via:Wireshark-dev mailing list Archives:https://www.wireshark.org/lists/wireshark-dev Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe
Re: [Wireshark-dev] tools/check[hf|APIs|filtername].pl need updating?
Hi, W dniu 2018-09-18 16:56, Maynard, Chris napisał(a): While investigating the transum-related crash, I had suspected some unregistered hf's and ran the various tools like checkhf.pl. I then noticed that a number of dissectors seemed to have changed a bit from what I was used to before (...) These changes are quite old. For udp I did it in Aug 2013 (88eaebaedf2e19c493ea696f633463e4f2a9a757). some wireshark-dev threads: - https://www.wireshark.org/lists/wireshark-dev/201307/msg00222.html - thread continuation: https://www.wireshark.org/lists/wireshark-dev/201308/msg00035.html Nobody stopped me that time. And I guess I missed the reasoning behind the restructuring, but what is the purpose/benefit of this restructuring To sum up: Restructuring idea was to remove usage of int hf_foo, so you would need only to declare header_field_info hfi_foo (unfortunate, you still need to do it on top of file). Benefit is no more ints, so: - proto_tree_ api checks if you passed header_field_info structure, - You don't need to declare int hf_foo = -1; (bonus: binary size smaller 4 bytes per hf), - no need for table lookup in proto_tree_add_* and use of HAVE_HFI_SECTION_INIT? Idea was that HFI_INIT(proto_bar) would put all protocol hfi's into single binary section. This way wireshark could auto-register these fields without need of some indirect array (bonus: binary size smaller by sizeof(void *) per hfi). After 5 years simple grep shows that only 36 dissectors are using NEW_PROTO_TREE_API, so it seems that this API is not well known or not liked. If it makes problem I would suggest to drop it. Alexis suggested the same in 2015: https://www.wireshark.org/lists/wireshark-dev/201508/msg00087.html Jakub. ___ Sent via:Wireshark-dev mailing list Archives:https://www.wireshark.org/lists/wireshark-dev Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe
[Wireshark-dev] tools/check[hf|APIs|filtername].pl need updating?
While investigating the transum-related crash, I had suspected some unregistered hf's and ran the various tools like checkhf.pl. I then noticed that a number of dissectors seemed to have changed a bit from what I was used to before, which lead me to the realization that at least some of these scripts no longer work as effectively as they once did, at least as far as I can tell. For example, if you remove an hf entry from the packet-wol.c's hf_register_info array and run checkhf.pl on packet-wol.c, you get output like this: $ tools/checkhf.pl epan/dissectors/packet-wol.c ERROR: NO ARRAY: epan/dissectors/packet-wol.c: hf_wol_passwd ... but if you remove an entry from the packet-udp.c's header_field_info, such as _udp_srcport, then checkhf.pl won't detect it. Later you end up with a dissector bug when running Wireshark of the form: 10:44:53.343 Warn Dissector bug, protocol UDP, in packet 1: proto.c:3377: failed assertion "(guint)hfinfo->id < gpa_hfinfo.len" (Unregistered hf!) While it's nice to get the warning, the bug would have been caught much earlier using the checkhf.pl tool had the code been structured like it used to be. Do these tools need to be updated to detect problems that were easily caught previously? And I guess I missed the reasoning behind the restructuring, but what is the purpose/benefit of this restructuring and use of HAVE_HFI_SECTION_INIT? Thanks. - Chris CONFIDENTIALITY NOTICE: This message is the property of International Game Technology PLC and/or its subsidiaries and may contain proprietary, confidential or trade secret information. This message is intended solely for the use of the addressee. If you are not the intended recipient and have received this message in error, please delete this message from your system. Any unauthorized reading, distribution, copying, or other use of this message or its attachments is strictly prohibited. ___ Sent via:Wireshark-dev mailing list Archives:https://www.wireshark.org/lists/wireshark-dev Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe
