-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
applayer/htp: convert to new FAIL/PASS API-v6 #12017
applayer/htp: convert to new FAIL/PASS API-v6 #12017
Conversation
NOTE: This PR may contain new authors. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #12017 +/- ##
==========================================
+ Coverage 83.22% 83.38% +0.16%
==========================================
Files 910 910
Lines 258136 257620 -516
==========================================
- Hits 214831 214826 -5
+ Misses 43305 42794 -511
Flags with carried forward coverage won't be shown. Click here to find out more. |
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.
We're almost there, good job with such a big first contrib!
I think that if you address the three inline comments that I've left that require changes, the next version should be ready for merging :)
FAIL_IF_NULL(tx_ud); | ||
FAIL_IF_NULL(tx_ud->request_uri_normalized); | ||
PrintRawDataFp(stdout, bstr_ptr(tx_ud->request_uri_normalized), | ||
bstr_len(tx_ud->request_uri_normalized)); |
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.
Although this changes the original test a bit, considering that all unittests are still passing, my assumption is that this is a valid representation, still.
Leaving this note here to set the expectations, in case someone else sees and asks for this to be changed (but I don't think anyone will, since tests are passing)
Let me address them just now, cant wait to see this contribution merged! its been a long road |
work continued in #12023 |
Ticket: #6935
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)
Link to ticket: https://redmine.openinfosecfoundation.org/issues/6935
Describe changes:
-convert applayer/htp: convert to new FAIL/PASS API
-previous pr #11973