Re: Dissector functions and variables that could be static

[João Valverde via Wireshark-dev <wireshark-dev () wireshark org>]

Hi Martin,

As you said some functions may only be used by third party plugins so 
indiscriminately removing every exported but not used function would be 
a bad policy. Even if they're not actually being used right now, who 
knows, they may be part of some public API for plugins, so for use as 
needed. The considerate way to remove them would be to have a 
deprecation period.

Unfortunately debian symbols doesn't help here. It does not say anything 
interesting not already said by the static/exported C keywords,  because 
functions that may be internal, for example exported from epan to the 
wireshark GUI application (not intended to be part of a public API), 
must also be included in debian symbols.

So really IMO the analysis must be done case by case for each function. 
And of course the risk of breaking someone elses plugin is always there, 
however small, so I think it needs to proceed with some caution.


On 27/01/21 11:06, Martin Mathieson via Wireshark-dev wrote:
> My most recent MR 
> (https://gitlab.com/wireshark/wireshark/-/merge_requests/1829 
> <https://gitlab.com/wireshark/wireshark/-/merge_requests/1829>), has 
> come across some symbols that don't appear to be in used by our repo.
>
> dpkg-gensymbols: error: some symbols or patterns disappeared in the 
> symbols file: see diff output below
> 4934 
> <https://gitlab.com/wireshark/wireshark/-/jobs/989357035#L4934>dpkg-gensymbols: 
> warning: debian/libwireshark0/DEBIAN/symbols doesn't match completely 
> debian/libwireshark0.symbols
> 4935 
> <https://gitlab.com/wireshark/wireshark/-/jobs/989357035#L4935>--- 
> debian/libwireshark0.symbols (libwireshark0_3.5.0_amd64)
> 4936 
> <https://gitlab.com/wireshark/wireshark/-/jobs/989357035#L4936>+++ 
> dpkg-gensymbolsUhOwDI 2021-01-27 10:38:17.000000000 +0000
> 4937 <https://gitlab.com/wireshark/wireshark/-/jobs/989357035#L4937>@@ 
> -2124,7 +2124,7 @@
> 4938 
> <https://gitlab.com/wireshark/wireshark/-/jobs/989357035#L4938>wsp_vals_pdu_type_ext@Base 
> 1.9.1
> 4939 
> <https://gitlab.com/wireshark/wireshark/-/jobs/989357035#L4939>wsp_vals_status_ext@Base 
> 1.9.1
> 4940 
> <https://gitlab.com/wireshark/wireshark/-/jobs/989357035#L4940>xml_escape@Base 
> 1.9.1
> 4941 <https://gitlab.com/wireshark/wireshark/-/jobs/989357035#L4941>- 
> xml_get_attrib@Base 1.9.1
> 4942 <https://gitlab.com/wireshark/wireshark/-/jobs/989357035#L4942>- 
> xml_get_cdata@Base 1.9.1
> 4943 <https://gitlab.com/wireshark/wireshark/-/jobs/989357035#L4943>- 
> xml_get_tag@Base 1.9.1
> 4944 
> <https://gitlab.com/wireshark/wireshark/-/jobs/989357035#L4944>+#MISSING: 
> 3.5.0# xml_get_attrib@Base 1.9.1
> 4945 
> <https://gitlab.com/wireshark/wireshark/-/jobs/989357035#L4945>+#MISSING: 
> 3.5.0# xml_get_cdata@Base 1.9.1
> 4946 
> <https://gitlab.com/wireshark/wireshark/-/jobs/989357035#L4946>+#MISSING: 
> 3.5.0# xml_get_tag@Base 1.9.1
> 4947 
> <https://gitlab.com/wireshark/wireshark/-/jobs/989357035#L4947>zbee_zcl_init_cluster@Base 
> 2.5.2
>
> It may be possible that someone has a private dissector that uses 
> these xml accessor functions to try to pick out some interesting fields.
>
> I am guessing that I should have my script read 
> debian/libwireshark0.symbols to realise that these functions need to 
> be non-static, even if there it can't find any actual references?
>
> Martin
>
>
>
> On Tue, Jan 26, 2021 at 7:59 PM Martin Mathieson 
> <martin.r.mathieson () googlemail com 
> <mailto:martin.r.mathieson () googlemail com>> wrote:
>
>     I have done a bit more on this - I started picking off the ones at
>     the end of the (alphabetical) list - 2nd one is
>     https://gitlab.com/wireshark/wireshark/-/merge_requests/1817
>     <https://gitlab.com/wireshark/wireshark/-/merge_requests/1817>.
>     Please feel free if anyone feels motivated to tackle some of the
>     earlier ones.  The script is much tidier now, and also checks for
>     references from the ui folder (this removed around 20 from the
>     list), will take another pass through it before creating an MR
>     with it.
>
>     Sometimes I see that header files are being used as a way to
>     untangle the order or functions, but just doing some static
>     forward-declarations at the top of the C module would be better if
>     they are not shared.  I am leery of deleting functions that are
>     not being called, but if they've been that way for years they
>     probably don't matter.
>
>     Martin
>
>
>
>     On Sun, Jan 24, 2021 at 11:06 PM Martin Mathieson
>     <martin.r.mathieson () googlemail com
>     <mailto:martin.r.mathieson () googlemail com>> wrote:
>
>
>
>         On Sun, Jan 24, 2021 at 8:27 PM Jirka Novak
>         <j.novak () netsystem cz <mailto:j.novak () netsystem cz>> wrote:
>
>             Hi,
>
>               I checked the code I know:
>
>             > epan/dissectors/packet-rtp-events.c (00000000000001a0 D>
>             rtp_event_type_values_ext) is not referred to so could be
>             static? (in>
>             header)
>             It is used in UI, outside of dissectors. Therefore it
>             should be exported.
>
>
>         Yes, I can have the script also check for references from the
>         object files in ui to avoid reporting cases like this.
>
>             > epan/dissectors/packet-rtp.c (00000000000006d0 T
>             rtp_dyn_payload_remove)
>             > is not referred to so could be static? (in header)
>             > epan/dissectors/packet-rtp.c (0000000000000660 T
>             > rtp_dyn_payload_replace) is not referred to so could be
>             static? (in header)
>
>
>         I think these 2 functions can be removed.
>
>             > epan/dissectors/packet-rtps.c (0000000000000e20 D
>             class_id_enum_names)
>             > is not referred to so could be static?
>
>
>         This variable is able to become static.
>
>             That two functions are not used at all. Can we remove them?
>
>             Best regards,
>
>                   Jirka Novak
>
>
> ___________________________________________________________________________
> 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

___________________________________________________________________________
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