[draft] bindgen poc - v1 #12018
[draft] bindgen poc - v1 #12018
Conversation
This reverts commit 45384ef.
| */ | ||
|
|
||
| #![allow(non_camel_case_types)] | ||
|
|
There was a problem hiding this comment.
sys is used, as naming crates sometime-sys is idiomatic for crates that pull in a C library into Rust.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #12018 +/- ##
==========================================
+ Coverage 83.24% 83.28% +0.03%
==========================================
Files 910 910
Lines 258136 258135 -1
==========================================
+ Hits 214895 214988 +93
+ Misses 43241 43147 -94
Flags with carried forward coverage won't be shown. Click here to find out more. |
|
ERROR: ERROR: QA failed on build_opt. Pipeline 23168 |
|
Information: QA ran without warnings. Pipeline 23171 |
|
| @@ -0,0 +1,12 @@ | |||
| fn main() { | |||
There was a problem hiding this comment.
can you add some explanation here and also to the commit about why we need build.rs and what it does?
There was a problem hiding this comment.
Broke the commit that introduced bindgen, and made use of the bindings into 2. Added some more comments to the code and the commit that introduces bindgen.
Not really better names, they imply ffi where these are just normal headers as far as the C code is concerned. The main point being is I just needed really cleaned up headers to avoid a fight with bindgen to test out bindgen. Should we chose to go to bindgen, we'll probably need some good header cleanup/review for the parts we expose - the big part being not exposing any headers that also depend on headers generated from Rust I think - circular dependencies, thats where things seem to choke. |
Extract a simple function function to a simple header with no circular dependencies. Next we'll use bindgen to generate Rust bindings for this function. Ticket: OISF#7341
Add a build.rs to generate Rust bindings to specific C functions at build time using the bindgen crate. As can be seen in build.rs, we currently only pull in the test header, "app-layer-ext.h" and only output items starting with "SC". Bindgen generates the bindings in a file named "bindings.rs" in the target/ direction, which "sys.rs" will statically "include" at compiling name, making these bindings available under package "crate::sys". "build.rs" had to be placed in the non-standard location of "src/build.rs" (its usually alongside Cargo.toml) to satisfy the out-of-tree build requirements of distcheck. Note that bindgen is also available as a command line tool which could be used instead of integrating this at compile time, however the tool requires a newer version of Rust than our MSRV, and may present additional issues with respect to autoconf and distcheck.
This function is now named SCAppLayerStateGetProgressFn, note how its
definition differs from ours:
pub type SCAppLayerStateGetProgressFn = ::std::option::Option<
unsafe extern "C" fn(
alstate: *mut ::std::os::raw::c_void,
direction: u8,
) -> ::std::os::raw::c_int,
>;
Our previous definition:
pub type StateGetProgressFn = unsafe extern "C" fn (*mut c_void, u8) -> c_int;
The main differing being wrapped in an option which makes sense as its
a function pointer that could be NULL/None, but does require wrapping
it in an Option now.
Rust will now fail, as the discrepancy between i8 and i32 is picked up.
|
To see the generated bindings you can do something like: Build from clean, so only one copy exists: Find the bindings: /* automatically generated by rust-bindgen 0.69.4 */
#[repr(u32)]
#[derive(Debug, Copy, Clone, Hash, PartialEq, Eq)]
pub enum AppLayerEventType {
APP_LAYER_EVENT_TYPE_TRANSACTION = 1,
APP_LAYER_EVENT_TYPE_PACKET = 2,
}
pub type SCAppLayerStateGetProgressFn = ::std::option::Option<
unsafe extern "C" fn(
alstate: *mut ::std::os::raw::c_void,
direction: u8,
) -> ::std::os::raw::c_int,
>;
pub type SCAppLayerStateGetEventInfoFn = ::std::option::Option<
unsafe extern "C" fn(
event_name: *const ::std::os::raw::c_char,
event_id: *mut ::std::os::raw::c_int,
event_type: *mut AppLayerEventType,
) -> ::std::os::raw::c_int,
>;
pub type SCAppLayerStateGetEventInfoByIdFn = ::std::option::Option<
unsafe extern "C" fn(
event_id: ::std::os::raw::c_int,
event_name: *mut *const ::std::os::raw::c_char,
event_type: *mut AppLayerEventType,
) -> ::std::os::raw::c_int,
>; |
|
ERROR: ERROR: QA failed on build_fetch. Pipeline 23178 |
See if using bindgen to generate Rust bindings for C functions could have prevented the issues fixed in this PR: #12009
Ticket: https://redmine.openinfosecfoundation.org/issues/7341
And yes, it would.
SCAppLayerStateGetEventInfoByIdFn, and some others inapp-layer-ext.h. Note that I had to bring back the definition of theAppLayerEventTypeenum to C from Rust to avoid circular dependency.app-layer-ext.his a bad name, but I just needed a "clean" header.i8andi32discrepancies.Caveats:
libclang. Many of the CI jobs will fail because clang is not installedbindgenlibrary requires a lower MSRV than the CLI tool, which requires a newer MSRV that we use right now, and adds another tool dependency. However, we could revert to a style like we do with cbindgen. Thenbindgen-cliandlibclangwould not be needed on releases from dist, but dist builders would need newer rust (not bindgen is often found in system packages, so may be a non-issue).Other notes:
i8and C is storing it in anint32_tfor instance.