Applayer plugin 5053 v1 #11321
Applayer plugin 5053 v1 #11321
Conversation
So that we can have dynamically registered protocols. Doing it at compile time, with CFLAGS=-DALPROTO_DYNAMIC_NB=1, allows to keep the rest of the code using ALPROTO_MAX Ticket: 5053
Ticket: 5053
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #11321 +/- ##
==========================================
- Coverage 82.47% 77.67% -4.80%
==========================================
Files 934 934
Lines 252270 252235 -35
==========================================
- Hits 208055 195920 -12135
- Misses 44215 56315 +12100
Flags with carried forward coverage won't be shown. Click here to find out more. |
|
Do you see new "built-in" app-layers going through this API? Or still using a static assignment? |
still static... |
|
ERROR: ERROR: QA failed on build_asan. Pipeline 21127 |
|
I would like to see the registration of AppProto's to be cleaner. Like in other places, we can have something like built-in app protos are static, followed by a dynamic range. _MAX would be replaced by a fully dynamic |
|
AppProto as an enum also isn't a great fit anymore, it's more just a registered id for these dynamic cases |
| @@ -1776,6 +1780,15 @@ void AppLayerParserRegisterProtocolParsers(void) | |||
| } else { | |||
| SCLogInfo("Protocol detection and parser disabled for pop3 protocol."); | |||
| } | |||
| #ifdef ALPROTO_DYNAMIC_NB | |||
There was a problem hiding this comment.
This would not be something that should be compile time optional?
Any particular reason? It would be nice to use these APIs for built in and dynamically loaded unless there is good reason not to. |
There a re 33 arrays to handle, not 2 like for keywords (cf
Ok, it can be done in follow up works like keywords in #11316
It is no longer the case here : `typedef uint16_t AppProto; |
|
Yeah I realize it's major effort, but I think we shouldn't hack around it, esp not in this phase of the 8 dev cycle. |
|
Some work done in #11355 |
|
So, closing this draft. #11572 is the first PR towards it |
Link to ticket: https://redmine.openinfosecfoundation.org/issues/
https://redmine.openinfosecfoundation.org/issues/5053
Describe changes:
Comes after merge of #11291
Draft to get general feedback
especially the use of
ALPROTO_DYNAMIC_NBStill todo: supply an example of an app-layer plugin