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: remove unused #9256

Closed
wants to merge 1 commit into from
Closed

Conversation

catenacyber
Copy link
Contributor

Link to redmine ticket:
None, generic cleaning
https://redmine.openinfosecfoundation.org/issues/4083

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 nothing more to be done :

  • There are parsed fields which get never used. (like rrclass in DNSQueryEntry) : I feel good to keep them

Rebase #9020, now that Suricata7 was released

@codecov
Copy link

codecov bot commented Jul 19, 2023

Codecov Report

Merging #9256 (db5615a) into master (9a33c53) will increase coverage by 0.00%.
The diff coverage is n/a.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #9256   +/-   ##
=======================================
  Coverage   82.42%   82.43%           
=======================================
  Files         968      968           
  Lines      273952   273914   -38     
=======================================
- Hits       225807   225790   -17     
+ Misses      48145    48124   -21     
Flag Coverage Δ
fuzzcorpus 64.70% <ø> (+0.01%) ⬆️
suricata-verify 60.81% <ø> (+<0.01%) ⬆️
unittests 62.94% <ø> (+<0.01%) ⬆️

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

@suricata-qa
Copy link

WARNING:

ERROR: QA failed on SURI_TLPR1_alerts_cmp.

field baseline test %
SURI_TLPW2_autofp_stats_chk
.flow.spare 1936156 1811134 93.54%
SURI_TLPR1_stats_chk
.uptime 1267 1334 105.29%
.tcp.active_sessions 0 323 -
.flow.active 0 100919 -
.flow.get_used 0 100919 -
.flow.get_used_eval 0 11453718 -
.flow.get_used_eval_reject 0 6323 -
.flow.get_used_eval_busy 0 2374 -
.flow.get_used_failed 0 1580690 -
.flow.emerg_mode_entered 0 2 -
.flow.emerg_mode_over 0 1 -
.app_layer.error.tls.parser 23942 29751 124.26%

Pipeline 15309

@catenacyber
Copy link
Contributor Author

catenacyber commented Aug 26, 2023

@jasonish could you review this please :-) ?
It completes #8976 which was already merged

@@ -103,29 +101,6 @@ impl ConfNode {
return Self { conf }
}

pub fn get_child_value(&self, key: &str) -> Option<&str> {
Copy link
Member

Choose a reason for hiding this comment

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

I consider this part of the public api footprint.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

@@ -373,36 +373,6 @@ impl JsonBuilder {
}
}

/// Add a byte array to a JSON array encoded as hex.
pub fn append_hex(&mut self, val: &[u8]) -> Result<&mut Self, JsonError> {
Copy link
Member

Choose a reason for hiding this comment

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

This can go, I see it coming back at some point tho. It kind of falls in the public API space, but we don't have plugin support for protos so is largely not usable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok

Side note that for plugin support for photos (for which I have a POC), you cannot export rust.
So, my rust plugin uses the C API of json builder...

Copy link
Member

Choose a reason for hiding this comment

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

Side note that for plugin support for photos (for which I have a POC), you cannot export rust. So, my rust plugin uses the C API of json builder...

Yeah, thats a known unfortunately. Rust plugins have to link in with the C api. We can provide Rust wrappers to make it feel more like Rust tho.

Copy link
Member

Choose a reason for hiding this comment

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

But Rust plugins can use standalone modules provided in Suricata proper like JSONBuilder, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But Rust plugins can use standalone modules provided in Suricata proper like JSONBuilder

How so ?
my rust plugin uses the C API of jsonbuilder...

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.

Let keep the conf API item.

@catenacyber catenacyber mentioned this pull request Aug 30, 2023
@catenacyber
Copy link
Contributor Author

Replaced by #9419

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

Successfully merging this pull request may close these issues.

3 participants