Re: Dissector functions and variables that could be static

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

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