Wireshark · Wireshark-dev: Re: [Wireshark-dev] Dissector functions and variables that could be static

[Martin Mathieson <martin.r.mathieson@xxxxxxxxxxxxxx>]

My most recent MR (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

4934dpkg-gensymbols: warning: debian/libwireshark0/DEBIAN/symbols doesn't match completely debian/libwireshark0.symbols

4935--- debian/libwireshark0.symbols (libwireshark0_3.5.0_amd64)

4936+++ dpkg-gensymbolsUhOwDI 2021-01-27 10:38:17.000000000 +0000

4937@@ -2124,7 +2124,7 @@

4938 wsp_vals_pdu_type_ext@Base 1.9.1

4939 wsp_vals_status_ext@Base 1.9.1

4940 xml_escape@Base 1.9.1

4941- xml_get_attrib@Base 1.9.1

4942- xml_get_cdata@Base 1.9.1

4943- xml_get_tag@Base 1.9.1

4944+#MISSING: 3.5.0# xml_get_attrib@Base 1.9.1

4945+#MISSING: 3.5.0# xml_get_cdata@Base 1.9.1

4946+#MISSING: 3.5.0# xml_get_tag@Base 1.9.1

4947 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@xxxxxxxxxxxxxx> 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. 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@xxxxxxxxxxxxxx> wrote:

On Sun, Jan 24, 2021 at 8:27 PM Jirka Novak <j.novak@xxxxxxxxxxxx> 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