eve-filetypes: separate from plugins, as eve filetypes are not plugins: plugins can register eve filetypes - v1 #10591
Conversation
Remove EVE filetypes from plugin context as they are not only used from plugins. Plugins allow user code to register filetypes, but we also have internal file types that use this api including the null output and syslog. Additionally library users can use this API to register filetypes, and they are not plugins. Ideally this code would go in "output-json.[ch]" as the "primary" eve API, however there are currently some include circular include issues there, so start new cleaned up EVE API in "output-eve.[ch]" which is "clean" with respect to includes, and as we cleanup existing EVE API for "public" use, it can be moved here. Ticket: OISF#6838
Remove "conf.h" from suricata-plugin.h as its not needed by that header. However, some other files became transitively dependent on through other includes, so fix those up.
EVE filetypes are not always plugins, for example, null and syslog that are built-in filetypes.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #10591 +/- ##
==========================================
- Coverage 82.74% 82.70% -0.05%
==========================================
Files 924 925 +1
Lines 247401 247401
==========================================
- Hits 204719 204619 -100
- Misses 42682 42782 +100
Flags with carried forward coverage won't be shown. Click here to find out more. |
|
Information: QA ran without warnings. Pipeline 19064 |
| * include issues are figured out. | ||
| */ | ||
|
|
||
| #ifndef __SURICATA_OUTPUT_EVE_H__ |
There was a problem hiding this comment.
why SURICATA part of the define here? We normally only base on the file name
There was a problem hiding this comment.
As an include that is part of the library, it should be name spaced as well. __OUTPUT_EVE_H__ is probably not that generic, but something simpler like __OUTPUT_H__ is more generic with a higher chance of conflicting with a user header.
LibHTP, libcurl, etc. all do this.
There was a problem hiding this comment.
should this type of change go into the whole codebase? Perhaps as a separate change/commit?
There was a problem hiding this comment.
I'm somewhat indifferent here as its not an introducer of merge conflicts like some other changes we have discussed would be. But would be nice I guess, should we also remove the leading double underscores (and trailing) as they are in reserved namespace?
| /** | ||
| * Structure used to define an Eve output file type plugin. | ||
| */ | ||
| typedef struct SCEveFileType_ { |
There was a problem hiding this comment.
would be nice to tighten the function signatures a bit, esp with adding const where appropriate
There was a problem hiding this comment.
DIdn't want to change or break the API with this PR, I have some breaking changes coming for 8.0 that hopefully make it easier, that would be where to change the signatures.
| @@ -20,6 +20,7 @@ | |||
|
|
|||
| #include <stdint.h> | |||
| #include <stdbool.h> | |||
| #include <queue.h> | |||
There was a problem hiding this comment.
is this meant to "queue.h"?
There was a problem hiding this comment.
Can we combine the SCPlugin and SCCapturePlugin types?
I think that SCPlugin would suffice for both (maybe all?) plugin types and then a backing type for the capture plugins can be created in a similar way that SCEveFileType is a backing type for SCPlugin.
Thoughts?
There was a problem hiding this comment.
The only catch here is that the filetypes don't require anything from the engine, but the captures do, at least now -- the "slot" they fit into. So capture plugins need that extra hook from the engine that file types do not.
This PR tried to be simple with no behaviour changes. However, it would make sense to integrate that with what I want to do next, simplify the |
|
Taking this all the way with breaking cleanups: #10621 |
EVE filetypes and plugins were conflated in naming and discussion when they are really different things which is clear by examples in code such as the nullsink and syslog eve filetypes, which are very much not plugins.
To make this more clear, rename and refactor such that these registered EVE filetypes are referred to as filetypes, not plugins.
Plugins can register these filetypes, but so can internal code as well as library users.
Includes some header cleanup due to removing "suricata-plugin.h" from places that shouldn't need any knowledge of plugins, but somehow become the way some files were getting the "conf.h" header.
Pushing for review, but as I'm reviewing myself right now, I want to fill out more of the in-code documentation.
Ticket: https://redmine.openinfosecfoundation.org/issues/6838