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

vlan: add vlan.id keyword - v3 #12273

Closed
wants to merge 1 commit into from

Conversation

AkakiAlice
Copy link
Contributor

Ticket: #1065

Make sure these boxes are checked accordingly before submitting your Pull Request -- thank you.

Contribution style:

Our Contribution agreements:

Changes (if applicable):

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

Describe changes:

  • Introduce vlan.id keyword

Provide values to any of the below to override the defaults.

  • To use an LibHTP, Suricata-Verify or Suricata-Update pull request,
    link to the pull request in the respective _BRANCH variable.
  • Leave unused overrides blank or remove.

SV_BRANCH= OISF/suricata-verify#2183
Previous PR= #12103

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 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;
Copy link
Contributor

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 ?

Copy link
Member

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;

Copy link
Contributor

@jufajardini jufajardini left a 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.

doc/userguide/rules/vlan-id.rst Show resolved Hide resolved
doc/userguide/rules/vlan-id.rst Show resolved Hide resolved
doc/userguide/rules/vlan-id.rst Show resolved Hide resolved
doc/userguide/rules/vlan-id.rst Show resolved Hide resolved
doc/userguide/rules/vlan-id.rst Show resolved Hide resolved
doc/userguide/rules/vlan-id.rst Show resolved Hide resolved
src/detect-vlan-id.c Show resolved Hide resolved
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.

5 participants