This is an automated email from the ASF dual-hosted git repository. tison pushed a commit to branch c-pointers in repository https://gitbox.apache.org/repos/asf/opendal.git
commit 1b1c09ef1bb8bd0efd763b321792ab57d22e9eac Author: tison <[email protected]> AuthorDate: Tue Oct 8 00:03:50 2024 -0600 refactor: resolve c pointers const Signed-off-by: tison <[email protected]> --- bindings/c/README.md | 6 +-- bindings/c/examples/basic.c | 6 +-- bindings/c/src/error.rs | 12 ------ bindings/c/src/operator.rs | 20 ++++----- bindings/c/src/result.rs | 2 +- bindings/c/src/types.rs | 49 +++++++++++++--------- bindings/c/tests/bdd.cpp | 6 +-- bindings/go/operator.go | 6 +-- bindings/go/stat.go | 1 - .../OpenDAL/Sources/OpenDAL/Data+OpenDAL.swift | 2 +- bindings/zig/test/bdd.zig | 4 +- 11 files changed, 54 insertions(+), 60 deletions(-) diff --git a/bindings/c/README.md b/bindings/c/README.md index bd225ceb14..f4e58ad0dc 100644 --- a/bindings/c/README.md +++ b/bindings/c/README.md @@ -32,13 +32,13 @@ int main() /* We can read it out, make sure the data is the same */ opendal_result_read r = opendal_operator_read(op, "/testpath"); - opendal_bytes* read_bytes = r.data; + opendal_bytes read_bytes = r.data; assert(r.error == NULL); - assert(read_bytes->len == 24); + assert(read_bytes.len == 24); /* Lets print it out */ for (int i = 0; i < 24; ++i) { - printf("%c", read_bytes->data[i]); + printf("%c", read_bytes.data[i]); } printf("\n"); diff --git a/bindings/c/examples/basic.c b/bindings/c/examples/basic.c index 5e4e7925ba..5fde22d9c5 100644 --- a/bindings/c/examples/basic.c +++ b/bindings/c/examples/basic.c @@ -42,13 +42,13 @@ int main() /* We can read it out, make sure the data is the same */ opendal_result_read r = opendal_operator_read(op, "/testpath"); - opendal_bytes* read_bytes = r.data; + opendal_bytes read_bytes = r.data; assert(r.error == NULL); - assert(read_bytes->len == 24); + assert(read_bytes.len == 24); /* Lets print it out */ for (int i = 0; i < 24; ++i) { - printf("%c", read_bytes->data[i]); + printf("%c", read_bytes.data[i]); } printf("\n"); diff --git a/bindings/c/src/error.rs b/bindings/c/src/error.rs index d1c6155ee7..63ae8c99d1 100644 --- a/bindings/c/src/error.rs +++ b/bindings/c/src/error.rs @@ -114,18 +114,6 @@ impl opendal_error { #[no_mangle] pub unsafe extern "C" fn opendal_error_free(ptr: *mut opendal_error) { if !ptr.is_null() { - let message_ptr = &(*ptr).message; - let message_ptr = message_ptr as *const opendal_bytes as *mut opendal_bytes; - if !message_ptr.is_null() { - let data_mut = (*message_ptr).data as *mut u8; - drop(Vec::from_raw_parts( - data_mut, - (*message_ptr).len, - (*message_ptr).len, - )); - } - - // free the pointer drop(Box::from_raw(ptr)) } } diff --git a/bindings/c/src/operator.rs b/bindings/c/src/operator.rs index 04df68ec45..4f1983bdae 100644 --- a/bindings/c/src/operator.rs +++ b/bindings/c/src/operator.rs @@ -47,7 +47,7 @@ static RUNTIME: Lazy<tokio::runtime::Runtime> = Lazy::new(|| { pub struct opendal_operator { /// The pointer to the opendal::BlockingOperator in the Rust code. /// Only touch this on judging whether it is NULL. - inner: *const c_void, + inner: *mut c_void, } impl opendal_operator { @@ -263,8 +263,9 @@ pub unsafe extern "C" fn opendal_operator_write( /// opendal_result_read r = opendal_operator_read(op, "testpath"); /// assert(r.error == NULL); /// -/// opendal_bytes *bytes = r.data; -/// assert(bytes->len == 13); +/// opendal_bytes bytes = r.data; +/// assert(bytes.len == 13); +/// opendal_bytes_free(bytes); /// ``` /// /// # Safety @@ -286,15 +287,12 @@ pub unsafe extern "C" fn opendal_operator_read( .to_str() .expect("malformed path"); match op.deref().read(path) { - Ok(d) => { - let v = Box::new(opendal_bytes::new(d)); - opendal_result_read { - data: Box::into_raw(v), - error: std::ptr::null_mut(), - } - } + Ok(b) => opendal_result_read { + data: opendal_bytes::new(b), + error: std::ptr::null_mut(), + }, Err(e) => opendal_result_read { - data: std::ptr::null_mut(), + data: opendal_bytes::empty(), error: opendal_error::new(e), }, } diff --git a/bindings/c/src/result.rs b/bindings/c/src/result.rs index ba7e1c64a0..e9aede5d47 100644 --- a/bindings/c/src/result.rs +++ b/bindings/c/src/result.rs @@ -49,7 +49,7 @@ pub struct opendal_result_operator_new { #[repr(C)] pub struct opendal_result_read { /// The byte array with length returned by read operations - pub data: *mut opendal_bytes, + pub data: opendal_bytes, /// The error, if ok, it is null pub error: *mut opendal_error, } diff --git a/bindings/c/src/types.rs b/bindings/c/src/types.rs index 795c7a73b5..5e7d664122 100644 --- a/bindings/c/src/types.rs +++ b/bindings/c/src/types.rs @@ -31,7 +31,7 @@ use opendal::Buffer; #[repr(C)] pub struct opendal_bytes { /// Pointing to the byte array on heap - pub data: *const u8, + pub data: *mut u8, /// The length of the byte array pub len: usize, /// The capacity of the byte array @@ -39,32 +39,41 @@ pub struct opendal_bytes { } impl opendal_bytes { + pub(crate) fn empty() -> Self { + Self { + data: std::ptr::null_mut(), + len: 0, + capacity: 0, + } + } + /// Construct a [`opendal_bytes`] from the Rust [`Vec`] of bytes - pub(crate) fn new(buf: Buffer) -> Self { - let vec = buf.to_vec(); - let mut buf = std::mem::ManuallyDrop::new(vec); - let data = buf.as_mut_ptr(); - let len = buf.len(); - let capacity = buf.capacity(); + pub(crate) fn new(b: Buffer) -> Self { + let mut b = std::mem::ManuallyDrop::new(b.to_vec()); Self { - data, - len, - capacity, + data: b.as_mut_ptr(), + len: b.len(), + capacity: b.capacity(), } } /// \brief Frees the heap memory used by the opendal_bytes #[no_mangle] - pub unsafe extern "C" fn opendal_bytes_free(ptr: *mut opendal_bytes) { - if !ptr.is_null() { - // transmuting `*const u8` to `*mut u8` is undefined behavior in any cases - // however, fields type of `opendal_bytes` is already related to the zig binding - // it should be fixed later - let _ = Vec::from_raw_parts((*ptr).data as *mut u8, (*ptr).len, (*ptr).capacity); - // it is too weird that call `Box::new` outside `opendal_bytes::new` but dealloc it here - // also, boxing `opendal_bytes` might not be necessary - // `data` points to heap, so `opendal_bytes` could be passed as a stack value - let _ = Box::from_raw(ptr); + pub unsafe extern "C" fn opendal_bytes_free(bs: opendal_bytes) { + if !bs.data.is_null() { + drop(Vec::from_raw_parts(bs.data, bs.len, bs.capacity)); + } + } +} + +impl Drop for opendal_bytes { + fn drop(&mut self) { + if !self.data.is_null() { + unsafe { + // Safety: the data is not null, and the capacity is correct + drop(Vec::from_raw_parts(self.data, self.len, self.capacity)); + } + self.data = std::ptr::null_mut(); } } } diff --git a/bindings/c/tests/bdd.cpp b/bindings/c/tests/bdd.cpp index 39856e81e5..246dbfda1c 100644 --- a/bindings/c/tests/bdd.cpp +++ b/bindings/c/tests/bdd.cpp @@ -85,9 +85,9 @@ TEST_F(OpendalBddTest, FeatureTest) // The blocking file "test" must have content "Hello, World!" struct opendal_result_read r = opendal_operator_read(this->p, this->path.c_str()); EXPECT_EQ(r.error, nullptr); - EXPECT_EQ(r.data->len, this->content.length()); - for (int i = 0; i < r.data->len; i++) { - EXPECT_EQ(this->content[i], (char)(r.data->data[i])); + EXPECT_EQ(r.data.len, this->content.length()); + for (int i = 0; i < r.data.len; i++) { + EXPECT_EQ(this->content[i], (char)(r.data.data[i])); } // The blocking file should be deleted diff --git a/bindings/go/operator.go b/bindings/go/operator.go index d63bacda61..be5c50f2c7 100644 --- a/bindings/go/operator.go +++ b/bindings/go/operator.go @@ -285,12 +285,12 @@ type bytesFree func(b *opendalBytes) var withBytesFree = withFFI(ffiOpts{ sym: symBytesFree, rType: &ffi.TypeVoid, - aTypes: []*ffi.Type{&ffi.TypePointer}, + aTypes: []*ffi.Type{&typeBytes}, }, func(_ context.Context, ffiCall func(rValue unsafe.Pointer, aValues ...unsafe.Pointer)) bytesFree { - return func(b *opendalBytes) { + return func(b opendalBytes) { ffiCall( nil, - unsafe.Pointer(&b), + b, ) } }) diff --git a/bindings/go/stat.go b/bindings/go/stat.go index 6b83ad605c..7eb7345939 100644 --- a/bindings/go/stat.go +++ b/bindings/go/stat.go @@ -96,7 +96,6 @@ func (op *Operator) Stat(path string) (*Metadata, error) { // } else { // fmt.Println("The file does not exist") // } -// func (op *Operator) IsExist(path string) (bool, error) { isExist := getFFI[operatorIsExist](op.ctx, symOperatorIsExist) return isExist(op.inner, path) diff --git a/bindings/swift/OpenDAL/Sources/OpenDAL/Data+OpenDAL.swift b/bindings/swift/OpenDAL/Sources/OpenDAL/Data+OpenDAL.swift index 8e61933754..e963da16fd 100644 --- a/bindings/swift/OpenDAL/Sources/OpenDAL/Data+OpenDAL.swift +++ b/bindings/swift/OpenDAL/Sources/OpenDAL/Data+OpenDAL.swift @@ -25,7 +25,7 @@ extension Data { /// This can be used to read data from Rust with zero-copying. /// The underlying buffer will be freed when the data gets /// deallocated. - init(openDALBytes: UnsafeMutablePointer<opendal_bytes>) { + init(openDALBytes: opendal_bytes) { let address = UnsafeRawPointer(openDALBytes.pointee.data)! let length = Int(openDALBytes.pointee.len) self.init( diff --git a/bindings/zig/test/bdd.zig b/bindings/zig/test/bdd.zig index 98d49525ce..4408a8f3ef 100644 --- a/bindings/zig/test/bdd.zig +++ b/bindings/zig/test/bdd.zig @@ -24,7 +24,7 @@ test "Opendal BDD test" { const c_str = [*:0]const u8; // define a type for 'const char*' in C const OpendalBDDTest = struct { - p: [*c]const opendal.c.opendal_operator, + p: [*c]opendal.c.opendal_operator, scheme: c_str, path: c_str, content: c_str, @@ -60,8 +60,8 @@ test "Opendal BDD test" { // When Blocking write path "test" with content "Hello, World!" const data: opendal.c.opendal_bytes = .{ .data = testkit.content, - // c_str does not have len field (.* is ptr) .len = std.mem.len(testkit.content), + .capacity = std.mem.len(testkit.content), }; const result = opendal.c.opendal_operator_write(testkit.p, testkit.path, data); try testing.expectEqual(result, null);
