-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
vlan: add vlan.id keyword - v3 #12273
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the work Alice
CI : red, could you investigate ?
Code : looking into it now
Commits segmentation : ok
Commit messages : title could be detect: add vlan.id keyword
Git ID set : looks fine for me
CLA : you already contributed
Doc update : good, some remark inline
Redmine ticket : ok
Rustfmt : looks ok for vlan_id.rs
Tests : nice, thanks, added a remark there
Dependencies added: none
if du16.is_err() { | ||
return None; | ||
} | ||
let du16 = du16.unwrap().1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jasonish do you have proposals for better rust style here ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I probably would have done something like:
let du16 = if let Ok(du16) = detect_parse_uint::<u16>(parts[0]) {
du16
} else {
return None;
};
Even tho it's not really shorter, but does remove the unwraps.
But just today I learned that you can use ?
on Option
return types which lets us replace:
let du16 = detect_parse_uint(parts[0]);
if du16.is_err() {
return None;
}
let du16 = du16.unwrap().1;
with
let du16 = detect_parse_uint(parts[0]).ok()?.1;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shared some comments to help with the docs, mostly. ^^
We're missing adding the new file to doc/userguide/rules/index.rst
, too, otherwise it won't be added, and the docs won't be generated, throwing an error.
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:
Provide values to any of the below to override the defaults.
link to the pull request in the respective
_BRANCH
variable.SV_BRANCH= OISF/suricata-verify#2183
Previous PR= #12103