Skip to content

Conversation

@catenacyber
Copy link
Contributor

Link to ticket: https://redmine.openinfosecfoundation.org/issues/
https://redmine.openinfosecfoundation.org/issues/7762

Describe changes:

  • rust: bindgen SCAppLayerRegisterParser : final round !

#14627 final round
@jasonish what do you think ?

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 catenacyber force-pushed the bindgen-more-7762-v8.4.1 branch from 1eff1dd to f2eca9e Compare January 20, 2026 22:11
}
}

#[cfg(not(feature = "suritest"))]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these helpers, and this belong in the main suricata crate now. This -sys crate should be limited to bindings only. Or this new -ffi crate we've discussed. But suricata would be a better place until then.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not allowed by rust

error[E0120]: the `Drop` trait may only be implemented for local structs, enums, and unions
   --> src/applayer.rs:100:15
    |
100 | impl Drop for AppLayerTxData {
    |               ^^^^^^^^^^^^^^ must be a struct, enum, or union in the current crate

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you see another way ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the common idiomatic Rust way is a wrapper like:

pub struct TxData(pub suricata_sys::sys::AppLayerTxData);

You see this pattern a lot.

But I see your point. I think the suritest also shows how details of the Suricata crate are leaking into this crate as well. I assume thats because we need to be linked with the C code for that to compile?

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
@catenacyber catenacyber force-pushed the bindgen-more-7762-v8.4.1 branch from f2eca9e to ef21bd1 Compare January 20, 2026 22:32
@catenacyber catenacyber marked this pull request as draft January 20, 2026 22:37
@catenacyber
Copy link
Contributor Author

Setting to draft as unclean history for rust fixups

Copy link
Member

@jasonish jasonish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just the one comment, otherwise looks OK.

@codecov
Copy link

codecov bot commented Jan 20, 2026

Codecov Report

❌ Patch coverage is 67.68293% with 53 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.10%. Comparing base (c333b28) to head (ef21bd1).

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #14660      +/-   ##
==========================================
- Coverage   82.11%   82.10%   -0.02%     
==========================================
  Files        1011     1012       +1     
  Lines      262812   262847      +35     
==========================================
- Hits       215812   215804       -8     
- Misses      47000    47043      +43     
Flag Coverage Δ
fuzzcorpus 60.25% <66.87%> (+0.07%) ⬆️
livemode 18.74% <26.38%> (+0.02%) ⬆️
pcap 44.58% <65.64%> (-0.01%) ⬇️
suricata-verify 65.29% <82.94%> (+<0.01%) ⬆️
unittests 59.27% <66.15%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@suricata-qa
Copy link

WARNING:

field baseline test %
IPS_AFP_stats_chk
.decoder.unknown_ethertype 6 0 -
.decoder.event.ethernet.unknown_ethertype 6 0 -
TREX_GENERIC_stats_chk
.decoder.unknown_ethertype 3 0 -
.decoder.event.ethernet.unknown_ethertype 3 0 -
.tcp.pkt_on_wrong_thread 0 107 -
.tcp.memuse 1824000000 912000000 50.0%
.tcp.reassembly_memuse 2403688 1185744 49.33%
.flow.tcp_reuse 0 5 -
.flow.end.tcp_state.syn_sent 0 5 -
.flow.end.tcp_state.fin_wait2 0 1 -
.flow.end.tcp_state.time_wait 0 1 -
.flow.end.tcp_state.close_wait 0 3 -
.app_layer.error.smtp.gap 0 1 -

Pipeline = 29201

@suricata-qa
Copy link

WARNING:

field baseline test %
IPS_AFP_stats_chk
.ips.drop_reason.stream_error 0 21 -
.ips.drop_reason.stream_midstream 0 2 -
.decoder.unknown_ethertype 21 0 -
.decoder.event.ethernet.unknown_ethertype 21 0 -
.flow.tcp_reuse 0 2 -
.flow.end.tcp_state.syn_sent 0 2 -
.exception_policy.tcp.midstream.drop_flow 0 2 -
TREX_GENERIC_stats_chk
.decoder.unknown_ethertype 23 0 -
.decoder.event.ethernet.unknown_ethertype 23 0 -
.tcp.pkt_on_wrong_thread 0 399 -
.tcp.memuse 1824000000 912000000 50.0%
.tcp.reassembly_memuse 2394336 1184736 49.48%
.flow.tcp_reuse 0 7 -
.flow.end.tcp_state.syn_sent 0 7 -
.flow.end.tcp_state.fin_wait2 0 1 -
.flow.end.tcp_state.time_wait 0 6 -
.flow.end.tcp_state.last_ack 0 1 -
.flow.end.tcp_state.close_wait 0 6 -
.app_layer.error.dcerpc_tcp.parser 0 2 -

Pipeline = 29210

@catenacyber
Copy link
Contributor Author

Replaced by #14665

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants