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 - v10 #12374

Closed
wants to merge 2 commits into from

Conversation

AkakiAlice
Copy link
Contributor

@AkakiAlice AkakiAlice commented Jan 11, 2025

Ticket: #1065

Contribution style:

Our Contribution agreements:

Changes (if applicable):

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

Description:

  • introduce vlan.id and vlan.layers keywords

Changes:

  • change VLAN.id range in the commit description to 0-4095
  • mod.rs:
    • undo rustfmt on mod.rs
  • vlan.rs:
    • correct integer type of VLAN_MAX_LAYERS: line 24
    • add comments: line 28
    • replace >= with > so vlan.id can match on values up to 4095: line 45
    • use -VLAN_MAX_LAYERS and VLAN_MAX_LAYERS - 1: line 56
  • vlan-keywords.rst:
    • valid range for id values = 0 - 4095: line 29
    • coreect vlan.layers typos
  • detect-vlan.c:

SV_BRANCH=OISF/suricata-verify#2222
Previous PR: #12360

vlan.id matches on Virtual Local Area Network IDs
It is an unsigned 16-bit integer
Valid range = [0-4095]
Supports prefiltering

Ticket: OISF#1065
vlan.layers matches on the number of VLAN layers per packet
It is an unsigned 8-bit integer
Valid range = [0-3]
Supports prefiltering

Ticket: OISF#1065
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.

Thanks Alice, nice work

CI : rerunning to get green CI with rebased SV 🤞
Code : cool
Commits segmentation : ok
Commit messages : nice
Git ID set : looks fine for me
CLA : you already contributed
Doc update : thanks for fixing the typos
Redmine ticket : ok
Rustfmt : ok
Tests : ok cool
Dependencies added: none

Copy link

codecov bot commented Jan 12, 2025

Codecov Report

Attention: Patch coverage is 88.66397% with 28 lines in your changes missing coverage. Please review.

Project coverage is 82.48%. Comparing base (bd1b9f6) to head (0ce4621).
Report is 9 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #12374      +/-   ##
==========================================
- Coverage   82.49%   82.48%   -0.01%     
==========================================
  Files         912      914       +2     
  Lines      258083   258467     +384     
==========================================
+ Hits       212897   213199     +302     
- Misses      45186    45268      +82     
Flag Coverage Δ
fuzzcorpus 60.37% <13.42%> (-0.12%) ⬇️
livemode 19.55% <13.42%> (+0.14%) ⬆️
pcap 44.38% <13.42%> (+<0.01%) ⬆️
suricata-verify 63.25% <78.37%> (+0.04%) ⬆️
unittests 58.07% <55.46%> (-0.04%) ⬇️

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

Copy link
Member

@victorjulien victorjulien left a comment

Choose a reason for hiding this comment

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

minor rust style feedback

Think we're almost there, nice work :)


static void DetectVlanIdFree(DetectEngineCtx *de_ctx, void *ptr)
{
rs_detect_vlan_id_free(ptr);
Copy link
Member

Choose a reason for hiding this comment

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

style: new code should use SCDetectVlanIdFree for public rust functions

Copy link
Member

Choose a reason for hiding this comment

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

check the whole PR, essentially there should be no new rs_ functions

Copy link
Contributor

Choose a reason for hiding this comment

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

@AkakiAlice AkakiAlice mentioned this pull request Jan 13, 2025
5 tasks
@AkakiAlice AkakiAlice closed this Jan 13, 2025
@AkakiAlice
Copy link
Contributor Author

Replaced by #12386

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