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

[Martin Mathieson via Wireshark-dev]

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 
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  *On Behalf Of
> *Martin Mathieson via Wireshark-dev
> *Sent:* den 27 januari 2021 13:27
> *To:* Developer support list for Wireshark 
> *Cc:* Martin Mathieson 
> *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.0 +
>
> 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/-/j

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

[Anders Broman via Wireshark-dev]

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  On Behalf Of Martin 
Mathieson via Wireshark-dev
Sent: den 27 januari 2021 13:27
To: Developer support list for Wireshark 
Cc: Martin Mathieson 
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 
mailto: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

 <https://gitlab.com/wireshark/wireshark/-/jobs/989357035#L4934> 
4934dpkg-gensymbols: warning: debian/libwireshark0/DEBIAN/symbols doesn't match 
completely debian/libwireshark0.symbols

 <https://gitlab.com/wireshark/wireshark/-/jobs/989357035#L4935> 4935--- 
debian/libwireshark0.symbols (libwireshark0_3.5.0_amd64)

 <https://gitlab.com/wireshark/wireshark/-/jobs/989357035#L4936> 4936+++ 
dpkg-gensymbolsUhOwDI 2021-01-27 10:38:17.0 +

 <https://gitlab.com/wireshark/wireshark/-/jobs/989357035#L4937> 4937@@ -2124,7 
+2124,7 @@

 <https://gitlab.com/wireshark/wireshark/-/jobs/989357035#L4938> 4938 
wsp_vals_pdu_type_ext@Base 1.9.1

 <https://gitlab.com/wireshark/wireshark/-/jobs/989357035#L4939> 4939 
wsp_vals_status_ext@Base 1.9.1

 <https://gitlab.com/wireshark/wireshark/-/jobs/989357035#L4940> 4940 
xml_escape@Base 1.9.1

 <https://gitlab.com/wireshark/wireshark/-/jobs/989357035#L4941> 4941- 
xml_get_attrib@Base 1.9.1

 <https://gitlab.com/wireshark/wireshark/-/jobs/989357035#L4942> 4942- 
xml_get_cdata@Base 1.9.1

 <https://gitlab.com/wireshark/wireshark/-/jobs/989357035#L4943> 4943- 
xml_get_tag@Base 1.9.1

 <https://gitlab.com/wireshark/wireshark/-/jobs/989357035#L4944> 4944+#MISSING: 
3.5.0# xml_get_attrib@Base 1.9.1

 <https://gitlab.com/wireshark/wireshark/-/jobs/989357035#L4945> 4945+#MISSING: 
3.5.0# xml_get_cdata@Base 1.9.1

 <https://gitlab.com/wireshark/wireshark/-/jobs/989357035#L4946> 4946+#MISSING: 
3.5.0# xml_get_tag@Base 1.9.1

 <https://gitlab.com/wireshark/wireshark/-/jobs/989357035#L4947> 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 
mailto:martin.r.mathie...@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

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

[Martin Mathieson via Wireshark-dev]

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 
> dpkg-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.0 +
> 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.mathie...@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.mathie...@googlemail.com> wrote:
>>
>>>
>>>
>>> On Sun, Jan 24, 2021 at 8:27 P

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

[João Valverde via Wireshark-dev]

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 
dpkg-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.0 +
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 
> 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
mailto:martin.r.mathie...@googlemail.com>> wrote:



On Sun, Jan 24, 2021 at 8:27 PM Jirka Novak
mailto:j.no...@netsystem.cz>> wrote:

Hi,

  I checked the code I know:

