From 04caf21da4f706948540e76e7dd2994a429e96e6 Mon Sep 17 00:00:00 2001 From: Mathias Kraus Date: Fri, 12 Jul 2024 22:18:14 +0200 Subject: [PATCH] [#210] Better naming convention for C API --- iceoryx2-ffi/ffi/README.md | 47 +++++--- iceoryx2-ffi/ffi/src/node.rs | 14 +-- iceoryx2-ffi/ffi/src/node_builder.rs | 22 +++- iceoryx2-ffi/ffi/src/node_name.rs | 167 +++++++++++++++------------ 4 files changed, 149 insertions(+), 101 deletions(-) diff --git a/iceoryx2-ffi/ffi/README.md b/iceoryx2-ffi/ffi/README.md index e22150b1..8c16aae4 100644 --- a/iceoryx2-ffi/ffi/README.md +++ b/iceoryx2-ffi/ffi/README.md @@ -2,33 +2,35 @@ - all constructs start with `iox2_` - `structs` end with a `_t` -- mutable handles end with a `_mut_h` and are a type definition to a `*mut iox2_foo_storage_t` -- immutable handles end with a `_h` and are a type definition to a `*const iox2_foo_storage_internal_t` which holds the Rust type `Option` +- handles end with a `_h` and are a type definition to a `struct iox2_foo_h_t;` as `pub type iox2_foo_h = *mut iox2_foo_h_t` +- immutable pointer to the Rust type end with a `_ptr` and are a type definition to a `struct iox2_foo_ptr_t;` as `pub type iox2_foo_ptr = *mut iox2_foo_ptr_t` +- mutable pointer to the Rust type end with a `_mut_ptr` and are a type definition to a `struct iox2_foo_mut_ptr_t;` as `pub type iox2_foo_mut_ptr = *mut iox2_foo_mut_ptr_t` - `enums` ends with a `_e` # Pattern for Type Erasure -The type erasure is usually done in two stages with `iox2_foo_storage_internal_t` and `iox2_foo_storage_t`. +The type erasure is usually done in two stages with `iox2_foo_storage_t` and `iox2_foo_t`. -The `iox2_foo_storage_internal_t` is the storage for the Rust type `Option` and must match the size and alignment of `Option`. +The `iox2_foo_storage_t` is the storage for the Rust type `Option` and must match the size and alignment of `Option`. If the internal storage must hold multiple types, the size and alignment is respectively the max value of the types. -The struct is not supposed to be used standalone but always in combination with an `iox2_foo_storage_t`. +The struct is not supposed to be used standalone but always in combination with an `iox2_foo_t`. Assuming the size is 160 and the alignment is 8, then the storage is defined as following ```rs #[repr(C)] #[repr(align(8))] // alignment of Option -pub struct iox2_foo_storage_internal_t { +pub struct iox2_foo_storage_t { internal: [u8; 160], // magic number obtained with size_of::>() } ``` -The `iox2_foo_storage_t` is the actual storage that is used by the user. It contains the internal storage, a deleter and -optionally further data, e.g. to distinguish between multiple allowed types of `iox2_foo_storage_internal_t`. +The `iox2_foo_t` is the actual type that is used by the user. It contains the internal storage, a deleter and +optionally further data, e.g. to distinguish between multiple allowed types of `iox2_foo_storage_t`. ```rs #[repr(C)] -pub struct iox2_foo_storage_t { - internal: iox2_foo_storage_internal_t, - deleter: fn(*mut iox2_foo_storage_t), +pub struct iox2_foo_t { + /// cbindgen:rename=internal + foo: iox2_foo_storage_t, + deleter: fn(*mut iox2_foo_t), } ``` @@ -38,10 +40,25 @@ corresponding `iox2_foo_drop` shall be used to destruct the underlying Rust type If the Rust API takes the ownership of `Foo`, the C API will also take the ownership of the handle and `iox2_foo_drop` shall not be called. -The corresponding handles are defined like this +When the handle is passed to a function, the ownership of the underlying data is moved to that specific function and the `*_h` handles +as well as all the `*_ptr` related to that handle are invalid. Accessing the handles or pointer afterwards lead to undefined behavior. +The only exception are the `iox2_cast_*` functions which can be used to get `_ptr` and `_mut_ptr` pointer the the Rust type. + +The corresponding handle and pointer are defined like this ```rs -pub type iox2_foo_mut_h = *mut iox2_foo_storage_t; -pub type iox2_foo_h = *const iox2_foo_storage_internal_t; +#[repr(C)] +pub struct iox2_foo_h_t; +pub type iox2_foo_h = *mut iox2_foo_h_t; + +#[repr(C)] +pub struct iox2_foo_ptr_t; +pub type iox2_foo_ptr = *const iox2_foo_ptr_t; + +#[repr(C)] +pub struct iox2_foo_mut_ptr_t; +pub type iox2_foo_mut_ptr = *mut iox2_foo_mut_ptr_t; ``` -The `_mut_h` handle is in general created by a builder and the `_h` handle is in general provided by a function, e.g. as return value. +The `_h` handle is in general created by a builder and the `_ptr` pointer ar in general provided by a function, e.g. as return value. + +The `src/node_name.rs` file can be used as a more comprehensive example on how to implement an FFI binding for a specific type. diff --git a/iceoryx2-ffi/ffi/src/node.rs b/iceoryx2-ffi/ffi/src/node.rs index 05dc3df6..bedb8d73 100644 --- a/iceoryx2-ffi/ffi/src/node.rs +++ b/iceoryx2-ffi/ffi/src/node.rs @@ -13,7 +13,7 @@ #![allow(non_camel_case_types)] use crate::{ - iox2_callback_progression_e, iox2_config_h, iox2_node_name_h, iox2_service_builder_mut_h, + iox2_callback_progression_e, iox2_config_h, iox2_node_name_ptr, iox2_service_builder_mut_h, iox2_service_builder_storage_t, iox2_service_name_mut_h, iox2_service_type_e, IntoCInt, IOX2_OK, }; @@ -129,7 +129,7 @@ pub type iox2_node_list_callback_context = *mut c_void; /// /// * [`iox2_node_state_e`] /// * [`iox2_node_id_h`] -/// * [`iox2_node_name_h`](crate::iox2_node_name_h) -> `NULL` for `iox2_node_state_e::INACCESSIBLE` and `iox2_node_state_e::UNDEFINED` +/// * [`iox2_node_name_ptr`](crate::iox2_node_name_ptr) -> `NULL` for `iox2_node_state_e::INACCESSIBLE` and `iox2_node_state_e::UNDEFINED` /// * [`iox2_config_h`](crate::iox2_config_h) -> `NULL` for `iox2_node_state_e::INACCESSIBLE` and `iox2_node_state_e::UNDEFINED` /// * [`iox2_node_list_callback_context`] -> provided by the user to [`iox2_node_list`] and can be `NULL` /// @@ -137,7 +137,7 @@ pub type iox2_node_list_callback_context = *mut c_void; pub type iox2_node_list_callback = extern "C" fn( iox2_node_state_e, iox2_node_id_h, - iox2_node_name_h, + iox2_node_name_ptr, iox2_config_h, iox2_node_list_callback_context, ) -> iox2_callback_progression_e; @@ -146,13 +146,13 @@ pub type iox2_node_list_callback = extern "C" fn( // BEGIN C API -/// Returns the [`iox2_node_name_h`](crate::iox2_node_name_h), an immutable handle to the node name. +/// Returns the [`iox2_node_name_ptr`](crate::iox2_node_name_ptr), an immutable pointer to the node name. /// /// # Safety /// /// The `node_handle` must be valid and obtained by [`iox2_node_builder_create`](crate::iox2_node_builder_create)! #[no_mangle] -pub unsafe extern "C" fn iox2_node_name(node_handle: iox2_node_mut_h) -> iox2_node_name_h { +pub unsafe extern "C" fn iox2_node_name(node_handle: iox2_node_mut_h) -> iox2_node_name_ptr { debug_assert!(!node_handle.is_null()); unsafe { @@ -264,7 +264,7 @@ fn iox2_node_list_impl( } } -/// Calls the callback repeatedly with an [`iox2_node_state_e`], [`iox2_node_id_h`], [´iox2_node_name_h´] and [`iox2_config_h`] for +/// Calls the callback repeatedly with an [`iox2_node_state_e`], [`iox2_node_id_h`], [´iox2_node_name_ptr´] and [`iox2_config_h`] for /// all [`Node`](iceoryx2::node::Node)s in the system under a given [`Config`](iceoryx2::config::Config). /// /// # Arguments @@ -453,7 +453,7 @@ mod test { extern "C" fn node_list_callback( node_state: iox2_node_state_e, _node_id: iox2_node_id_h, - _node_name: iox2_node_name_h, + _node_name: iox2_node_name_ptr, _config: iox2_config_h, ctx: iox2_node_list_callback_context, ) -> iox2_callback_progression_e { diff --git a/iceoryx2-ffi/ffi/src/node_builder.rs b/iceoryx2-ffi/ffi/src/node_builder.rs index e5eef2d2..c8659abd 100644 --- a/iceoryx2-ffi/ffi/src/node_builder.rs +++ b/iceoryx2-ffi/ffi/src/node_builder.rs @@ -13,7 +13,7 @@ #![allow(non_camel_case_types)] use crate::{ - iox2_node_mut_h, iox2_node_name_drop, iox2_node_name_mut_h, iox2_node_storage_t, + iox2_node_mut_h, iox2_node_name_drop, iox2_node_name_h, iox2_node_name_t, iox2_node_storage_t, iox2_service_type_e, IntoCInt, IOX2_OK, }; @@ -142,16 +142,28 @@ pub unsafe extern "C" fn iox2_node_builder_new( handle } +/// Sets the node name for the builder +/// +/// # Arguments +/// +/// * `node_builder_handle` - Must be a valid [`iox2_node_builder_mut_h`] obtained by [`iox2_node_builder_new`]. +/// * `node_name_handle` - Must be a valid [`iox2_node_name_h`] obtained by [`iox2_node_name_new`](crate::iox2_node_name_new). +/// +/// Returns IOX2_OK +/// +/// # Safety +/// +/// `node_builder_handle` as well as `node_name_handle` must be valid handles #[no_mangle] pub unsafe extern "C" fn iox2_node_builder_set_name( node_builder_handle: iox2_node_builder_mut_h, - node_name_handle_mut: iox2_node_name_mut_h, + node_name_handle: iox2_node_name_h, ) -> c_int { debug_assert!(!node_builder_handle.is_null()); - debug_assert!(!node_name_handle_mut.is_null()); + debug_assert!(!node_name_handle.is_null()); - let node_name = unsafe { (*node_name_handle_mut).take().unwrap() }; - iox2_node_name_drop(node_name_handle_mut); + let node_name = unsafe { (*iox2_node_name_t::cast(node_name_handle)).take().unwrap() }; + iox2_node_name_drop(node_name_handle); let node_builder = std::mem::take(unsafe { (*node_builder_handle).node_builder_assume_init() }); let node_builder = node_builder.name(node_name); diff --git a/iceoryx2-ffi/ffi/src/node_name.rs b/iceoryx2-ffi/ffi/src/node_name.rs index 563db608..3b7fcff4 100644 --- a/iceoryx2-ffi/ffi/src/node_name.rs +++ b/iceoryx2-ffi/ffi/src/node_name.rs @@ -26,74 +26,90 @@ use std::alloc::{alloc, dealloc, Layout}; #[repr(C)] #[repr(align(8))] // alignment of Option -pub struct iox2_node_name_storage_internal_t { +pub struct iox2_node_name_storage_t { internal: [u8; 24], // magic number obtained with size_of::>() } -impl iox2_node_name_storage_internal_t { +impl iox2_node_name_storage_t { const fn assert_storage_layout() { static_assert_ge::< - { align_of::() }, + { align_of::() }, { align_of::>() }, >(); static_assert_ge::< - { size_of::() }, + { size_of::() }, { size_of::>() }, >(); } fn init(&mut self, node_name: NodeName) { - iox2_node_name_storage_internal_t::assert_storage_layout(); + iox2_node_name_storage_t::assert_storage_layout(); unsafe { &mut *(self as *mut Self).cast::>>() } .write(Some(node_name)); } unsafe fn assume_init_mut(&mut self) -> &mut Option { - (&mut *(self as *mut Self).cast::>>()).assume_init_mut() + (*(self as *mut Self).cast::>>()).assume_init_mut() } unsafe fn assume_init_ref(&self) -> &Option { - (&*(self as *const Self).cast::>>()).assume_init_ref() - } - - unsafe fn _assume_init_mut_inner(&mut self) -> &mut NodeName { - self.assume_init_mut().as_mut().unwrap() - } - - unsafe fn assume_init_ref_inner(&self) -> &NodeName { - self.assume_init_ref().as_ref().unwrap() + (*(self as *const Self).cast::>>()).assume_init_ref() } } #[repr(C)] -pub struct iox2_node_name_storage_t { +pub struct iox2_node_name_t { /// cbindgen:rename=internal - node_name: iox2_node_name_storage_internal_t, - deleter: fn(*mut iox2_node_name_storage_t), + node_name: iox2_node_name_storage_t, + deleter: fn(*mut iox2_node_name_t), } -/// The handle to use for the `iox2_node_name_*` functions which mutate the node name -pub type iox2_node_name_mut_h = *mut iox2_node_name_storage_t; +impl iox2_node_name_t { + pub(crate) fn cast(node_name: iox2_node_name_h) -> *mut Self { + node_name as *mut _ as *mut Self + } -/// The immutable handle to the underlying `NodeName` -pub type iox2_node_name_h = *const iox2_node_name_storage_internal_t; + pub(crate) fn cast_node_name(node_name_ptr: iox2_node_name_ptr) -> *const NodeName { + debug_assert!(!node_name_ptr.is_null()); + let maybe_node_name = + unsafe { (*(node_name_ptr as *const _ as *const Option)).as_ref() }; + debug_assert!(maybe_node_name.is_some()); + unsafe { maybe_node_name.unwrap_unchecked() as *const _ } + } -impl iox2_node_name_storage_t { pub(crate) fn take(&mut self) -> Option { unsafe { self.node_name.assume_init_mut().take() } } - fn alloc() -> *mut iox2_node_name_storage_t { - unsafe { alloc(Layout::new::()) as *mut iox2_node_name_storage_t } + fn alloc() -> *mut iox2_node_name_t { + unsafe { alloc(Layout::new::()) as *mut iox2_node_name_t } } - fn dealloc(storage: *mut iox2_node_name_storage_t) { + fn dealloc(storage: *mut iox2_node_name_t) { unsafe { - dealloc(storage as *mut _, Layout::new::()); + dealloc(storage as *mut _, Layout::new::()); } } } +#[repr(C)] +pub struct iox2_node_name_h_t; + +/// The handle for `iox2_node_name_t`. Passing the handle to an function transfers the ownership. +pub type iox2_node_name_h = *mut iox2_node_name_h_t; + +#[repr(C)] +pub struct iox2_node_name_ptr_t; + +/// The immutable pointer to the underlying `NodeName` +pub type iox2_node_name_ptr = *const iox2_node_name_ptr_t; + +#[repr(C)] +pub struct iox2_node_name_mut_ptr_t; + +/// The mutable pointer to the underlying `NodeName` +pub type iox2_node_name_mut_ptr = *mut iox2_node_name_mut_ptr_t; + // END type definition // BEGIN C API @@ -102,35 +118,35 @@ impl iox2_node_name_storage_t { /// /// # Arguments /// -/// * `node_name_storage` - Must be either a NULL pointer or a pointer to a valid [`iox2_node_name_storage_t`]. If it is a NULL pointer, the storage will be allocated on the heap. -/// * `node_name` - Must be valid node name string. +/// * `node_name_struct_ptr` - Must be either a NULL pointer or a pointer to a valid [`iox2_node_name_t`]. If it is a NULL pointer, the storage will be allocated on the heap. +/// * `node_name_str` - Must be valid node name string. /// * `node_name_len` - The length of the node name string, not including a null termination. -/// * `node_name_handle_mut_ptr` - An uninitialized or dangling [`iox2_node_name_mut_h`] handle which will be initialized by this function call. +/// * `node_name_handle_ptr` - An uninitialized or dangling [`iox2_node_name_h`] handle which will be initialized by this function call. /// /// Returns IOX2_OK on success, an [`iox2_semantic_string_error_e`](crate::iox2_semantic_string_error_e) otherwise. /// /// # Safety /// -/// Terminates if `node_name` or `node_name_handle_mut_ptr` is a NULL pointer! -/// It is undefined behavior to pass a `node_name_len` which is larger than the actual length of `node_name`! +/// Terminates if `node_name_str` or `node_name_handle_ptr` is a NULL pointer! +/// It is undefined behavior to pass a `node_name_len` which is larger than the actual length of `node_name_str`! #[no_mangle] pub unsafe extern "C" fn iox2_node_name_new( - node_name_storage: *mut iox2_node_name_storage_t, - node_name: *const c_char, + node_name_struct_ptr: *mut iox2_node_name_t, + node_name_str: *const c_char, node_name_len: c_int, - node_name_handle_mut_ptr: *mut iox2_node_name_mut_h, + node_name_handle_ptr: *mut iox2_node_name_h, ) -> c_int { - debug_assert!(!node_name.is_null()); - debug_assert!(!node_name_handle_mut_ptr.is_null()); + debug_assert!(!node_name_str.is_null()); + debug_assert!(!node_name_handle_ptr.is_null()); - *node_name_handle_mut_ptr = std::ptr::null_mut(); + *node_name_handle_ptr = std::ptr::null_mut(); - let mut handle = node_name_storage; - fn no_op(_storage: *mut iox2_node_name_storage_t) {} - let mut deleter: fn(*mut iox2_node_name_storage_t) = no_op; + let mut handle = node_name_struct_ptr; + fn no_op(_storage: *mut iox2_node_name_t) {} + let mut deleter: fn(*mut iox2_node_name_t) = no_op; if handle.is_null() { - handle = iox2_node_name_storage_t::alloc(); - deleter = iox2_node_name_storage_t::dealloc; + handle = iox2_node_name_t::alloc(); + deleter = iox2_node_name_t::dealloc; } debug_assert!(!handle.is_null()); @@ -138,7 +154,7 @@ pub unsafe extern "C" fn iox2_node_name_new( (*handle).deleter = deleter; } - let node_name = slice::from_raw_parts(node_name as *const u8, node_name_len as usize); + let node_name = slice::from_raw_parts(node_name_str as *const u8, node_name_len as usize); let node_name = if let Ok(node_name) = str::from_utf8(node_name) { node_name @@ -159,52 +175,54 @@ pub unsafe extern "C" fn iox2_node_name_new( (*handle).node_name.init(node_name); } - *node_name_handle_mut_ptr = handle; + *node_name_handle_ptr = handle as *mut _ as *mut _; IOX2_OK } -/// This function converts a [`iox2_node_name_mut_h`] into a [`iox2_node_name_h`] +/// This function casts a [`iox2_node_name_h`] into a [`iox2_node_name_ptr`] /// /// # Arguments /// -/// * `node_name_handle_mut` obtained by [`iox2_node_name_new`] +/// * `node_name_handle` obtained by [`iox2_node_name_new`] /// -/// Returns a [`iox2_node_name_h`] +/// Returns a [`iox2_node_name_ptr`] /// /// # Safety /// -/// The `node_name_handle_mut` must be a valid handle. -/// The `node_name_handle_mut` is still valid after the call to this function. +/// The `node_name_handle` must be a valid handle. +/// The `node_name_handle` is still valid after the call to this function. #[no_mangle] -pub unsafe extern "C" fn iox2_into_node_name_h( - node_name_handle_mut: iox2_node_name_mut_h, -) -> iox2_node_name_h { - debug_assert!(!node_name_handle_mut.is_null()); +pub unsafe extern "C" fn iox2_cast_node_name_ptr( + node_name_handle: iox2_node_name_h, +) -> iox2_node_name_ptr { + debug_assert!(!node_name_handle.is_null()); - &(*node_name_handle_mut).node_name as *const _ + (*iox2_node_name_t::cast(node_name_handle)) + .node_name + .assume_init_ref() as *const _ as *const _ } /// This function gives access to the node name as a C-style string /// /// # Arguments /// -/// * `node_name_handle` obtained by e.g. [`iox2_into_node_name_h`] or a function returning a [`iox2_node_name_h`] +/// * `node_name_ptr` obtained by e.g. [`iox2_cast_node_name_ptr`] or a function returning a [`iox2_node_name_ptr`] /// * `node_name_len` can be used to get the length of the C-style string if not `NULL` /// /// Returns zero terminated C-style string /// /// # Safety /// -/// The `node_name_handle` must be a valid handle. +/// The `node_name_ptr` must be a valid pointer to a node name. #[no_mangle] pub unsafe extern "C" fn iox2_node_name_as_c_str( - node_name_handle: iox2_node_name_h, + node_name_ptr: iox2_node_name_ptr, node_name_len: *mut c_int, ) -> *const c_char { - debug_assert!(!node_name_handle.is_null()); + debug_assert!(!node_name_ptr.is_null()); - let node_name = (*node_name_handle).assume_init_ref_inner(); + let node_name = &*iox2_node_name_t::cast_node_name(node_name_ptr); if !node_name_len.is_null() { unsafe { @@ -217,22 +235,24 @@ pub unsafe extern "C" fn iox2_node_name_as_c_str( /// This function needs to be called to destroy the node name! /// -/// In general, this function is not required to call, since [`iox2_node_builder_set_name`](crate::iox2_node_builder_set_name) will consume the [`iox2_node_name_mut_h`] handle. +/// In general, this function is not required to call, since [`iox2_node_builder_set_name`](crate::iox2_node_builder_set_name) will consume the [`iox2_node_name_h`] handle. /// /// # Arguments /// -/// * `node_name_handle` - A valid [`iox2_node_name_mut_h`] +/// * `node_name_handle` - A valid [`iox2_node_name_h`] /// /// # Safety /// -/// The `node_name_handle_mut` is invalid after the return of this function and leads to undefined behavior if used in another function call! -/// The corresponding [`iox2_node_name_storage_t`] can be re-used with a call to [`iox2_node_name_new`]! +/// The `node_name_handle` is invalid after the return of this function and leads to undefined behavior if used in another function call! +/// The corresponding [`iox2_node_name_t`] can be re-used with a call to [`iox2_node_name_new`]! #[no_mangle] -pub unsafe extern "C" fn iox2_node_name_drop(node_name_handle_mut: iox2_node_name_mut_h) { - debug_assert!(!node_name_handle_mut.is_null()); +pub unsafe extern "C" fn iox2_node_name_drop(node_name_handle: iox2_node_name_h) { + debug_assert!(!node_name_handle.is_null()); + + let node_name_struct = &mut (*iox2_node_name_t::cast(node_name_handle)); - (*node_name_handle_mut).node_name.assume_init_mut().take(); - ((*node_name_handle_mut).deleter)(node_name_handle_mut); + node_name_struct.node_name.assume_init_mut().take(); + (node_name_struct.deleter)(node_name_struct); } // END C API @@ -246,8 +266,7 @@ mod test { #[test] fn assert_storage_size() { // all const functions; if it compiles, the storage size is sufficient - const _STORAGE_LAYOUT_CHECK: () = - iox2_node_name_storage_internal_t::assert_storage_layout(); + const _STORAGE_LAYOUT_CHECK: () = iox2_node_name_storage_t::assert_storage_layout(); } #[test] @@ -255,18 +274,18 @@ mod test { unsafe { let expected_node_name = NodeName::new("hypnotaod")?; - let mut node_name_handle_mut: iox2_node_name_mut_h = std::ptr::null_mut(); + let mut node_name_handle: iox2_node_name_h = std::ptr::null_mut(); let ret_val = iox2_node_name_new( std::ptr::null_mut(), expected_node_name.as_str().as_ptr() as *const _, expected_node_name.len() as _, - &mut node_name_handle_mut, + &mut node_name_handle, ); assert_that!(ret_val, eq(IOX2_OK)); let mut node_name_len = 0; let node_name_c_str = iox2_node_name_as_c_str( - iox2_into_node_name_h(node_name_handle_mut), + iox2_cast_node_name_ptr(node_name_handle), &mut node_name_len, ); @@ -275,7 +294,7 @@ mod test { assert_that!(node_name, eq(expected_node_name.as_str())); - iox2_node_name_drop(node_name_handle_mut); + iox2_node_name_drop(node_name_handle); Ok(()) }