suricata-ffi: nicer rust bindings to suricata c interfaces#14753
suricata-ffi: nicer rust bindings to suricata c interfaces#14753
Conversation
jasonish
requested review from
a team,
jufajardini and
victorjulien
as code owners
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #14753 +/- ##
==========================================
- Coverage 82.15% 82.14% -0.01%
==========================================
Files 1003 1003
Lines 263674 263672 -2
==========================================
- Hits 216611 216606 -5
- Misses 47063 47066 +3
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
jasonish
force-pushed
the
suricata-ffi/v1
branch
from
5045db9 to
900edb2
Compare
|
Information: QA ran without warnings. Pipeline = 29445 |
|
Information: QA ran without warnings. Pipeline = 29446 |
jasonish
force-pushed
the
suricata-ffi/v1
branch
2 times, most recently
from
e1a4017 to
da5bb76
Compare
|
Information: QA ran without warnings. Pipeline = 29447 |
| #include "util-logopenfile.h" | ||
| #include "output.h" | ||
| #include "output-eve-bindgen.h" | ||
| #include "output-eve.h" |
There was a problem hiding this comment.
I do not agree with your changes because I do not like super-includes (see cppclean usage a long ago) but this is a detail
There was a problem hiding this comment.
Perhaps a better name then output-eve-bindgen? That leaks bindgen details into Suricata source files that should have no knowledge of it. I guess its a sign there needs to be more modular includes in the "output-eve-*" space. I don't think bindgen is a good module break down.
Its still not much of a super include is it? Ironic that it exists for the sole purpose of building a super include (bindgen.h).
There was a problem hiding this comment.
Actually my next action item is to expose whats left in output-eve.h to bindgen, which means output-eve.h will be empty, which means output-eve-bindgen.h could be renamed to output-eve.h and be bindgen clean (with some ifndef SURICATA_BINDGEN_H).
There was a problem hiding this comment.
Its still not much of a super include is it? Ironic that it exists for the sole purpose of building a super include
Yeah, maybe you are right
Does cppclean still work?
It had been disabled in CI by Victor because its true-positive reports prevented him to merge his work
| message: &str, | ||
| ) -> SCError { | ||
| unsafe { | ||
| SCLogMessage( |
There was a problem hiding this comment.
Should this code not be removed from Suricata crate ? and just used from this one (can be a pub use to minimize code changes)
There was a problem hiding this comment.
It can use some of this, but not all. As test cases won't have the library linkage required. Which is why I didn't do the conversion in the suricata crate yet, as there is some hassle involved.
|
Looks cool globally, waiting for #14667 merge first to complete https://redmine.openinfosecfoundation.org/issues/7762 before tackling https://redmine.openinfosecfoundation.org/issues/7666 |
|
Information: QA ran without warnings. Pipeline = 29455 |
jasonish
force-pushed
the
suricata-ffi/v1
branch
2 times, most recently
from
ca962b6 to
150ca26
Compare
| // | ||
| // Perhaps look at the typestate builder pattern to enforce at | ||
| // compile time. | ||
| assert!(init.is_some(), "init must not be None"); |
There was a problem hiding this comment.
|
Information: QA ran without warnings. Pipeline = 29460 |
As output-eve-bindgen.h exists to support bindgen, its odd to see other Suricata C files using it. Instead Suricata C code should import "output-eve.h", which itself includes "output-eve-bindgen.h", only broken out to support the external tool bindgen.
There is an unfortunate side-affect that one has to read output-eve-bindgen.h for the documentation on this type, however, I think we can resolve that in time.
Used by Rust output plugins.
This crate is for Rust wrappers around the -sys crate which includes only raw bindings. This is the place to add nice wrappers around those bindings, however it should remain clear of dependencies on the main Suricata core crates. Ticket: OISF#7666
Mostly a copy of Suricata core's logging wrappers into the ffi crate. These are not yet used by Suricata-core as they do require the Suricata library to be avaiable, which is not the case with tests. And the `cfg(test)` parameter is not passed through to sub-crates. However, this does allow a plugin (or library) to call the logging macros without depending on the "suricata" crate. Ticket: OISF#7666
A plugin can now create a "Plugin" struct with Rust strings. The `into_raw` method converts it to a run pointer suitable for returning during plugin registration.
jasonish
force-pushed
the
suricata-ffi/v1
branch
from
150ca26 to
9d18c0d
Compare
|
Information: QA ran without warnings. Pipeline = 29485 |
3 participants
A first take on a zero dependency crate for plugins, etc.
This has allowed me to remove my own ffi glue from my Redis output plugin, as well as remove the main "suricata" crate as a dependency.
Pushing for early comment to ease some near-future plugin exploration.
Related ticket: https://redmine.openinfosecfoundation.org/issues/7666