> epan/dissectors/packet-rtp-events.c (01a0 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 (06d0 T

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

[Martin Mathieson via Wireshark-dev]

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 
dpkg-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.0 +
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.mathie...@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.mathie...@googlemail.com> wrote:
>
>>
>>
>> On Sun, Jan 24, 2021 at 8:27 PM Jirka Novak  wrote:
>>
>>> Hi,
>>>
>>>   I checked the code I know:
>>>
>>> > epan/dissectors/packet-rtp-events.c (01a0 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 (06d0 T rtp_dyn_payload_remove)
>>> > is not referred to so could be static? (in header)
>>> > epan/dissectors/packet-rtp.c (0660 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 (0e20 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 
Archives:https://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
 mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe

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

[Martin Mathieson via Wireshark-dev]

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.mathie...@googlemail.com> wrote:

>
>
> On Sun, Jan 24, 2021 at 8:27 PM Jirka Novak  wrote:
>
>> Hi,
>>
>>   I checked the code I know:
>>
>> > epan/dissectors/packet-rtp-events.c (01a0 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 (06d0 T rtp_dyn_payload_remove)
>> > is not referred to so could be static? (in header)
>> > epan/dissectors/packet-rtp.c (0660 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 (0e20 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 
Archives:https://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
 mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe

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

[Martin Mathieson via Wireshark-dev]

On Sun, Jan 24, 2021 at 8:27 PM Jirka Novak  wrote:

> Hi,
>
>   I checked the code I know:
>
> > epan/dissectors/packet-rtp-events.c (01a0 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 (06d0 T rtp_dyn_payload_remove)
> > is not referred to so could be static? (in header)
> > epan/dissectors/packet-rtp.c (0660 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 (0e20 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 
Archives:https://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
 mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe

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

[Jirka Novak]

Hi,

  I checked the code I know:

> epan/dissectors/packet-rtp-events.c (01a0 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.

> epan/dissectors/packet-rtp.c (06d0 T rtp_dyn_payload_remove)
> is not referred to so could be static? (in header)
> epan/dissectors/packet-rtp.c (0660 T
> rtp_dyn_payload_replace) is not referred to so could be static? (in header)
> epan/dissectors/packet-rtps.c (0e20 D class_id_enum_names)
> is not referred to so could be static?

That two functions are not used at all. Can we remove them?

Best regards,

Jirka Novak
___
Sent via:Wireshark-dev mailing list 
Archives:https://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
 mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe

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

[Martin Mathieson via Wireshark-dev]

Hi Moshe,

It would be possible. I would still like to hear from people if they think
it is worth doing, and we would need to clear the current cases (perhaps
with a list of agreed exemptions, like the PDCP key-setting functions I
mentioned).  I will likely start picking off issues when I have time
regardless, and I might as well clean up and submit the script.  If there
were lots of grey cases / exemptions it may not be suitable for causing CI
(or at least for causing the pipeline to fail).

Like the other check_*.py scripts I've added to tools, it has an option to
check individual files, or from recent commits, etc.  It doesn't take very
long to run - I timed 8s for a single dissector, or 37s for all dissectors.

The script works by looking at the output of 'nm' over the dissector object
files, but it also does a crude check for declarations in header files, so
it will need the path to the build folder to be passed in.  In
.gitlab-ci.yml (build-ubuntu), the build folder is inside the
source/checkout folder, so this would be simple.  This check could maybe be
done after the checking build in ubuntu-clang-other-tests (as we need the
object files)?

Martin

On Sat, Jan 23, 2021 at 11:57 PM Moshe Kaplan 
wrote:

> Would it also be possible to build this script into a GitLab CI job, to
> detect and prevent the issue from recurring?
>
> Moshe
>
> On Sat, Jan 23, 2021 at 11:45 AM Martin Mathieson via Wireshark-dev <
> wireshark-dev@wireshark.org> wrote:
>
>> Hi,
>>
>> I wrote a script to check whether variables and functions in dissector
>> modules that were not static were:
>> - not called by any other dissector modules (including dissectors.c)
>> - mentioned in the header file corresponding to that dissector
>>
>> Reasons to clean up these cases could include:
>> - it pollutes a large namespace (most, but not all, of the symbols
>> include the protocol name so likely won't clash)
>> - it will make it easier for people to spot and delete functions that are
>> not actually being called
>> - it sets a better example for people to copy when adding new code
>> - there may even be some cases where the compiler is able to optimise
>> better, don't know if it might speed up linking a little?
>>
>> What do others think - does it sound like it could be worth it?
>>
>> Where there is no extern or declaration in the header file, I think we
>> can safely try to use static.  If it is not used even by the dissector
>> itself (which the script doesn't currently check, but could...) it can
>> presumably be deleted (unless we know that someone is getting ready to use
>> it soon).
>>
>> Where the variable or function is mentioned in the dissector's header
>> file, in a lot of cases it could be removed from the header file and made
>> static.  I do have some cases where I call functions only from private
>> dissectors to set decryption keys, i.e.
>>
>> epan/dissectors/packet-pdcp-lte.c (3670 T
>> set_pdcp_lte_rrc_ciphering_key) is not referred to so could be static? (in
>> header)
>> epan/dissectors/packet-pdcp-lte.c (3730 T
>> set_pdcp_lte_rrc_integrity_key) is not referred to so could be static? (in
>> header)
>> epan/dissectors/packet-pdcp-lte.c (37f0 T
>> set_pdcp_lte_up_ciphering_key) is not referred to so could be static? (in
>> header)
>> epan/dissectors/packet-pdcp-nr.c (33b0 T
>> set_pdcp_nr_rrc_ciphering_key) is not referred to so could be static? (in
>> header)
>> epan/dissectors/packet-pdcp-nr.c (3470 T
>> set_pdcp_nr_rrc_integrity_key) is not referred to so could be static? (in
>> header)
>> epan/dissectors/packet-pdcp-nr.c (3530 T
>> set_pdcp_nr_up_ciphering_key) is not referred to so could be static? (in
>> header)
>> epan/dissectors/packet-pdcp-nr.c (35f0 T
>> set_pdcp_nr_up_integrity_key) is not referred to so could be static? (in
>> header)
>>
>> but I imagine there are not many more like this.  One clue that people
>> might be making references from a Windows plugin could be the presence of
>> WS_DLL_PUBLIC (script doesn't currently try to check).
>>
>> I currently don't check dissectors that are generated (otherwise there'd
>> be around 1000 other 'issues').  The dcerpc ones seem to declare lots of
>> functions in header files.
>>
>> Here is the output of my script against master:
>>
>> epan/dissectors/packet-ncp-sss.c (1130 T dissect_sss_reply)
>> is not referred to so could be static? (in header)
>> epan/dissectors/packet-ncp-sss.c (0d20 T dissect_sss_request)
>> is not referred to so could be static? (in header)
>> epan/dissectors/packet-ncp.c (0040 D ett_ncp) is not referred
>> to so could be static?
>> epan/dissectors/packet-ncp.c (0034 D ett_nds_segment) is not
>> referred to so could be static?
>> epan/dissectors/packet-ncp.c (0038 D ett_nds_segments) is not
>> referred to so could be static?
>> epan/dissectors/packet-ncp.c ( D ncp_nds_verb_vals

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

[Moshe Kaplan]

Would it also be possible to build this script into a GitLab CI job, to
detect and prevent the issue from recurring?

Moshe

On Sat, Jan 23, 2021 at 11:45 AM Martin Mathieson via Wireshark-dev <
wireshark-dev@wireshark.org> wrote:

> Hi,
>
> I wrote a script to check whether variables and functions in dissector
> modules that were not static were:
> - not called by any other dissector modules (including dissectors.c)
> - mentioned in the header file corresponding to that dissector
>
> Reasons to clean up these cases could include:
> - it pollutes a large namespace (most, but not all, of the symbols include
> the protocol name so likely won't clash)
> - it will make it easier for people to spot and delete functions that are
> not actually being called
> - it sets a better example for people to copy when adding new code
> - there may even be some cases where the compiler is able to optimise
> better, don't know if it might speed up linking a little?
>
> What do others think - does it sound like it could be worth it?
>
> Where there is no extern or declaration in the header file, I think we can
> safely try to use static.  If it is not used even by the dissector itself
> (which the script doesn't currently check, but could...) it can presumably
> be deleted (unless we know that someone is getting ready to use it soon).
>
> Where the variable or function is mentioned in the dissector's header
> file, in a lot of cases it could be removed from the header file and made
> static.  I do have some cases where I call functions only from private
> dissectors to set decryption keys, i.e.
>
> epan/dissectors/packet-pdcp-lte.c (3670 T
> set_pdcp_lte_rrc_ciphering_key) is not referred to so could be static? (in
> header)
> epan/dissectors/packet-pdcp-lte.c (3730 T
> set_pdcp_lte_rrc_integrity_key) is not referred to so could be static? (in
> header)
> epan/dissectors/packet-pdcp-lte.c (37f0 T
> set_pdcp_lte_up_ciphering_key) is not referred to so could be static? (in
> header)
> epan/dissectors/packet-pdcp-nr.c (33b0 T
> set_pdcp_nr_rrc_ciphering_key) is not referred to so could be static? (in
> header)
> epan/dissectors/packet-pdcp-nr.c (3470 T
> set_pdcp_nr_rrc_integrity_key) is not referred to so could be static? (in
> header)
> epan/dissectors/packet-pdcp-nr.c (3530 T
> set_pdcp_nr_up_ciphering_key) is not referred to so could be static? (in
> header)
> epan/dissectors/packet-pdcp-nr.c (35f0 T
> set_pdcp_nr_up_integrity_key) is not referred to so could be static? (in
> header)
>
> but I imagine there are not many more like this.  One clue that people
> might be making references from a Windows plugin could be the presence of
> WS_DLL_PUBLIC (script doesn't currently try to check).
>
> I currently don't check dissectors that are generated (otherwise there'd
> be around 1000 other 'issues').  The dcerpc ones seem to declare lots of
> functions in header files.
>
> Here is the output of my script against master:
>
> epan/dissectors/packet-ncp-sss.c (1130 T dissect_sss_reply) is
> not referred to so could be static? (in header)
> epan/dissectors/packet-ncp-sss.c (0d20 T dissect_sss_request)
> is not referred to so could be static? (in header)
> epan/dissectors/packet-ncp.c (0040 D ett_ncp) is not referred
> to so could be static?
> epan/dissectors/packet-ncp.c (0034 D ett_nds_segment) is not
> referred to so could be static?
> epan/dissectors/packet-ncp.c (0038 D ett_nds_segments) is not
> referred to so could be static?
> epan/dissectors/packet-ncp.c ( D ncp_nds_verb_vals) is not
> referred to so could be static?
> epan/dissectors/packet-ncp.c (00e4 D proto_ncp) is not
> referred to so could be static?
> epan/dissectors/packet-netmon.c (3790 T netmon_sid_field) is
> not referred to so could be static? (in header)
> epan/dissectors/packet-netrom.c (0080 D op_code_vals_abbrev)
> is not referred to so could be static?
> epan/dissectors/packet-netrom.c ( D op_code_vals_text) is
> not referred to so could be static?
> epan/dissectors/packet-nstrace.c ( T add35records) is not
> referred to so could be static?
> epan/dissectors/packet-nwp.c ( D nwp_type_vals) is not
> referred to so could be static?
> epan/dissectors/packet-ocfs2.c ( D ext_dlm_magic) is not
> referred to so could be static?
> epan/dissectors/packet-oer.c (01c0 T dissect_oer_boolean) is
> not referred to so could be static? (in header)
> epan/dissectors/packet-oer.c (10b0 T dissect_oer_IA5String) is
> not referred to so could be static? (in header)
> epan/dissectors/packet-opa-mad.c ( D
> pref_attempt_rmpp_defragment) is not referred to so could be static?
> epan/dissectors/packet-oran.c (18c0 D compression_options) is
> not referred to so could be static

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

[Martin Mathieson via Wireshark-dev]

Hi,

I wrote a script to check whether variables and functions in dissector
modules that were not static were:
- not called by any other dissector modules (including dissectors.c)
- mentioned in the header file corresponding to that dissector

Reasons to clean up these cases could include:
- it pollutes a large namespace (most, but not all, of the symbols include
the protocol name so likely won't clash)
- it will make it easier for people to spot and delete functions that are
not actually being called
- it sets a better example for people to copy when adding new code
- there may even be some cases where the compiler is able to optimise
better, don't know if it might speed up linking a little?

What do others think - does it sound like it could be worth it?

Where there is no extern or declaration in the header file, I think we can
safely try to use static.  If it is not used even by the dissector itself
(which the script doesn't currently check, but could...) it can presumably
be deleted (unless we know that someone is getting ready to use it soon).

Where the variable or function is mentioned in the dissector's header file,
in a lot of cases it could be removed from the header file and made
static.  I do have some cases where I call functions only from private
dissectors to set decryption keys, i.e.

epan/dissectors/packet-pdcp-lte.c (3670 T
set_pdcp_lte_rrc_ciphering_key) is not referred to so could be static? (in
header)
epan/dissectors/packet-pdcp-lte.c (3730 T
set_pdcp_lte_rrc_integrity_key) is not referred to so could be static? (in
header)
epan/dissectors/packet-pdcp-lte.c (37f0 T
set_pdcp_lte_up_ciphering_key) is not referred to so could be static? (in
header)
epan/dissectors/packet-pdcp-nr.c (33b0 T
set_pdcp_nr_rrc_ciphering_key) is not referred to so could be static? (in
header)
epan/dissectors/packet-pdcp-nr.c (3470 T
set_pdcp_nr_rrc_integrity_key) is not referred to so could be static? (in
header)
epan/dissectors/packet-pdcp-nr.c (3530 T
set_pdcp_nr_up_ciphering_key) is not referred to so could be static? (in
header)
epan/dissectors/packet-pdcp-nr.c (35f0 T
set_pdcp_nr_up_integrity_key) is not referred to so could be static? (in
header)

but I imagine there are not many more like this.  One clue that people
might be making references from a Windows plugin could be the presence of
WS_DLL_PUBLIC (script doesn't currently try to check).

I currently don't check dissectors that are generated (otherwise there'd be
around 1000 other 'issues').  The dcerpc ones seem to declare lots of
functions in header files.

Here is the output of my script against master:

epan/dissectors/packet-ncp-sss.c (1130 T dissect_sss_reply) is
not referred to so could be static? (in header)
epan/dissectors/packet-ncp-sss.c (0d20 T dissect_sss_request)
is not referred to so could be static? (in header)
epan/dissectors/packet-ncp.c (0040 D ett_ncp) is not referred
to so could be static?
epan/dissectors/packet-ncp.c (0034 D ett_nds_segment) is not
referred to so could be static?
epan/dissectors/packet-ncp.c (0038 D ett_nds_segments) is not
referred to so could be static?
epan/dissectors/packet-ncp.c ( D ncp_nds_verb_vals) is not
referred to so could be static?
epan/dissectors/packet-ncp.c (00e4 D proto_ncp) is not referred
to so could be static?
epan/dissectors/packet-netmon.c (3790 T netmon_sid_field) is
not referred to so could be static? (in header)
epan/dissectors/packet-netrom.c (0080 D op_code_vals_abbrev) is
not referred to so could be static?
epan/dissectors/packet-netrom.c ( D op_code_vals_text) is
not referred to so could be static?
epan/dissectors/packet-nstrace.c ( T add35records) is not
referred to so could be static?
epan/dissectors/packet-nwp.c ( D nwp_type_vals) is not
referred to so could be static?
epan/dissectors/packet-ocfs2.c ( D ext_dlm_magic) is not
referred to so could be static?
epan/dissectors/packet-oer.c (01c0 T dissect_oer_boolean) is
not referred to so could be static? (in header)
epan/dissectors/packet-oer.c (10b0 T dissect_oer_IA5String) is
not referred to so could be static? (in header)
epan/dissectors/packet-opa-mad.c ( D
pref_attempt_rmpp_defragment) is not referred to so could be static?
epan/dissectors/packet-oran.c (18c0 D compression_options) is
not referred to so could be static?
epan/dissectors/packet-packetbb.c (0200 D addrtlv_type_vals) is
not referred to so could be static?
epan/dissectors/packet-packetbb.c (0180 D linkstatus_vals) is
not referred to so could be static?
epan/dissectors/packet-packetbb.c (01c0 D localif_vals) is not
referred to so could be static?
epan/dissectors/packet-packetbb.c (0100 D mpr_vals) is not
referred