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: add vlan.id keyword - v7 #12324

Closed
wants to merge 1 commit into from

Conversation

AkakiAlice
Copy link
Contributor

@AkakiAlice AkakiAlice commented Dec 29, 2024

Ticket: #1065

Contribution style:

Our Contribution agreements:

Changes (if applicable):

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

Description:

  • Introduce vlan.id keyword

detect-vlan-id.c changes:

  • Check if vdata.layer is count before returning: line 97

vlan_id.rs changes:

  • Define consts VLAN_MAX_LAYERS and VLAN_MAX_LAYER_IDX: line 25, line 26
  • Check if du16.arg2 is a valid value: line 41
  • Replace magic values with constants: line 49, line 57
  • Move .ok(): line 56
  • Move check when setting DETECT_VLAN_ID_COUNT : line 49
  • Move check right after i8::from_str : line 57
  • Add any option: line 53
  • Add test for option any: line 107
  • Add test for invalid count value: line 209
  • Add test for invalid layer value: line 210
  • Add test for invalid input with more than 2 arguments: line 211

vlan-keywords.rst changes:

  • Documentation nits
  • Document option any: line 52

SV_BRANCH=OISF/suricata-verify#2208
Previous PR= #12301

vlan.id matches on Virtual Local Area Network IDs
It is an unsigned 16-bit integer
Valid range for the default configuration = [1-4094]
Supports prefiltering

Ticket: OISF#1065
@AkakiAlice AkakiAlice force-pushed the detect-vlan-id-1065-v7 branch from 08b0d38 to 3214cba Compare December 29, 2024 19:57
Copy link

codecov bot commented Dec 29, 2024

Codecov Report

Attention: Patch coverage is 94.57014% with 12 lines in your changes missing coverage. Please review.

Project coverage is 83.23%. Comparing base (6f937c7) to head (3214cba).

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #12324      +/-   ##
==========================================
- Coverage   83.26%   83.23%   -0.04%     
==========================================
  Files         912      914       +2     
  Lines      257643   257864     +221     
==========================================
+ Hits       214521   214621     +100     
- Misses      43122    43243     +121     
Flag Coverage Δ
fuzzcorpus 61.12% <8.92%> (-0.02%) ⬇️
livemode 19.39% <8.92%> (-0.01%) ⬇️
pcap 44.39% <8.92%> (-0.04%) ⬇️
suricata-verify 62.90% <83.92%> (+0.04%) ⬆️
unittests 59.19% <64.25%> (+<0.01%) ⬆️

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

Copy link
Contributor

@catenacyber catenacyber left a comment

Choose a reason for hiding this comment

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

CI : 🟢
Code : cool
Commits segmentation : ok
Commit messages : nice
Git ID set : looks fine for me
CLA : you already contributed
Doc update : excellent
Redmine ticket : ok (please put the clickable link to https://redmine.openinfosecfoundation.org/issues/1065 instead of PR 1065 ;-) )
Rustfmt : almost ok for vlan_id.rs :-p
Tests : nice
Dependencies added: none

@AkakiAlice
Copy link
Contributor Author

CI : 🟢 Code : cool Commits segmentation : ok Commit messages : nice Git ID set : looks fine for me CLA : you already contributed Doc update : excellent Redmine ticket : ok (please put the clickable link to https://redmine.openinfosecfoundation.org/issues/1065 instead of PR 1065 ;-) ) Rustfmt : almost ok for vlan_id.rs :-p Tests : nice Dependencies added: none

Should I include this change with rustfmt in the same pull request, or would it be better to create a separate PR specifically for this modification?

@catenacyber
Copy link
Contributor

New PR is preferred

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