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

smb: add smb.version keyword 5075 v4 #9469

Closed
wants to merge 4 commits into from

Conversation

jmtaylor90
Copy link
Contributor

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

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

Describe changes:

  • fixed up broken commit so builds successfully at each commit
  • used initial authors commits
  • removed unnecessary NULL checks
  • removed unnecessary debug related function calls

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

To use a pull request use a branch name like pr/N where N is the
pull request number.

Alternatively, SV_BRANCH may also be a link to an
OISF/suricata-verify pull-request.

SV_REPO=
SV_BRANCH=pr/1380
SU_REPO=
SU_BRANCH=
LIBHTP_REPO=
LIBHTP_BRANCH=

@codecov
Copy link

codecov bot commented Sep 8, 2023

Codecov Report

Merging #9469 (d32e5cb) into master (2b57179) will decrease coverage by 0.02%.
The diff coverage is 90.21%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #9469      +/-   ##
==========================================
- Coverage   82.18%   82.17%   -0.02%     
==========================================
  Files         968      969       +1     
  Lines      274199   274283      +84     
==========================================
+ Hits       225363   225381      +18     
- Misses      48836    48902      +66     
Flag Coverage Δ
fuzzcorpus 64.07% <26.82%> (-0.02%) ⬇️
suricata-verify 60.88% <87.80%> (-0.04%) ⬇️
unittests 62.88% <36.95%> (-0.01%) ⬇️

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

📢 Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=OISF).

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.

Documentation change looks good, commit separation and authoring, too.

Left a common wrt the copyright years for the files to be added, not sure about how to proceed there.

@@ -0,0 +1,159 @@
/* Copyright (C) 2022-2023 Open Information Security Foundation
Copy link
Contributor

Choose a reason for hiding this comment

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

Very sorry for not noticing this before, but technically speaking, since we're only adding this now, this should be 2023 only, shouldn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With the original commits/work from Eloy being from last year, I just added this year. Of course willing to update with whatever you all see fit.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see your point. I think we use the year of when the code is merged, but not sure in such cases, so can't decide...

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

  • CI : ✅
  • Code : good enough for me
  • Commits segmentation : ok (I would have squashed the 2 commits from Eloy)
  • Commit messages : Fair enough for me
  • Git ID set : looks fine for me
  • CLA : I do not have access, but looks like some commits were already merged
  • Doc update : really nice 👏
  • Redmine ticket : ok
  • Rustfmt : my rustfmt wants a new line at the end of rust/src/smb/detect.rs
  • Tests : Suricata-verify tests look fine
  • Dependencies added: none

@catenacyber catenacyber added this to the 8.0 milestone Nov 8, 2023
@jmtaylor90
Copy link
Contributor Author

jmtaylor90 commented Nov 21, 2023

Seeing as how #9852 was merged. I was wondering if you all would like me to submit an updated PR with the appropriate changes and of course rebased?

@jufajardini
Copy link
Contributor

Seeing as how #9852 was merged. I was wondering if you all would like me to submit an updated PR with the appropriate changes and of course rebased?

Thanks for catching that up, if you could, that'd be great! :) :)

@catenacyber catenacyber added the needs rebase Needs rebase to master label Nov 23, 2023
@catenacyber
Copy link
Contributor

Yes @jmtaylor90 please rebase this :-)

@jmtaylor90
Copy link
Contributor Author

continued in #9905

@jmtaylor90 jmtaylor90 closed this Nov 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs rebase Needs rebase to master
Development

Successfully merging this pull request may close these issues.

4 participants