Skip to content

Conversation

goatgoose
Copy link
Contributor

@goatgoose goatgoose commented Sep 25, 2025

Resolved issues:

Part of #2767

Description of changes:

This PR allows C types to be defined and associated with events. This will allow the future C APIs that publish events to be provided with the defined C types as arguments.

For an event to be published, the s2n-events publisher must be provided with information that's specific to each event. For example, for an ApplicationProtocolInformation event, the publisher must be provided with the array of bytes associated with the received ALPN. This is handled by a "builder" event struct that's generated according to how the event is defined by the user.

The C API for s2n-events will allow s2n-tls to invoke the s2n-events publisher, so s2n-tls will also need to provide the publisher with information that's specific to each event. My plan is to use a similar event struct which contains the event-specific data that will be passed to the on_event API for that event in the publisher. Unfortunately, it will be difficult to automatically generate the C argument struct for this purpose since the Rust and C structs will generally be different. For example, consider the ApplicationProtocolInformation event, which needs an array of bytes as input. In Rust, this is handled with a single &'a [u8] field. In C, this needs to be handled with a data: *const u8 field and a len: u32 field.

Rather than try to do this conversion as part of the code generation, this PR adds a way for the developer to specify a C-specific struct for each event. The developer can choose the representation of the event data that makes the most sense for C, and then implement IntoEvent on the C struct to convert it into the Rust "builder" struct so the event can be published.

Call-outs:

This PR only adds support for associating C types with events. These C types will be used when generating the C API for the publisher interface, but this will be done in a separate PR.

Testing:

New tests were added to make sure the IntoEvent implementations to convert C structs to the Rust event structs are correct.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@goatgoose goatgoose force-pushed the s2n-events-c-args branch 3 times, most recently from 08bec44 to 8c3d5fc Compare September 26, 2025 15:23
@Mark-Simulacrum
Copy link
Collaborator

Rather than try to do this conversion as part of the code generation, this PR adds a way to the developer to specify a C-specific struct for each event. The developer can choose the representation of the event data that makes most sense to C, and then implement IntoEvent on the C struct to convert it into the Rust "builder" struct so the event can be published.

FWIW, I will say that this seems pretty painful. The vast majority of events are composed of 'simple' types (enums, integers, &[u8]). Forcing handwriting all of that in repr(C) -- and presumably a C header, too? -- feels pretty frustrating, and potentially error prone (especially for enums). It's also fairly repetitive.

Could we try to generate the builder repr(C) structs with fields defined as:

trait CEvent {
    type CType;
    // SAFETY: `Self::CType` must have been produced by the generated C code.
    unsafe fn from_c(v: Self::CType) -> Self;
}

#[repr(C)]
struct CBuilder {
     field_1: <$field_1_ty as CEvent>::CType,
}
typedef rust_u64 uint64_t; // or something like this, this part is probably handwritten, matching with the impls of CEvent's `type CType ...`.

struct {
    field_1: rust_$field_ty,
} CBuilder;

Not sure how far that will get us... but maybe it covers 90% of events essentially automatically?

@goatgoose
Copy link
Contributor Author

goatgoose commented Sep 26, 2025

Thanks! I'll look into doing this!

and presumably a C header, too?

I'm planning on automatically generating the C header with cbindgen.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants