Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rust unused v5 #8976

Closed
wants to merge 4 commits into from
Closed

Rust unused v5 #8976

wants to merge 4 commits into from

Conversation

catenacyber
Copy link
Contributor

Link to redmine ticket:
None, generic cleaning

Describe changes:

  • rust: remove unused dead code

Found with
git grep 'pub ' rust/src/ | cut -d: -f1 | uniq | xargs sed -i -e 's/pub /pub(crate) /' then see rust warnings when compiling

There is more to it :

  • https://redmine.openinfosecfoundation.org/issues/4517 APP_LAYER_PARSER_NO_INSPECTION_PAYLOAD and like should only be defined on the rust side
  • There are parsed fields which get never used. (like pad in DCERPCState) Coming in a next PR
  • dcerpc defines lots of unused constants like PFC_RESERVED_1. These should get removed as well

Follows #8880 with needed rebase

@codecov
Copy link

codecov bot commented Jun 6, 2023

Codecov Report

Merging #8976 (77c3b13) into master (6154bab) will increase coverage by 0.02%.
The diff coverage is 86.36%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8976      +/-   ##
==========================================
+ Coverage   82.42%   82.44%   +0.02%     
==========================================
  Files         969      969              
  Lines      273476   273367     -109     
==========================================
- Hits       225410   225382      -28     
+ Misses      48066    47985      -81     
Flag Coverage Δ
fuzzcorpus 64.99% <76.92%> (+0.05%) ⬆️
suricata-verify 60.53% <15.38%> (+0.02%) ⬆️
unittests 62.93% <45.45%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

@suricata-qa
Copy link

Information: QA ran without warnings.

Pipeline 14288

Copy link
Contributor

@jufajardini jufajardini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pgsql changes still looking good to me :)

Copy link
Member

@jasonish jasonish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall I'm OK with a rather aggressive purge of unused code. As we do 8, where lib stuff may be more of Suri, along with tightening down of Lua, unused functions may become more of the norm, yet should still have test cases. Just something to think about as we make this move more to a platform/framework for others.

@victorjulien victorjulien mentioned this pull request Jun 9, 2023
@victorjulien
Copy link
Member

Merged in #9001, thanks!

@catenacyber
Copy link
Contributor Author

Overall I'm OK with a rather aggressive purge of unused code. As we do 8, where lib stuff may be more of Suri, along with tightening down of Lua, unused functions may become more of the norm, yet should still have test cases. Just something to think about as we make this move more to a platform/framework for others.

@jasonish Should I also remove

  • get_child_value
  • ConfGetChildValue
  • append_hex
  • rrclass in DNSQueryEntry ? (I think it is good to keep fields in parsed structures even if we do not use them)

@catenacyber catenacyber mentioned this pull request Jun 12, 2023
@jasonish
Copy link
Member

Overall I'm OK with a rather aggressive purge of unused code. As we do 8, where lib stuff may be more of Suri, along with tightening down of Lua, unused functions may become more of the norm, yet should still have test cases. Just something to think about as we make this move more to a platform/framework for others.

@jasonish Should I also remove

  • get_child_value
  • ConfGetChildValue
  • append_hex
  • rrclass in DNSQueryEntry ? (I think it is good to keep fields in parsed structures even if we do not use them)

Keep rrclass. The rest can go.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
5 participants