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

detect/analyzer: add more details for tcp_mss #10828

Closed
wants to merge 1 commit into from

Conversation

catenacyber
Copy link
Contributor

Link to redmine ticket:
https://redmine.openinfosecfoundation.org/issues/6355

Describe changes:

  • detect/analyzer: add more details for tcp_mss

SV_BRANCH=OISF/suricata-verify#1759

#10826 with even better names

Add more details to the tcp.mss keyword engine analysis output
Issue: OISF#6355
Copy link

codecov bot commented Apr 12, 2024

Codecov Report

Attention: Patch coverage is 90.00000% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 82.67%. Comparing base (172b55c) to head (37b72f9).

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #10828      +/-   ##
==========================================
- Coverage   82.75%   82.67%   -0.08%     
==========================================
  Files         928      929       +1     
  Lines      247913   247953      +40     
==========================================
- Hits       205162   205004     -158     
- Misses      42751    42949     +198     
Flag Coverage Δ
fuzzcorpus 63.98% <0.00%> (-0.25%) ⬇️
suricata-verify 62.04% <90.00%> (+0.03%) ⬆️
unittests 62.17% <0.00%> (-0.02%) ⬇️

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

@victorjulien
Copy link
Member

Final request: think this should be 2 commits: 1 for the tojson feature, 1 for the tcp mss support.

use crate::detect::uint::{DetectIntType, DetectUintData, DetectUintMode};
use crate::jsonbuilder::{JsonBuilder, JsonError};

pub fn detect_uint_to_json<T: DetectIntType>(
Copy link
Member

Choose a reason for hiding this comment

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

Does this require a whole new module? Why not just a tojson method on DetectUintMode?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So that uint does not depend on json builder

}

#[no_mangle]
pub unsafe extern "C" fn rs_detect_u16_to_json(
Copy link
Member

Choose a reason for hiding this comment

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

New extern functions should following C naming and namspacing.. So SC....

Copy link
Contributor

Choose a reason for hiding this comment

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

Curious: Does this apply to any such functions, that is, functions used for adding new keywords, or other functions that could be needed for particular parsers, should those also follow this?

Copy link
Member

Choose a reason for hiding this comment

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

Curious: Does this apply to any such functions, that is, functions used for adding new keywords, or other functions that could be needed for particular parsers, should those also follow this?

Essentially any pub extern "C" becomes part of our C API, so should follow that format. Pure Rust function, and even non-pub extern fn's that are used as function pointers can follow Rust convention are they are subject to Rust's visibility rules and namespacing. pub extern will enter the global namespace, and its best we stick to a convention here whether C or Rust.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could some tool check this ? Like cargo fmt with some config ?

@suricata-qa
Copy link

Information: QA ran without warnings.

Pipeline 20028

@catenacyber
Copy link
Contributor Author

Continued in #10841

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