refactor: resolve c pointers const (#5171)

* refactor: resolve c pointers const Signed-off-by: tison <[email protected]> * fix zig cases Signed-off-by: tison <[email protected]> * fix swift cases Signed-off-by: tison <[email protected]> * fix go cases Signed-off-by: tison <[email protected]> * pass pointer to opendal_bytes_free Signed-off-by: tison <[email protected]> * try fix go case Signed-off-by: tison <[email protected]> * fix(bindings/go): make opendal_bytes pointer Signed-off-by: Hanchin Hsieh <[email protected]> --------- Signed-off-by: tison <[email protected]> Signed-off-by: Hanchin Hsieh <[email protected]> Co-authored-by: Hanchin Hsieh <[email protected]>
apache · Oct 14, 2024 · 2a332f3 · 2a332f3
1 parent a225b19
commit 2a332f3
Show file tree

Hide file tree

Showing 19 changed files with 107 additions and 106 deletions.
diff --git a/bindings/c/README.md b/bindings/c/README.md
@@ -32,18 +32,18 @@ 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");
 
     /* the opendal_bytes read is heap allocated, please free it */
-    opendal_bytes_free(read_bytes);
+    opendal_bytes_free(&read_bytes);
 
     /* the operator_ptr is also heap allocated */
     opendal_operator_free(&op);

diff --git a/bindings/c/examples/basic.c b/bindings/c/examples/basic.c
@@ -42,18 +42,18 @@ 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");
 
     /* the opendal_bytes read is heap allocated, please free it */
-    opendal_bytes_free(read_bytes);
+    opendal_bytes_free(&read_bytes);
 
     /* the operator_ptr is also heap allocated */
     opendal_operator_free(op);

diff --git a/bindings/c/include/opendal.h b/bindings/c/include/opendal.h
@@ -94,7 +94,7 @@ typedef struct opendal_bytes {
   /**
    * Pointing to the byte array on heap
    */
-  const uint8_t *data;
+  uint8_t *data;
   /**
    * The length of the byte array
    */
@@ -113,7 +113,7 @@ typedef struct opendal_bytes {
  * represents no error has taken placed**. If any error has taken place, the caller should check
  * the error code and print the error message.
  *
- * The error code is represented in opendal_code, which is a enum on different type of errors.
+ * The error code is represented in opendal_code, which is an enum on different type of errors.
  * The error messages is represented in opendal_bytes, which is a non-null terminated byte array.
  *
  * \note 1. The error message is on heap, so the error needs to be freed by the caller, by calling
@@ -217,7 +217,7 @@ typedef struct opendal_operator {
    * The pointer to the opendal::BlockingOperator in the Rust code.
    * Only touch this on judging whether it is NULL.
    */
-  const void *inner;
+  void *inner;
 } opendal_operator;
 
 /**
@@ -273,7 +273,7 @@ typedef struct opendal_result_read {
   /**
    * The byte array with length returned by read operations
    */
-  struct opendal_bytes *data;
+  struct opendal_bytes data;
   /**
    * The error, if ok, it is null
    */
@@ -814,7 +814,7 @@ struct opendal_result_operator_new opendal_operator_new(const char *scheme,
  */
 struct opendal_error *opendal_operator_write(const struct opendal_operator *op,
                                              const char *path,
-                                             struct opendal_bytes bytes);
+                                             const struct opendal_bytes *bytes);
 
 /**
  * \brief Blocking read the data from `path`.
@@ -842,8 +842,9 @@ struct opendal_error *opendal_operator_write(const struct opendal_operator *op,
  * 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
@@ -1394,7 +1395,7 @@ void opendal_reader_free(struct opendal_reader *ptr);
  * \brief Write data to the writer.
  */
 struct opendal_result_writer_write opendal_writer_write(struct opendal_writer *self,
-                                                        struct opendal_bytes bytes);
+                                                        const struct opendal_bytes *bytes);
 
 /**
  * \brief Frees the heap memory used by the opendal_writer.

diff --git a/bindings/c/src/error.rs b/bindings/c/src/error.rs
@@ -82,7 +82,7 @@ impl From<core::ErrorKind> for opendal_code {
 /// represents no error has taken placed**. If any error has taken place, the caller should check
 /// the error code and print the error message.
 ///
-/// The error code is represented in opendal_code, which is a enum on different type of errors.
+/// The error code is represented in opendal_code, which is an enum on different type of errors.
 /// The error messages is represented in opendal_bytes, which is a non-null terminated byte array.
 ///
 /// \note 1. The error message is on heap, so the error needs to be freed by the caller, by calling
@@ -114,19 +114,7 @@ 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))
+            drop(Box::from_raw(ptr));
         }
     }
 }
diff --git 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 {
@@ -226,7 +226,7 @@ pub unsafe extern "C" fn opendal_operator_new(
 pub unsafe extern "C" fn opendal_operator_write(
     op: &opendal_operator,
     path: *const c_char,
-    bytes: opendal_bytes,
+    bytes: &opendal_bytes,
 ) -> *mut opendal_error {
     assert!(!path.is_null());
     let path = std::ffi::CStr::from_ptr(path)
@@ -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
@@ -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
@@ -31,46 +31,58 @@ 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
     pub capacity: usize,
 }
 
 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);
+            let bs = &mut *ptr;
+            if !bs.data.is_null() {
+                drop(Vec::from_raw_parts(bs.data, bs.len, bs.capacity));
+                bs.data = std::ptr::null_mut();
+                bs.len = 0;
+                bs.capacity = 0;
+            }
+        }
+    }
+}
+
+impl Drop for opendal_bytes {
+    fn drop(&mut self) {
+        unsafe {
+            // Safety: the pointer is always valid
+            Self::opendal_bytes_free(self);
         }
     }
 }
 
-impl From<opendal_bytes> for Buffer {
-    fn from(v: opendal_bytes) -> Self {
+impl From<&opendal_bytes> for Buffer {
+    fn from(v: &opendal_bytes) -> Self {
         let slice = unsafe { std::slice::from_raw_parts(v.data, v.len) };
         Buffer::from(bytes::Bytes::copy_from_slice(slice))
     }

diff --git a/bindings/c/src/writer.rs b/bindings/c/src/writer.rs
@@ -49,7 +49,7 @@ impl opendal_writer {
     #[no_mangle]
     pub unsafe extern "C" fn opendal_writer_write(
         &mut self,
-        bytes: opendal_bytes,
+        bytes: &opendal_bytes,
     ) -> opendal_result_writer_write {
         let size = bytes.len;
         match self.deref_mut().write(bytes) {

diff --git a/bindings/c/tests/bdd.cpp b/bindings/c/tests/bdd.cpp
@@ -61,7 +61,7 @@ TEST_F(OpendalBddTest, FeatureTest)
         .data = (uint8_t*)this->content.c_str(),
         .len = this->content.length(),
     };
-    opendal_error* error = opendal_operator_write(this->p, this->path.c_str(), data);
+    opendal_error* error = opendal_operator_write(this->p, this->path.c_str(), &data);
     EXPECT_EQ(error, nullptr);
 
     // The blocking file "test" should exist
@@ -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
@@ -99,7 +99,7 @@ TEST_F(OpendalBddTest, FeatureTest)
 
     opendal_result_operator_writer writer = opendal_operator_writer(this->p, this->path.c_str());
     EXPECT_EQ(writer.error, nullptr);
-    opendal_result_writer_write w = opendal_writer_write(writer.writer, data);
+    opendal_result_writer_write w = opendal_writer_write(writer.writer, &data);
     EXPECT_EQ(w.error, nullptr);
     EXPECT_EQ(w.size, this->content.length());
     opendal_writer_free(writer.writer);
@@ -120,7 +120,7 @@ TEST_F(OpendalBddTest, FeatureTest)
     error = opendal_operator_delete(this->p, this->path.c_str());
     EXPECT_EQ(error, nullptr);
 
-    opendal_bytes_free(r.data);
+    opendal_bytes_free(&r.data);
 
     // The directory "tmpdir/" should exist and should be a directory
     error = opendal_operator_create_dir(this->p, "tmpdir/");

diff --git a/bindings/c/tests/error_msg.cpp b/bindings/c/tests/error_msg.cpp
@@ -59,7 +59,7 @@ TEST_F(OpendalErrorTest, ErrorReadTest)
     ASSERT_GT(error_msg->len, 0);
 
     // the opendal_bytes read is heap allocated, please free it
-    opendal_bytes_free(r.data);
+    opendal_bytes_free(&r.data);
 
     // free the error
     opendal_error_free(r.error);

diff --git a/bindings/c/tests/list.cpp b/bindings/c/tests/list.cpp
@@ -65,7 +65,7 @@ TEST_F(OpendalListTest, ListDirTest)
     };
 
     // write must succeed
-    EXPECT_EQ(opendal_operator_write(this->p, path.c_str(), data),
+    EXPECT_EQ(opendal_operator_write(this->p, path.c_str(), &data),
         nullptr);
 
     // list must succeed since the write succeeded

diff --git a/bindings/go/error.go b/bindings/go/error.go
@@ -77,7 +77,7 @@ func parseError(ctx context.Context, err *opendalError) error {
 	defer free(err)
 	return &Error{
 		code:    ErrorCode(err.code),
-		message: string(parseBytes(&err.message)),
+		message: string(parseBytes(err.message)),
 	}
 }
 

diff --git a/bindings/go/reader.go b/bindings/go/reader.go
@@ -68,8 +68,7 @@ func (op *Operator) Read(path string) ([]byte, error) {
 	data := parseBytes(bytes)
 	if len(data) > 0 {
 		free := getFFI[bytesFree](op.ctx, symBytesFree)
-		free(bytes)
-
+		free(&bytes)
 	}
 	return data, nil
 }
@@ -203,17 +202,17 @@ func (r *Reader) Close() error {
 
 const symOperatorRead = "opendal_operator_read"
 
-type operatorRead func(op *opendalOperator, path string) (*opendalBytes, error)
+type operatorRead func(op *opendalOperator, path string) (opendalBytes, error)
 
 var withOperatorRead = withFFI(ffiOpts{
 	sym:    symOperatorRead,
 	rType:  &typeResultRead,
 	aTypes: []*ffi.Type{&ffi.TypePointer, &ffi.TypePointer},
 }, func(ctx context.Context, ffiCall func(rValue unsafe.Pointer, aValues ...unsafe.Pointer)) operatorRead {
-	return func(op *opendalOperator, path string) (*opendalBytes, error) {
+	return func(op *opendalOperator, path string) (opendalBytes, error) {
 		bytePath, err := unix.BytePtrFromString(path)
 		if err != nil {
-			return nil, err
+			return opendalBytes{}, err
 		}
 		var result resultRead
 		ffiCall(

diff --git 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)