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

Flow bytes pkts syntax/v10 #12362

Closed
wants to merge 3 commits into from

Conversation

inashivb
Copy link
Member

@inashivb inashivb commented Jan 9, 2025

Previous PR: #12353

Redmine ticket: https://redmine.openinfosecfoundation.org/issues/5646

SV_BRANCH=OISF/suricata-verify#2217

Changes since v9:

  • cleaned up error handling as per review
  • rebased on top of latest master

Currently, the syntax includes direction as a part of the keyword which
is against how usually keywords are done. By making direction as a
mandatory argument, it is possible to make the syntax cleaner and the
implementation more compact and easily extendable.
Pros:
- Registration table sees lesser entries if newer options are added
- If the options have to be extended, it can be done trivially
- In accordance w existing keyword implementations

Note that this commit also retains the existing direction specific
keywords.
For flow.bytes and flow.pkts keywords, allow matching in either
direction.

Feature 5646
Copy link

codecov bot commented Jan 9, 2025

Codecov Report

Attention: Patch coverage is 76.39485% with 55 lines in your changes missing coverage. Please review.

Project coverage is 82.50%. Comparing base (494d7bf) to head (24e14a3).

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #12362      +/-   ##
==========================================
- Coverage   82.54%   82.50%   -0.04%     
==========================================
  Files         912      912              
  Lines      258028   258165     +137     
==========================================
+ Hits       212988   213008      +20     
- Misses      45040    45157     +117     
Flag Coverage Δ
fuzzcorpus 60.53% <39.05%> (-0.19%) ⬇️
livemode 19.39% <21.45%> (-0.01%) ⬇️
pcap 44.42% <21.45%> (+<0.01%) ⬆️
suricata-verify 63.20% <75.96%> (+0.01%) ⬆️
unittests 58.08% <21.45%> (-0.03%) ⬇️

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

@victorjulien
Copy link
Member

Attention: Patch coverage is 76.39485% with 55 lines in your changes missing coverage. Please review.

55 lines w/o coverage is quite a few, can you investigate if more tests are required?

@inashivb
Copy link
Member Author

inashivb commented Jan 9, 2025

Attention: Patch coverage is 76.39485% with 55 lines in your changes missing coverage. Please review.

55 lines w/o coverage is quite a few, can you investigate if more tests are required?

The lines missing coverage are:

  • Error conditions for invalid data/memalloc failures
  • prefilter (trying to add tests for these)

@inashivb inashivb closed this Jan 9, 2025
@inashivb inashivb deleted the flow-bytes-pkts-syntax/v10 branch January 9, 2025 10:26
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.

2 participants