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.
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:
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?
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.
> 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?
