From 40e6d994d5941b2aaedbfd272dee5e6190cdf865 Mon Sep 17 00:00:00 2001 From: Enwei Jiao Date: Wed, 11 Oct 2023 14:54:41 -0700 Subject: [PATCH 1/4] Add blocking_read_with_buffer for C binding --- bindings/c/include/opendal.h | 47 ++++++++++++++++++++++++ bindings/c/src/lib.rs | 69 ++++++++++++++++++++++++++++++++++++ bindings/c/tests/bdd.cpp | 10 ++++++ 3 files changed, 126 insertions(+) diff --git a/bindings/c/include/opendal.h b/bindings/c/include/opendal.h index 51d66aadb3c0..e76558c3a110 100644 --- a/bindings/c/include/opendal.h +++ b/bindings/c/include/opendal.h @@ -560,6 +560,53 @@ struct opendal_error *opendal_operator_blocking_write(const struct opendal_opera struct opendal_result_read opendal_operator_blocking_read(const struct opendal_operator_ptr *ptr, const char *path); +/** + * \brief Blockingly read the data from `path`. + * + * Read the data out from `path` blockingly by operator, returns + * an opendal_result_read with error code. + * + * @param ptr The opendal_operator_ptr created previously + * @param path The path you want to read the data out + * @param buffer The buffer you want to read the data into + * @param buffer_len The length of the buffer + * @see opendal_operator_ptr + * @see opendal_result_read + * @see opendal_code + * @return Returns opendal_code + * + * \note If the read operation succeeds, the returned opendal_bytes is newly allocated on heap. + * After your usage of that, please call opendal_bytes_free() to free the space. + * + * # Example + * + * Following is an example + * ```C + * // ... you have write "Hello, World!" to path "/testpath" + * + * int length = 13; + * unsigned char buffer[length]; + * opendal_code r = opendal_operator_blocking_read_with_buffer(ptr, "testpath", buffer, length); + * assert(r == OPENDAL_OK); + * // assert buffer == "Hello, World!" + * + * ``` + * + * # Safety + * + * It is **safe** under the cases below + * * The memory pointed to by `path` must contain a valid nul terminator at the end of + * the string. + * + * # Panic + * + * * If the `path` points to NULL, this function panics, i.e. exits with information + */ +enum opendal_code opendal_operator_blocking_read_with_buffer(const struct opendal_operator_ptr *ptr, + const char *path, + uint8_t *buffer, + uintptr_t buffer_len); + /** * \brief Blockingly delete the object in `path`. * diff --git a/bindings/c/src/lib.rs b/bindings/c/src/lib.rs index 013e2a307359..21d28c9cde05 100644 --- a/bindings/c/src/lib.rs +++ b/bindings/c/src/lib.rs @@ -29,7 +29,9 @@ mod error; mod result; mod types; +use core::slice; use std::collections::HashMap; +use std::io::Read; use std::os::raw::c_char; use std::str::FromStr; @@ -274,6 +276,73 @@ pub unsafe extern "C" fn opendal_operator_blocking_read( } } +/// \brief Blockingly read the data from `path`. +/// +/// Read the data out from `path` blockingly by operator, returns +/// an opendal_result_read with error code. +/// +/// @param ptr The opendal_operator_ptr created previously +/// @param path The path you want to read the data out +/// @param buffer The buffer you want to read the data into +/// @param buffer_len The length of the buffer +/// @see opendal_operator_ptr +/// @see opendal_result_read +/// @see opendal_code +/// @return Returns opendal_code +/// +/// \note If the read operation succeeds, the returned opendal_bytes is newly allocated on heap. +/// After your usage of that, please call opendal_bytes_free() to free the space. +/// +/// # Example +/// +/// Following is an example +/// ```C +/// // ... you have write "Hello, World!" to path "/testpath" +/// +/// int length = 13; +/// unsigned char buffer[length]; +/// opendal_code r = opendal_operator_blocking_read_with_buffer(ptr, "testpath", buffer, length); +/// assert(r == OPENDAL_OK); +/// // assert buffer == "Hello, World!" +/// +/// ``` +/// +/// # Safety +/// +/// It is **safe** under the cases below +/// * The memory pointed to by `path` must contain a valid nul terminator at the end of +/// the string. +/// +/// # Panic +/// +/// * If the `path` points to NULL, this function panics, i.e. exits with information +#[no_mangle] +pub unsafe extern "C" fn opendal_operator_blocking_read_with_buffer( + ptr: *const opendal_operator_ptr, + path: *const c_char, + buffer: *mut u8, + buffer_len: usize, +) -> opendal_code { + if path.is_null() { + panic!("The path given is pointing at NULL"); + } + if buffer.is_null() { + panic!("The buffer given is pointing at NULL"); + } + let op = (*ptr).as_ref(); + let path = unsafe { std::ffi::CStr::from_ptr(path).to_str().unwrap() }; + match op.reader(path) { + Ok(mut reader) => { + let mut buf = unsafe { slice::from_raw_parts_mut(buffer, buffer_len) }; + match reader.read(&mut buf) { + Ok(_) => opendal_code::OPENDAL_OK, + Err(_) => opendal_code::OPENDAL_UNEXPECTED, + } + } + Err(e) => opendal_code::from_opendal_error(e), + } +} + /// \brief Blockingly delete the object in `path`. /// /// Delete the object in `path` blockingly by `op_ptr`. diff --git a/bindings/c/tests/bdd.cpp b/bindings/c/tests/bdd.cpp index 76d5047faf0a..b4970a67c06c 100644 --- a/bindings/c/tests/bdd.cpp +++ b/bindings/c/tests/bdd.cpp @@ -87,6 +87,16 @@ TEST_F(OpendalBddTest, FeatureTest) EXPECT_EQ(this->content[i], (char)(r.data->data[i])); } + // The blocking file "test" must have content "Hello, World!" and read into buffer + int length = this->content.length(); + unsigned char buffer[this->content.length()]; + code = opendal_operator_blocking_read_with_buffer(this->p, this->path.c_str(), + buffer, length); + EXPECT_EQ(code, OPENDAL_OK); + for (int i = 0; i < this->content.length(); i++) { + EXPECT_EQ(this->content[i], buffer[i]); + } + // The blocking file should be deleted error = opendal_operator_blocking_delete(this->p, this->path.c_str()); EXPECT_EQ(error, nullptr); From 05ce70500d16aba7b472b7a06e7ee66cfc1c8180 Mon Sep 17 00:00:00 2001 From: Enwei Jiao Date: Wed, 11 Oct 2023 15:11:38 -0700 Subject: [PATCH 2/4] Fix clippy warning --- bindings/c/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bindings/c/src/lib.rs b/bindings/c/src/lib.rs index 21d28c9cde05..36ec4654141b 100644 --- a/bindings/c/src/lib.rs +++ b/bindings/c/src/lib.rs @@ -333,8 +333,8 @@ pub unsafe extern "C" fn opendal_operator_blocking_read_with_buffer( let path = unsafe { std::ffi::CStr::from_ptr(path).to_str().unwrap() }; match op.reader(path) { Ok(mut reader) => { - let mut buf = unsafe { slice::from_raw_parts_mut(buffer, buffer_len) }; - match reader.read(&mut buf) { + let buf = unsafe { slice::from_raw_parts_mut(buffer, buffer_len) }; + match reader.read(buf) { Ok(_) => opendal_code::OPENDAL_OK, Err(_) => opendal_code::OPENDAL_UNEXPECTED, } From 19f1cf16cf88fdd61d7d829be12065f6c8869292 Mon Sep 17 00:00:00 2001 From: Enwei Jiao Date: Thu, 12 Oct 2023 11:36:29 -0700 Subject: [PATCH 3/4] Add opendal_operator_blocking_reader --- bindings/c/include/opendal.h | 29 +++++++++++++++++++++++++---- bindings/c/src/lib.rs | 29 ++++++++++++----------------- bindings/c/src/result.rs | 13 +++++++++++++ bindings/c/src/types.rs | 27 +++++++++++++++++++++++++++ bindings/c/tests/bdd.cpp | 7 ++++--- 5 files changed, 81 insertions(+), 24 deletions(-) diff --git a/bindings/c/include/opendal.h b/bindings/c/include/opendal.h index e76558c3a110..b1557d6ac3d0 100644 --- a/bindings/c/include/opendal.h +++ b/bindings/c/include/opendal.h @@ -144,6 +144,12 @@ typedef struct BlockingLister BlockingLister; */ typedef struct BlockingOperator BlockingOperator; +/** + * BlockingReader is designed to read data from given path in an blocking + * manner. + */ +typedef struct BlockingReader BlockingReader; + /** * Entry returned by [`Lister`] or [`BlockingLister`] to represent a path and it's relative metadata. * @@ -320,6 +326,21 @@ typedef struct opendal_result_read { struct opendal_error *error; } opendal_result_read; +typedef struct opendal_reader { + struct BlockingReader *ptr; +} opendal_reader; + +/** + * \brief The result type returned by opendal_operator_reader(). + * The result type for opendal_operator_reader(), the field `reader` contains the reader + * of the path, which is an iterator of the objects under the path. the field `code` represents + * whether the stat operation is successful. + */ +typedef struct opendal_result_reader { + struct opendal_reader *reader; + enum opendal_code code; +} opendal_result_reader; + /** * \brief The result type returned by opendal_operator_is_exist(). * @@ -602,10 +623,8 @@ struct opendal_result_read opendal_operator_blocking_read(const struct opendal_o * * * If the `path` points to NULL, this function panics, i.e. exits with information */ -enum opendal_code opendal_operator_blocking_read_with_buffer(const struct opendal_operator_ptr *ptr, - const char *path, - uint8_t *buffer, - uintptr_t buffer_len); +struct opendal_result_reader opendal_operator_blocking_reader(const struct opendal_operator_ptr *ptr, + const char *path); /** * \brief Blockingly delete the object in `path`. @@ -951,6 +970,8 @@ char *opendal_list_entry_name(const struct opendal_list_entry *self); */ void opendal_list_entry_free(struct opendal_list_entry *ptr); +intptr_t opendal_reader_read(const struct opendal_reader *self, uint8_t *buf, uintptr_t len); + #ifdef __cplusplus } // extern "C" #endif // __cplusplus diff --git a/bindings/c/src/lib.rs b/bindings/c/src/lib.rs index 36ec4654141b..4fe3483f964c 100644 --- a/bindings/c/src/lib.rs +++ b/bindings/c/src/lib.rs @@ -29,9 +29,7 @@ mod error; mod result; mod types; -use core::slice; use std::collections::HashMap; -use std::io::Read; use std::os::raw::c_char; use std::str::FromStr; @@ -39,7 +37,9 @@ use ::opendal as od; use error::opendal_error; use result::opendal_result_list; use result::opendal_result_operator_new; +use result::opendal_result_reader; use types::opendal_blocking_lister; +use types::opendal_reader; use crate::result::opendal_result_is_exist; use crate::result::opendal_result_read; @@ -317,29 +317,24 @@ pub unsafe extern "C" fn opendal_operator_blocking_read( /// /// * If the `path` points to NULL, this function panics, i.e. exits with information #[no_mangle] -pub unsafe extern "C" fn opendal_operator_blocking_read_with_buffer( +pub unsafe extern "C" fn opendal_operator_blocking_reader( ptr: *const opendal_operator_ptr, path: *const c_char, - buffer: *mut u8, - buffer_len: usize, -) -> opendal_code { +) -> opendal_result_reader { if path.is_null() { panic!("The path given is pointing at NULL"); } - if buffer.is_null() { - panic!("The buffer given is pointing at NULL"); - } let op = (*ptr).as_ref(); let path = unsafe { std::ffi::CStr::from_ptr(path).to_str().unwrap() }; match op.reader(path) { - Ok(mut reader) => { - let buf = unsafe { slice::from_raw_parts_mut(buffer, buffer_len) }; - match reader.read(buf) { - Ok(_) => opendal_code::OPENDAL_OK, - Err(_) => opendal_code::OPENDAL_UNEXPECTED, - } - } - Err(e) => opendal_code::from_opendal_error(e), + Ok(reader) => opendal_result_reader { + reader: Box::into_raw(Box::new(opendal_reader::new(reader))), + code: opendal_code::OPENDAL_OK, + }, + Err(e) => opendal_result_reader { + reader: std::ptr::null_mut(), + code: opendal_code::from_opendal_error(e), + }, } } diff --git a/bindings/c/src/result.rs b/bindings/c/src/result.rs index aa95c1190b01..101488df64d6 100644 --- a/bindings/c/src/result.rs +++ b/bindings/c/src/result.rs @@ -41,6 +41,9 @@ pub struct opendal_result_operator_new { pub operator_ptr: *mut opendal_operator_ptr, pub error: *mut opendal_error, } +======= +use crate::types::opendal_reader; +>>>>>>> 86ecc6f48 (Add opendal_operator_blocking_reader) /// \brief The result type returned by opendal's read operation. /// @@ -97,3 +100,13 @@ pub struct opendal_result_list { /// The error, if ok, it is null pub error: *mut opendal_error, } + +/// \brief The result type returned by opendal_operator_reader(). +/// The result type for opendal_operator_reader(), the field `reader` contains the reader +/// of the path, which is an iterator of the objects under the path. the field `code` represents +/// whether the stat operation is successful. +#[repr(C)] +pub struct opendal_result_reader { + pub reader: *mut opendal_reader, + pub code: opendal_code, +} diff --git a/bindings/c/src/types.rs b/bindings/c/src/types.rs index dbc0aa68ee9d..323fa88e3389 100644 --- a/bindings/c/src/types.rs +++ b/bindings/c/src/types.rs @@ -17,6 +17,7 @@ use std::collections::HashMap; use std::ffi::CString; +use std::io::Read; use std::os::raw::c_char; use ::opendal as od; @@ -408,3 +409,29 @@ impl opendal_list_entry { } } } + +#[repr(C)] +pub struct opendal_reader { + ptr: *mut od::BlockingReader, +} + +impl opendal_reader { + pub(crate) fn new(reader: od::BlockingReader) -> Self { + Self { + ptr: Box::into_raw(Box::new(reader)), + } + } + + #[no_mangle] + pub unsafe extern "C" fn opendal_reader_read(&self, buf: *mut u8, len: usize) -> isize { + if buf.is_null() { + panic!("The buffer given is pointing at NULL"); + } + let buf = unsafe { std::slice::from_raw_parts_mut(buf, len) }; + let r = (*self.ptr).read(buf); + match r { + Ok(n) => n as isize, + Err(_) => -1, + } + } +} diff --git a/bindings/c/tests/bdd.cpp b/bindings/c/tests/bdd.cpp index b4970a67c06c..07e91de5cbd9 100644 --- a/bindings/c/tests/bdd.cpp +++ b/bindings/c/tests/bdd.cpp @@ -90,9 +90,10 @@ TEST_F(OpendalBddTest, FeatureTest) // The blocking file "test" must have content "Hello, World!" and read into buffer int length = this->content.length(); unsigned char buffer[this->content.length()]; - code = opendal_operator_blocking_read_with_buffer(this->p, this->path.c_str(), - buffer, length); - EXPECT_EQ(code, OPENDAL_OK); + opendal_result_reader reader = opendal_operator_blocking_reader(this->p, this->path.c_str()); + EXPECT_EQ(reader.code, OPENDAL_OK); + long size = opendal_reader_read(reader.reader, buffer, length); + EXPECT_EQ(size, length); for (int i = 0; i < this->content.length(); i++) { EXPECT_EQ(this->content[i], buffer[i]); } From 4317cce7b5fa90777a85750e069ff38d55a85171 Mon Sep 17 00:00:00 2001 From: Enwei Jiao Date: Thu, 12 Oct 2023 11:47:25 -0700 Subject: [PATCH 4/4] Fix memory leak --- bindings/c/include/opendal.h | 15 +++++++++--- bindings/c/src/lib.rs | 13 +++++++---- bindings/c/src/result.rs | 6 ++--- bindings/c/src/types.rs | 45 +++++++++++++++++++++++++++++++----- bindings/c/tests/bdd.cpp | 7 +++--- 5 files changed, 65 insertions(+), 21 deletions(-) diff --git a/bindings/c/include/opendal.h b/bindings/c/include/opendal.h index b1557d6ac3d0..044a451b5fa9 100644 --- a/bindings/c/include/opendal.h +++ b/bindings/c/include/opendal.h @@ -327,7 +327,7 @@ typedef struct opendal_result_read { } opendal_result_read; typedef struct opendal_reader { - struct BlockingReader *ptr; + struct BlockingReader *inner; } opendal_reader; /** @@ -338,7 +338,7 @@ typedef struct opendal_reader { */ typedef struct opendal_result_reader { struct opendal_reader *reader; - enum opendal_code code; + struct opendal_error *error; } opendal_result_reader; /** @@ -442,6 +442,11 @@ typedef struct opendal_list_entry { struct Entry *inner; } opendal_list_entry; +typedef struct opendal_result_reader_read { + uintptr_t size; + struct opendal_error *error; +} opendal_result_reader_read; + #ifdef __cplusplus extern "C" { #endif // __cplusplus @@ -970,7 +975,11 @@ char *opendal_list_entry_name(const struct opendal_list_entry *self); */ void opendal_list_entry_free(struct opendal_list_entry *ptr); -intptr_t opendal_reader_read(const struct opendal_reader *self, uint8_t *buf, uintptr_t len); +struct opendal_result_reader_read opendal_reader_read(const struct opendal_reader *self, + uint8_t *buf, + uintptr_t len); + +void opendal_reader_free(struct opendal_reader *ptr); #ifdef __cplusplus } // extern "C" diff --git a/bindings/c/src/lib.rs b/bindings/c/src/lib.rs index 4fe3483f964c..91dc253bea2c 100644 --- a/bindings/c/src/lib.rs +++ b/bindings/c/src/lib.rs @@ -329,12 +329,15 @@ pub unsafe extern "C" fn opendal_operator_blocking_reader( match op.reader(path) { Ok(reader) => opendal_result_reader { reader: Box::into_raw(Box::new(opendal_reader::new(reader))), - code: opendal_code::OPENDAL_OK, - }, - Err(e) => opendal_result_reader { - reader: std::ptr::null_mut(), - code: opendal_code::from_opendal_error(e), + error: std::ptr::null_mut(), }, + Err(e) => { + let e = Box::new(opendal_error::from_opendal_error(e)); + opendal_result_reader { + reader: std::ptr::null_mut(), + error: Box::into_raw(e), + } + } } } diff --git a/bindings/c/src/result.rs b/bindings/c/src/result.rs index 101488df64d6..610e6d43e5ea 100644 --- a/bindings/c/src/result.rs +++ b/bindings/c/src/result.rs @@ -25,6 +25,7 @@ use crate::types::opendal_blocking_lister; use crate::types::opendal_bytes; use crate::types::opendal_metadata; use crate::types::opendal_operator_ptr; +use crate::types::opendal_reader; /// \brief The result type returned by opendal_operator_new() operation. /// @@ -41,9 +42,6 @@ pub struct opendal_result_operator_new { pub operator_ptr: *mut opendal_operator_ptr, pub error: *mut opendal_error, } -======= -use crate::types::opendal_reader; ->>>>>>> 86ecc6f48 (Add opendal_operator_blocking_reader) /// \brief The result type returned by opendal's read operation. /// @@ -108,5 +106,5 @@ pub struct opendal_result_list { #[repr(C)] pub struct opendal_result_reader { pub reader: *mut opendal_reader, - pub code: opendal_code, + pub error: *mut opendal_error, } diff --git a/bindings/c/src/types.rs b/bindings/c/src/types.rs index 323fa88e3389..19c9365ca220 100644 --- a/bindings/c/src/types.rs +++ b/bindings/c/src/types.rs @@ -22,6 +22,9 @@ use std::os::raw::c_char; use ::opendal as od; +use crate::error::opendal_code; +use crate::error::opendal_error; + /// \brief Used to access almost all OpenDAL APIs. It represents a /// operator that provides the unified interfaces provided by OpenDAL. /// @@ -412,26 +415,56 @@ impl opendal_list_entry { #[repr(C)] pub struct opendal_reader { - ptr: *mut od::BlockingReader, + inner: *mut od::BlockingReader, +} + +#[repr(C)] +pub struct opendal_result_reader_read { + pub size: usize, + pub error: *mut opendal_error, } impl opendal_reader { pub(crate) fn new(reader: od::BlockingReader) -> Self { Self { - ptr: Box::into_raw(Box::new(reader)), + inner: Box::into_raw(Box::new(reader)), } } #[no_mangle] - pub unsafe extern "C" fn opendal_reader_read(&self, buf: *mut u8, len: usize) -> isize { + pub unsafe extern "C" fn opendal_reader_read( + &self, + buf: *mut u8, + len: usize, + ) -> opendal_result_reader_read { if buf.is_null() { panic!("The buffer given is pointing at NULL"); } let buf = unsafe { std::slice::from_raw_parts_mut(buf, len) }; - let r = (*self.ptr).read(buf); + let r = (*self.inner).read(buf); match r { - Ok(n) => n as isize, - Err(_) => -1, + Ok(n) => opendal_result_reader_read { + size: n, + error: std::ptr::null_mut(), + }, + Err(e) => { + let e = Box::new(opendal_error::manual_error( + opendal_code::OPENDAL_UNEXPECTED, + e.to_string(), + )); + opendal_result_reader_read { + size: 0, + error: Box::into_raw(e), + } + } + } + } + + #[no_mangle] + pub unsafe extern "C" fn opendal_reader_free(ptr: *mut opendal_reader) { + if !ptr.is_null() { + let _ = unsafe { Box::from_raw((*ptr).inner) }; + let _ = unsafe { Box::from_raw(ptr) }; } } } diff --git a/bindings/c/tests/bdd.cpp b/bindings/c/tests/bdd.cpp index 07e91de5cbd9..e5a94d2afd2b 100644 --- a/bindings/c/tests/bdd.cpp +++ b/bindings/c/tests/bdd.cpp @@ -91,12 +91,13 @@ TEST_F(OpendalBddTest, FeatureTest) int length = this->content.length(); unsigned char buffer[this->content.length()]; opendal_result_reader reader = opendal_operator_blocking_reader(this->p, this->path.c_str()); - EXPECT_EQ(reader.code, OPENDAL_OK); - long size = opendal_reader_read(reader.reader, buffer, length); - EXPECT_EQ(size, length); + EXPECT_EQ(reader.error, nullptr); + auto rst = opendal_reader_read(reader.reader, buffer, length); + EXPECT_EQ(rst.size, length); for (int i = 0; i < this->content.length(); i++) { EXPECT_EQ(this->content[i], buffer[i]); } + opendal_reader_free(reader.reader); // The blocking file should be deleted error = opendal_operator_blocking_delete(this->p, this->path.c_str());