Re: Dissector functions and variables that could be static

[Martin Mathieson via Wireshark-dev <wireshark-dev () wireshark org>]

Even in that tree/version (which is from 9 years ago), packet-xml.c doesn't
call the function itself.  I don't see any out-of-tree commits to
packet-xml.c in the history of that tree. The only file that includes
packet-xml.h is packet-xmpp-utils.h (which is included by various XMPP
dissectors), but they didn't even exist in the repo/snapshot you pointed to.

Looking with 'git annotate',  the lines of these 3 functions were
re-formatted by Bill Meier in a cleanup change in 2011, and I'm not going
to dig further back than that.

I think I'd best just leave those 3 functions in, and will add them to an
exemption list in my script.  But this does show that investigating these
symbols can be time-consuming and inconclusive.

Martin


On Wed, Jan 27, 2021 at 1:46 PM Anders Broman <anders.broman () ericsson com>
wrote:

> Hi,
>
> Did some googling out of curiosity and found
> https://jelmer.uk/klaus/wireshark/blob/e738b556d72d4db5d7df85969c15117dedd0d063/epan/dissectors/packet-xml.c
>
> Search for “xml_get_attrib" So it seems it was part of packet-xml.c at
> some point so perhaps safe to remove…
>
> /Anders
>
>
>
> *From:* Wireshark-dev <wireshark-dev-bounces () wireshark org> *On Behalf Of
> *Martin Mathieson via Wireshark-dev
> *Sent:* den 27 januari 2021 13:27
> *To:* Developer support list for Wireshark <wireshark-dev () wireshark org>
> *Cc:* Martin Mathieson <martin.r.mathieson () googlemail com>
> *Subject:* Re: [Wireshark-dev] Dissector functions and variables that
> could be static
>
>
>
> Hi João,
>
>
>
> I agree that every function / variable needs to be looked at carefully,
> but more so if they have WS_DLL_PUBLIC in a header file.
>
>
>
> I will reinstate the XML functions in my change.  Hopefully, in other
> places I will find clear comments saying that they are provided for calling
> from private code or plugins, etc.
>
>
>
> The few occasions I've added to debian/libwireshark0.symbols have been to
> unbreak the pipeline when linking between dissectors and GUI code.  My pdcp
> key-setting functions don't even appear there, but I don't build Windows
> plugins.
>
>
>
> Martin
>
>
>
> On Wed, Jan 27, 2021 at 11:48 AM João Valverde via Wireshark-dev <
> wireshark-dev () wireshark org> wrote:
>
> 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), 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> 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 () googlemail com> wrote:
>
>
>
>
>
> On Sun, Jan 24, 2021 at 8:27 PM Jirka Novak <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> <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 <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
___________________________________________________________________________
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