Conversation
AkakiAlice
requested review from
jasonish,
jufajardini and
victorjulien
as code owners
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #12290 +/- ##
==========================================
+ Coverage 83.22% 83.25% +0.02%
==========================================
Files 912 914 +2
Lines 257311 257811 +500
==========================================
+ Hits 214154 214646 +492
- Misses 43157 43165 +8
Flags with carried forward coverage won't be shown. Click here to find out more. |
jufajardini
left a comment
jufajardini
left a comment
There was a problem hiding this comment.
It looks to me that we're almost done, kudos! :)
Shivani's comment made me think of a possible improvement for the doc, I left a comment about that, and two more nit ones.
About the commit message: since we're adding a new keyword with it, it's a good practice (although not always followed) to summarize what are the keyword capabilities and what it is for, on the commit message itself. A good example could be: 3aa313d
catenacyber
left a comment
catenacyber
left a comment
There was a problem hiding this comment.
CI : red, could you rebase because latest master has clippy fixes ?
Code : looking into it now
Commits segmentation : ok
Commit messages : nit Ticket: #1065 instead of Ticket: OISF#1065
Git ID set : looks fine for me
CLA : you already contributed
Doc update : looks good, will review with the code
Redmine ticket : ok
Rustfmt : looks ok for vlan_id.rs
Tests : nice, thanks, added a remark there
Dependencies added: none
| return None; | ||
| } | ||
| let layer = if parts.len() == 2 { | ||
| if parts[1] == "all" { |
There was a problem hiding this comment.
If you wish, you may also add the syntax sugar any
And you may add also the feature count to match on p->vlan_idx
So you can match on packets without vlan like vlan.id:0, count
There was a problem hiding this comment.
what do you mean by syntax sugar any ?
There was a problem hiding this comment.
Some code like
if parts[1] == "all" {
// do something
} else if parts[1] == "any" {
// do something else
}...
This is syntax sugar because it does not bring expressivity, just some more explicit readability for rules
catenacyber
left a comment
catenacyber
left a comment
There was a problem hiding this comment.
Great work, needs some polishing, but all good
|
Replaced w #12301 |
4 participants
Ticket: #1065
Make sure these boxes are checked accordingly before submitting your Pull Request -- thank you.
Contribution style:
https://docs.suricata.io/en/latest/devguide/contributing/contribution-process.html
Our Contribution agreements:
https://suricata.io/about/contribution-agreement/ (note: this is only required once)
Changes (if applicable):
(including schema descriptions)
https://redmine.openinfosecfoundation.org/projects/suricata/issues
Link to ticket: https://redmine.openinfosecfoundation.org/issues/1065
Describe changes:
SV_BRANCH=OISF/suricata-verify#2188
Previous PR= #12285