alamb commented on code in PR #10431:
URL: https://github.com/apache/arrow-rs/pull/10431#discussion_r3658829193
##########
arrow-data/src/ffi.rs:
##########
@@ -53,15 +53,21 @@ pub struct FFI_ArrowArray {
pub children: *mut *mut FFI_ArrowArray,
/// Pointer to the underlying array of dictionary values
pub dictionary: *mut FFI_ArrowArray,
- /// Pointer to a producer-provided release callback
- pub release: Option<unsafe extern "C" fn(arg1: *mut FFI_ArrowArray)>,
+ /// Producer-provided release callback.
+ ///
+ /// Private so safe code can't install a callback that [`Drop`] would
invoke.
Review Comment:
If the goal is to avoid potentially unsafe access do we have to change the
other fields to so they aren't pub? For example if I set the `buffers` field
to something invalid that would cause a crash / undefined behavior without
requiring an `unsafe` block 🤔
##########
arrow-array/src/ffi_stream.rs:
##########
@@ -213,6 +219,48 @@ impl FFI_ArrowArrayStream {
private_data: std::ptr::null_mut(),
}
}
+
+ /// Returns the producer-provided release callback, if any.
+ ///
+ /// Lets a consumer wrap release: save this callback, install its own with
+ /// [`FFI_ArrowArrayStream::set_release`], and chain back to it on drop.
See
+ /// <https://github.com/apache/arrow-rs/issues/9771>.
+ pub fn release(&self) -> Option<unsafe extern "C" fn(arg1: *mut Self)> {
+ self.release
+ }
+
+ /// Returns the opaque producer-provided private data pointer.
+ ///
+ /// See [`FFI_ArrowArrayStream::release`] for the intended use.
+ pub fn private_data(&self) -> *mut c_void {
+ self.private_data
+ }
+
+ /// Replaces the release callback, returning the previous one.
+ ///
+ /// # Safety
+ ///
+ /// [`Drop`] calls this callback with a pointer to `self`. The new callback
+ /// must correctly release this stream (usually by chaining to the returned
+ /// one) and must match the [`FFI_ArrowArrayStream::private_data`] it
reads.
+ /// A wrong callback is undefined behavior on drop.
+ pub unsafe fn set_release(
Review Comment:
Another potential API could be
```rust
unsafe fn new_from_parts(...) -> Self {
...
}
```
To allow someone to set the fields as they want, but acknoledging it is not
safe. The benefit of the API in this PR is that we won't have to modify the
existing API if a new field is added to `FFI_Stream` in the future
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
