Bindgen more 7762 v8.4.3 #14667
Bindgen more 7762 v8.4.3 #14667
Conversation
As will be needed such as AppLayerTxData
and bindgen it to rust. Will make easier the bindgen of RustParser structure which uses a callback which uses AppLayerGetFileState
and bindgen it to rust, and use default trait instead of new Will make easier the bindgen of RustParser structure which uses a callback which uses AppLayerStateData
and bindgen it to rust Will make easier the bindgen of RustParser structure which uses a callback which uses AppLayerGetTxIterTuple
and bindgen it to rust Will make easier the bindgen of RustParser structure which uses a callback which uses StreamSlice
and bindgen it to rust Will make easier the bindgen of RustParser structure which uses a callback which uses AppLayerResult Keep From<> impl in sys crate that defines it
catenacyber
requested review from
a team,
jasonish,
jufajardini and
victorjulien
as code owners
and bindgen it to rust Will make easier the bindgen of RustParser structure which uses a callback which uses AppLayerTxData Move also the free function to C SCAppLayerTxDataCleanup As suricata-sys crate defines AppLayerTxData for rust, It must itself implement the Drop trait, and thus, We need to define a feature surest
and reorder definitions
Ticket: 7662
catenacyber
force-pushed
the
bindgen-more-7762-v8.4.3
branch
from
550973d to
f502856
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #14667 +/- ##
==========================================
- Coverage 82.11% 82.11% -0.01%
==========================================
Files 1011 1011
Lines 262812 262843 +31
==========================================
+ Hits 215812 215834 +22
- Misses 47000 47009 +9
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
WARNING:
Pipeline = 29228 |
| let buf = stream_slice.as_slice(); | ||
| let hdr = &mut state.transaction.srv_hdr; | ||
| state.transaction.tx_data.updated_tc = true; | ||
| state.transaction.tx_data.0.updated_tc = true; |
There was a problem hiding this comment.
I don't like this .0. pattern that this PR introduces, why was it added? I don't see an explanation for it.
There was a problem hiding this comment.
This was discussed in earlier versions of the PR : #14660 (comment)
Trying to sum up :
We move AppLayerTxData definition to C and bindgen it (because we bindgen function SCAppLayerRegisterParser whose arguments have AppLayerTxData)
We need to free memory of some fields of AppLayerTxData which are pointers so we implement Drop trait on it, this way, a WhateverTransaction having a AppLayerTxData field will do the right thing (ie free the memory).
But we cannot implement Drop on a type outside the current crate : suricata crate cannot implement Drop for AppLayerTxData in the suricata_sys crate
My first solution was to add the impl Drop for AppLayerTxData in rust/sys/src/lib.rs in #14660 but Jason did not like it and proposed to use a wrapper type in suricata crate.
So we have
pub struct AppLayerTxData(pub suricata_sys::sys::AppLayerTxData);
and we can implement Drop on suricata's AppLayerTxData
But we need to add the .0 to access its inner fields
There was a problem hiding this comment.
It's a pretty common pattern when you need to wrap something in a struct to work around some issues in Rust, one being where you can't define a trait on a struct from another crate.
From the Rust book: https://doc.rust-lang.org/book/ch20-02-advanced-traits.html#implementing-external-traits-with-the-newtype-pattern
4 participants
Link to ticket: https://redmine.openinfosecfoundation.org/issues/
https://redmine.openinfosecfoundation.org/issues/7762
Describe changes:
#14665 with cargo test and plugin build fix
cc @jasonish