-
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
Output alert applayer v26 #10680
Output alert applayer v26 #10680
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #10680 +/- ##
==========================================
+ Coverage 82.69% 82.78% +0.09%
==========================================
Files 926 914 -12
Lines 247637 247288 -349
==========================================
- Hits 204790 204724 -66
+ Misses 42847 42564 -283
Flags with carried forward coverage won't be shown. Click here to find out more. |
Information: QA ran without warnings. Pipeline 19768 |
return TM_ECODE_FAILED; | ||
} | ||
|
||
static int JsonGenericDirPacketLogger(ThreadVars *tv, void *thread_data, const Packet *p, Flow *f, |
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 like the removal of files, but I don't like how we now have all these protocol specific functions here. It feels like a step backwards. What is the purpose of it? Do have have some next step in mind where it can be registered from the parsers directly?
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.
Do you mean the ones like OutputSIPLogInitSub
?
This OutputSIPLogInitSub
currently lives in output-json-sip.c on the master branch.
Would you like to keep output-json-sip.c just to have this OutputSIPLogInitSub
?
Or would you like the calls to AppLayerParserRegisterLogger
to be moved in rust/src/sip ?
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.
Just pushed a draft commit on top to show the second possibility
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 do like the second possibility more. I'd rather see less JSON stuff in output.c
than more, as they should be at different levels of abstraction that got a little messy at some point.
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.
Ok doing in next PR
44645a5
to
06df11b
Compare
Information: QA ran without warnings. Pipeline 19817 |
Is this also preliminary work for https://redmine.openinfosecfoundation.org/issues/4683? Just was looking at programmatically registering a batch of keywords that differ in just a flag, and what would be very useful. |
I also have commits for https://redmine.openinfosecfoundation.org/issues/4683 but I indeed focus on this first |
Replaced by #10732 |
Link to redmine tickets:
https://redmine.openinfosecfoundation.org/issues/3827
Preliminary work for https://redmine.openinfosecfoundation.org/issues/5053
Describe changes:
#10631 rebased