From 96f000eeb3e18cfd0e927464a1079b26311aaa61 Mon Sep 17 00:00:00 2001 From: Ji-Xinyou Date: Mon, 9 Oct 2023 12:11:26 -0400 Subject: [PATCH 01/19] Implement opendal_error --- bindings/c/include/opendal.h | 43 +++++++++++---------- bindings/c/src/error.rs | 38 +++++++++++-------- bindings/c/src/lib.rs | 72 ++++++++++++++++++++++-------------- bindings/c/src/result.rs | 18 +++++---- 4 files changed, 101 insertions(+), 70 deletions(-) diff --git a/bindings/c/include/opendal.h b/bindings/c/include/opendal.h index bc47bf5b541f..1822a00ce07d 100644 --- a/bindings/c/include/opendal.h +++ b/bindings/c/include/opendal.h @@ -31,14 +31,6 @@ * added in the future. */ typedef enum opendal_code { - /** - * All is well - */ - OPENDAL_OK, - /** - * General error - */ - OPENDAL_ERROR, /** * returning it back. For example, s3 returns an internal service error. */ @@ -237,6 +229,11 @@ typedef struct opendal_bytes { uintptr_t len; } opendal_bytes; +typedef struct opendal_error { + enum opendal_code code; + struct opendal_bytes message; +} opendal_error; + /** * \brief The result type returned by opendal's read operation. * @@ -251,9 +248,9 @@ typedef struct opendal_result_read { */ struct opendal_bytes *data; /** - * The error code, should be OPENDAL_OK if succeeds + * The error, if ok, it is null */ - enum opendal_code code; + struct opendal_error *error; } opendal_result_read; /** @@ -272,9 +269,9 @@ typedef struct opendal_result_is_exist { */ bool is_exist; /** - * The error code, should be OPENDAL_OK if succeeds + * The error, if ok, it is null */ - enum opendal_code code; + struct opendal_error *error; } opendal_result_is_exist; /** @@ -308,9 +305,9 @@ typedef struct opendal_result_stat { */ struct opendal_metadata *meta; /** - * The error code, should be OPENDAL_OK if succeeds + * The error, if ok, it is null */ - enum opendal_code code; + struct opendal_error *error; } opendal_result_stat; /** @@ -334,8 +331,14 @@ typedef struct opendal_blocking_lister { * whether the stat operation is successful. */ typedef struct opendal_result_list { + /** + * The lister, used for further listing operations + */ struct opendal_blocking_lister *lister; - enum opendal_code code; + /** + * The error, if ok, it is null + */ + struct opendal_error *error; } opendal_result_list; /** @@ -445,9 +448,9 @@ const struct opendal_operator_ptr *opendal_operator_new(const char *scheme, * * * If the `path` points to NULL, this function panics, i.e. exits with information */ -enum opendal_code opendal_operator_blocking_write(const struct opendal_operator_ptr *ptr, - const char *path, - struct opendal_bytes bytes); +struct opendal_error *opendal_operator_blocking_write(const struct opendal_operator_ptr *ptr, + const char *path, + struct opendal_bytes bytes); /** * \brief Blockingly read the data from `path`. @@ -533,8 +536,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_delete(const struct opendal_operator_ptr *ptr, - const char *path); +struct opendal_error *opendal_operator_blocking_delete(const struct opendal_operator_ptr *ptr, + const char *path); /** * \brief Check whether the path exists. diff --git a/bindings/c/src/error.rs b/bindings/c/src/error.rs index 8aa8c87ca497..ac59fcda6f9e 100644 --- a/bindings/c/src/error.rs +++ b/bindings/c/src/error.rs @@ -17,20 +17,17 @@ use ::opendal as od; +use crate::types::opendal_bytes; + /// The wrapper type for opendal's error, wrapped because of the /// orphan rule -struct opendal_error(od::Error); +struct opendal_internal_error(od::Error); /// \brief The error code for all opendal APIs in C binding. /// \todo The error handling is not complete, the error with error message will be /// added in the future. #[repr(C)] -pub enum opendal_code { - /// All is well - OPENDAL_OK, - /// General error - // \todo: make details in the `opendal_error *` - OPENDAL_ERROR, +pub(crate) enum opendal_code { /// returning it back. For example, s3 returns an internal service error. OPENDAL_UNEXPECTED, /// Underlying service doesn't support this operation. @@ -53,14 +50,7 @@ pub enum opendal_code { OPENDAL_IS_SAME_FILE, } -impl opendal_code { - pub(crate) fn from_opendal_error(e: od::Error) -> Self { - let error = opendal_error(e); - error.error_code() - } -} - -impl opendal_error { +impl opendal_internal_error { /// Convert the [`od::ErrorKind`] of [`od::Error`] to [`opendal_code`] pub(crate) fn error_code(&self) -> opendal_code { let e = &self.0; @@ -81,3 +71,21 @@ impl opendal_error { } } } + +#[repr(C)] +pub struct opendal_error { + code: opendal_code, + message: opendal_bytes, +} + +impl opendal_error { + // The caller should sink the error to heap memory and return the pointer + // that will not be freed by rustc + pub(crate) fn from_opendal_error(error: od::Error) -> Self { + let error = opendal_internal_error(error); + let code = error.error_code(); + let c_str = format!("{}", error.0); + let message = opendal_bytes::new(c_str.into_bytes()); + opendal_error { code, message } + } +} diff --git a/bindings/c/src/lib.rs b/bindings/c/src/lib.rs index 6535b796573b..c2844dde76d6 100644 --- a/bindings/c/src/lib.rs +++ b/bindings/c/src/lib.rs @@ -34,10 +34,10 @@ use std::os::raw::c_char; use std::str::FromStr; use ::opendal as od; +use error::opendal_error; use result::opendal_result_list; use types::opendal_blocking_lister; -use crate::error::opendal_code; use crate::result::opendal_result_is_exist; use crate::result::opendal_result_read; use crate::result::opendal_result_stat; @@ -172,7 +172,7 @@ pub unsafe extern "C" fn opendal_operator_blocking_write( ptr: *const opendal_operator_ptr, path: *const c_char, bytes: opendal_bytes, -) -> opendal_code { +) -> *mut opendal_error { if path.is_null() { panic!("The path given is pointing at NULL"); } @@ -180,8 +180,11 @@ pub unsafe extern "C" fn opendal_operator_blocking_write( let op = (*ptr).as_ref(); let path = unsafe { std::ffi::CStr::from_ptr(path).to_str().unwrap() }; match op.write(path, bytes) { - Ok(_) => opendal_code::OPENDAL_OK, - Err(e) => opendal_code::from_opendal_error(e), + Ok(_) => std::ptr::null_mut(), + Err(e) => { + let e = Box::new(opendal_error::from_opendal_error(e)); + Box::into_raw(e) + } } } @@ -241,13 +244,16 @@ pub unsafe extern "C" fn opendal_operator_blocking_read( let v = Box::new(opendal_bytes::new(d)); opendal_result_read { data: Box::into_raw(v), - code: opendal_code::OPENDAL_OK, + error: std::ptr::null_mut(), + } + } + Err(e) => { + let e = Box::new(opendal_error::from_opendal_error(e)); + opendal_result_read { + data: std::ptr::null_mut(), + error: Box::into_raw(e), } } - Err(e) => opendal_result_read { - data: std::ptr::null_mut(), - code: opendal_code::from_opendal_error(e), - }, } } @@ -293,7 +299,7 @@ pub unsafe extern "C" fn opendal_operator_blocking_read( pub unsafe extern "C" fn opendal_operator_blocking_delete( ptr: *const opendal_operator_ptr, path: *const c_char, -) -> opendal_code { +) -> *mut opendal_error { if path.is_null() { panic!("The path given is pointing at NULL"); } @@ -301,8 +307,11 @@ pub unsafe extern "C" fn opendal_operator_blocking_delete( let op = (*ptr).as_ref(); let path = unsafe { std::ffi::CStr::from_ptr(path).to_str().unwrap() }; match op.delete(path) { - Ok(_) => opendal_code::OPENDAL_OK, - Err(e) => opendal_code::from_opendal_error(e), + Ok(_) => std::ptr::null_mut(), + Err(e) => { + let e = Box::new(opendal_error::from_opendal_error(e)); + Box::into_raw(e) + } } } @@ -359,12 +368,15 @@ pub unsafe extern "C" fn opendal_operator_is_exist( match op.is_exist(path) { Ok(e) => opendal_result_is_exist { is_exist: e, - code: opendal_code::OPENDAL_OK, - }, - Err(err) => opendal_result_is_exist { - is_exist: false, - code: opendal_code::from_opendal_error(err), + error: std::ptr::null_mut(), }, + Err(e) => { + let e = Box::new(opendal_error::from_opendal_error(e)); + opendal_result_is_exist { + is_exist: false, + error: Box::into_raw(e), + } + } } } @@ -420,12 +432,15 @@ pub unsafe extern "C" fn opendal_operator_stat( match op.stat(path) { Ok(m) => opendal_result_stat { meta: Box::into_raw(Box::new(opendal_metadata::new(m))), - code: opendal_code::OPENDAL_OK, - }, - Err(err) => opendal_result_stat { - meta: std::ptr::null_mut(), - code: opendal_code::from_opendal_error(err), + error: std::ptr::null_mut(), }, + Err(e) => { + let e = Box::new(opendal_error::from_opendal_error(e)); + opendal_result_stat { + meta: std::ptr::null_mut(), + error: Box::into_raw(e), + } + } } } @@ -488,12 +503,15 @@ pub unsafe extern "C" fn opendal_operator_blocking_list( match op.lister(path) { Ok(lister) => opendal_result_list { lister: Box::into_raw(Box::new(opendal_blocking_lister::new(lister))), - code: opendal_code::OPENDAL_OK, + error: std::ptr::null_mut(), }, - Err(e) => opendal_result_list { - lister: std::ptr::null_mut(), - code: opendal_code::from_opendal_error(e), - }, + Err(e) => { + let e = Box::new(opendal_error::from_opendal_error(e)); + opendal_result_list { + lister: std::ptr::null_mut(), + error: Box::into_raw(e), + } + } } } diff --git a/bindings/c/src/result.rs b/bindings/c/src/result.rs index c0d113718294..8ac76d6666cb 100644 --- a/bindings/c/src/result.rs +++ b/bindings/c/src/result.rs @@ -20,7 +20,7 @@ //! "opendal_result_opendal_operator_ptr", which is unacceptable. Therefore, //! we are defining all Result types here -use crate::error::opendal_code; +use crate::error::opendal_error; use crate::types::opendal_blocking_lister; use crate::types::opendal_bytes; use crate::types::opendal_metadata; @@ -35,8 +35,8 @@ use crate::types::opendal_metadata; pub struct opendal_result_read { /// The byte array with length returned by read operations pub data: *mut opendal_bytes, - /// The error code, should be OPENDAL_OK if succeeds - pub code: opendal_code, + /// The error, if ok, it is null + pub error: *mut opendal_error, } /// \brief The result type returned by opendal_operator_is_exist(). @@ -51,8 +51,8 @@ pub struct opendal_result_read { pub struct opendal_result_is_exist { /// Whether the path exists pub is_exist: bool, - /// The error code, should be OPENDAL_OK if succeeds - pub code: opendal_code, + /// The error, if ok, it is null + pub error: *mut opendal_error, } /// \brief The result type returned by opendal_operator_stat(). @@ -63,8 +63,8 @@ pub struct opendal_result_is_exist { pub struct opendal_result_stat { /// The metadata output of the stat pub meta: *mut opendal_metadata, - /// The error code, should be OPENDAL_OK if succeeds - pub code: opendal_code, + /// The error, if ok, it is null + pub error: *mut opendal_error, } /// \brief The result type returned by opendal_operator_blocking_list(). @@ -74,6 +74,8 @@ pub struct opendal_result_stat { /// whether the stat operation is successful. #[repr(C)] pub struct opendal_result_list { + /// The lister, used for further listing operations pub lister: *mut opendal_blocking_lister, - pub code: opendal_code, + /// The error, if ok, it is null + pub error: *mut opendal_error, } From 38be0de95340f56df1d2d20c35caf8b6405a01b3 Mon Sep 17 00:00:00 2001 From: Ji-Xinyou Date: Mon, 9 Oct 2023 14:41:14 -0400 Subject: [PATCH 02/19] docs --- bindings/c/README.md | 6 +-- bindings/c/include/opendal.h | 97 ++++++++++++++++++------------------ bindings/c/src/lib.rs | 76 ++++++++++++++-------------- bindings/c/src/result.rs | 15 +++--- bindings/c/src/types.rs | 6 +-- 5 files changed, 101 insertions(+), 99 deletions(-) diff --git a/bindings/c/README.md b/bindings/c/README.md index 7d57c27fc22f..f89cead26666 100644 --- a/bindings/c/README.md +++ b/bindings/c/README.md @@ -24,13 +24,13 @@ int main() }; /* Write this into path "/testpath" */ - opendal_code code = opendal_operator_blocking_write(op, "/testpath", data); - assert(code == OPENDAL_OK); + opendal_error *error = opendal_operator_blocking_write(op, "/testpath", data); + assert(error == NULL); /* We can read it out, make sure the data is the same */ opendal_result_read r = opendal_operator_blocking_read(op, "/testpath"); opendal_bytes* read_bytes = r.data; - assert(r.code == OPENDAL_OK); + assert(r.error == NULL); assert(read_bytes->len == 24); /* Lets print it out */ diff --git a/bindings/c/include/opendal.h b/bindings/c/include/opendal.h index 1822a00ce07d..a996a9576500 100644 --- a/bindings/c/include/opendal.h +++ b/bindings/c/include/opendal.h @@ -238,9 +238,9 @@ typedef struct opendal_error { * \brief The result type returned by opendal's read operation. * * The result type of read operation in opendal C binding, it contains - * the data that the read operation returns and a error code. + * the data that the read operation returns and an NULL error. * If the read operation failed, the `data` fields should be a nullptr - * and the error code is **NOT** OPENDAL_OK. + * and the error is not NULL. */ typedef struct opendal_result_read { /** @@ -257,8 +257,8 @@ typedef struct opendal_result_read { * \brief The result type returned by opendal_operator_is_exist(). * * The result type for opendal_operator_is_exist(), the field `is_exist` - * contains whether the path exists, and the field `code` contains the - * corresponding error code. + * contains whether the path exists, and the field `error` contains the + * corresponding error. If successful, the `error` field is null. * * \note If the opendal_operator_is_exist() fails, the `is_exist` field * will be set to false. @@ -297,7 +297,8 @@ typedef struct opendal_metadata { * \brief The result type returned by opendal_operator_stat(). * * The result type for opendal_operator_stat(), the field `meta` contains the metadata - * of the path, the field `code` represents whether the stat operation is successful. + * of the path, the field `error` represents whether the stat operation is successful. + * If successful, the `error` field is null. */ typedef struct opendal_result_stat { /** @@ -327,8 +328,8 @@ typedef struct opendal_blocking_lister { * \brief The result type returned by opendal_operator_blocking_list(). * * The result type for opendal_operator_blocking_list(), the field `lister` contains the lister - * of the path, which is an iterator of the objects under the path. the field `code` represents - * whether the stat operation is successful. + * of the path, which is an iterator of the objects under the path. the field `error` represents + * whether the stat operation is successful. If successful, the `error` field is null. */ typedef struct opendal_result_list { /** @@ -405,19 +406,19 @@ const struct opendal_operator_ptr *opendal_operator_new(const char *scheme, /** * \brief Blockingly write raw bytes to `path`. * - * Write the `bytes` into the `path` blockingly by `op_ptr`, returns the opendal_code OPENDAL_OK - * if succeeds, others otherwise. + * Write the `bytes` into the `path` blockingly by `op_ptr`. + * Error is NULL if successful, otherwise it contains the error code and error message. * * @NOTE It is important to notice that the `bytes` that is passes in will be consumed by this - * function. + * function. Therefore, you should not use the `bytes` after this function returns. * * @param ptr The opendal_operator_ptr created previously * @param path The designated path you want to write your bytes in * @param bytes The opendal_byte typed bytes to be written * @see opendal_operator_ptr * @see opendal_bytes - * @see opendal_code - * @return OPENDAL_OK if succeeds others otherwise + * @see opendal_error + * @return NULL if succeeds, otherwise it contains the error code and error message. * * # Example * @@ -430,10 +431,10 @@ const struct opendal_operator_ptr *opendal_operator_new(const char *scheme, * opendal_bytes bytes = opendal_bytes { .data = (uint8_t*)data, .len = 13 }; * * // now you can write! - * opendal_code code = opendal_operator_blocking_write(ptr, "/testpath", bytes); + * opendal_error *err = opendal_operator_blocking_write(ptr, "/testpath", bytes); * * // Assert that this succeeds - * assert(code == OPENDAL_OK) + * assert(err == NULL); * ``` * * # Safety @@ -455,17 +456,16 @@ struct opendal_error *opendal_operator_blocking_write(const struct opendal_opera /** * \brief Blockingly read the data from `path`. * - * Read the data out from `path` blockingly by operator, returns - * an opendal_result_read with error code. + * Read the data out from `path` blockingly by operator. * * @param ptr The opendal_operator_ptr created previously * @param path The path you want to read the data out * @see opendal_operator_ptr * @see opendal_result_read - * @see opendal_code + * @see opendal_error * @return Returns opendal_result_read, the `data` field is a pointer to a newly allocated - * opendal_bytes, the `code` field contains the error code. If the `code` is not OPENDAL_OK, - * the `data` field points to NULL. + * opendal_bytes, the `error` field contains the error. If the `error` is not NULL, then + * the operation failed and the `data` field is a nullptr. * * \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. @@ -477,7 +477,7 @@ struct opendal_error *opendal_operator_blocking_write(const struct opendal_opera * // ... you have write "Hello, World!" to path "/testpath" * * opendal_result_read r = opendal_operator_blocking_read(ptr, "testpath"); - * assert(r.code == OPENDAL_OK); + * assert(r.error == NULL); * * opendal_bytes *bytes = r.data; * assert(bytes->len == 13); @@ -499,14 +499,14 @@ struct opendal_result_read opendal_operator_blocking_read(const struct opendal_o /** * \brief Blockingly delete the object in `path`. * - * Delete the object in `path` blockingly by `op_ptr`, returns the opendal_code OPENDAL_OK - * if succeeds, others otherwise + * Delete the object in `path` blockingly by `op_ptr`. + * Error is NULL if successful, otherwise it contains the error code and error message. * * @param ptr The opendal_operator_ptr created previously * @param path The designated path you want to delete * @see opendal_operator_ptr - * @see opendal_code - * @return OPENDAL_OK if succeeds others otherwise + * @see opendal_error + * @return NULL if succeeds, otherwise it contains the error code and error message. * * # Example * @@ -517,13 +517,15 @@ struct opendal_result_read opendal_operator_blocking_read(const struct opendal_o * // prepare your data * char* data = "Hello, World!"; * opendal_bytes bytes = opendal_bytes { .data = (uint8_t*)data, .len = 13 }; - * opendal_code code = opendal_operator_blocking_write(ptr, "/testpath", bytes); + * opendal_error *error = opendal_operator_blocking_write(ptr, "/testpath", bytes); + * + * assert(error == NULL); * * // now you can delete! - * opendal_code code = opendal_operator_blocking_delete(ptr, "/testpath"); + * opendal_error *error = opendal_operator_blocking_delete(ptr, "/testpath"); * * // Assert that this succeeds - * assert(code == OPENDAL_OK) + * assert(error == NULL); * ``` * * # Safety @@ -543,30 +545,28 @@ struct opendal_error *opendal_operator_blocking_delete(const struct opendal_oper * \brief Check whether the path exists. * * If the operation succeeds, no matter the path exists or not, - * the error code should be opendal_code::OPENDAL_OK. Otherwise, - * the field `is_exist` is filled with false, and the error code - * is set correspondingly. + * the error should be a nullptr. Otherwise, the field `is_exist` + * is filled with false, and the error is set * * @param ptr The opendal_operator_ptr created previously * @param path The path you want to check existence * @see opendal_operator_ptr * @see opendal_result_is_exist - * @see opendal_code + * @see opendal_error * @return Returns opendal_result_is_exist, the `is_exist` field contains whether the path exists. - * However, it the operation fails, the `is_exist` will contains false and the error code will be - * stored in the `code` field. + * However, it the operation fails, the `is_exist` will contains false and the error will be set. * * # Example * * ```C * // .. you previously wrote some data to path "/mytest/obj" * opendal_result_is_exist e = opendal_operator_is_exist(ptr, "/mytest/obj"); - * assert(e.code == OPENDAL_OK); + * assert(e.error == NULL); * assert(e.is_exist); * * // but you previously did **not** write any data to path "/yourtest/obj" - * opendal_result_is_exist e = opendal_operator_is_exist(ptr, "yourtest/obj"); - * assert(e.code == OPENDAL_OK); + * opendal_result_is_exist e = opendal_operator_is_exist(ptr, "/yourtest/obj"); + * assert(e.error == NULL); * assert(!e.is_exist); * ``` * @@ -586,26 +586,24 @@ struct opendal_result_is_exist opendal_operator_is_exist(const struct opendal_op /** * \brief Stat the path, return its metadata. * - * If the operation succeeds, the error code should be - * OPENDAL_OK. Otherwise, the field `meta` is filled with - * a NULL pointer, and the error code is set correspondingly. + * Error is NULL if successful, otherwise it contains the error code and error message. * * @param ptr The opendal_operator_ptr created previously * @param path The path you want to stat * @see opendal_operator_ptr * @see opendal_result_stat * @see opendal_metadata - * @return Returns opendal_result_stat, containing a metadata and a opendal_code. + * @return Returns opendal_result_stat, containing a metadata and an opendal_error. * If the operation succeeds, the `meta` field would holds a valid metadata and - * the `code` field should hold OPENDAL_OK. Otherwise the metadata will contain a - * NULL pointer, i.e. invalid, and the `code` will be set correspondingly. + * the `error` field should hold nullptr. Otherwise the metadata will contain a + * NULL pointer, i.e. invalid, and the `error` will be set correspondingly. * * # Example * * ```C * // ... previously you wrote "Hello, World!" to path "/testpath" * opendal_result_stat s = opendal_operator_stat(ptr, "/testpath"); - * assert(s.code == OPENDAL_OK); + * assert(s.error == NULL); * * const opendal_metadata *meta = s.meta; * @@ -636,7 +634,10 @@ struct opendal_result_stat opendal_operator_stat(const struct opendal_operator_p * @param ptr The opendal_operator_ptr created previously * @param path The designated path you want to delete * @see opendal_blocking_lister - * @return + * @return Returns opendal_result_list, containing a lister and an opendal_error. + * If the operation succeeds, the `lister` field would holds a valid lister and + * the `error` field should hold nullptr. Otherwise the `lister`` will contain a + * NULL pointer, i.e. invalid, and the `error` will be set correspondingly. * * # Example * @@ -645,7 +646,7 @@ struct opendal_result_stat opendal_operator_stat(const struct opendal_operator_p * // You have written some data into some files path "root/dir1" * // Your opendal_operator_ptr was called ptr * opendal_result_list l = opendal_operator_blocking_list(ptr, "root/dir1"); - * assert(l.code == OPENDAL_OK); + * assert(l.error == ERROR); * * opendal_blocking_lister *lister = l.lister; * opendal_list_entry *entry; @@ -712,7 +713,7 @@ void opendal_metadata_free(struct opendal_metadata *ptr); * ```C * // ... previously you wrote "Hello, World!" to path "/testpath" * opendal_result_stat s = opendal_operator_stat(ptr, "/testpath"); - * assert(s.code == OPENDAL_OK); + * assert(s.error == NULL); * * opendal_metadata *meta = s.meta; * assert(opendal_metadata_content_length(meta) == 13); @@ -727,7 +728,7 @@ uint64_t opendal_metadata_content_length(const struct opendal_metadata *self); * ```C * // ... previously you wrote "Hello, World!" to path "/testpath" * opendal_result_stat s = opendal_operator_stat(ptr, "/testpath"); - * assert(s.code == OPENDAL_OK); + * assert(s.error == NULL); * * opendal_metadata *meta = s.meta; * assert(opendal_metadata_is_file(meta)); @@ -742,7 +743,7 @@ bool opendal_metadata_is_file(const struct opendal_metadata *self); * ```C * // ... previously you wrote "Hello, World!" to path "/testpath" * opendal_result_stat s = opendal_operator_stat(ptr, "/testpath"); - * assert(s.code == OPENDAL_OK); + * assert(s.error == NULL); * * opendal_metadata *meta = s.meta; * diff --git a/bindings/c/src/lib.rs b/bindings/c/src/lib.rs index c2844dde76d6..0fc7329c8077 100644 --- a/bindings/c/src/lib.rs +++ b/bindings/c/src/lib.rs @@ -125,19 +125,19 @@ pub unsafe extern "C" fn opendal_operator_new( /// \brief Blockingly write raw bytes to `path`. /// -/// Write the `bytes` into the `path` blockingly by `op_ptr`, returns the opendal_code OPENDAL_OK -/// if succeeds, others otherwise. +/// Write the `bytes` into the `path` blockingly by `op_ptr`. +/// Error is NULL if successful, otherwise it contains the error code and error message. /// /// @NOTE It is important to notice that the `bytes` that is passes in will be consumed by this -/// function. +/// function. Therefore, you should not use the `bytes` after this function returns. /// /// @param ptr The opendal_operator_ptr created previously /// @param path The designated path you want to write your bytes in /// @param bytes The opendal_byte typed bytes to be written /// @see opendal_operator_ptr /// @see opendal_bytes -/// @see opendal_code -/// @return OPENDAL_OK if succeeds others otherwise +/// @see opendal_error +/// @return NULL if succeeds, otherwise it contains the error code and error message. /// /// # Example /// @@ -150,10 +150,10 @@ pub unsafe extern "C" fn opendal_operator_new( /// opendal_bytes bytes = opendal_bytes { .data = (uint8_t*)data, .len = 13 }; /// /// // now you can write! -/// opendal_code code = opendal_operator_blocking_write(ptr, "/testpath", bytes); +/// opendal_error *err = opendal_operator_blocking_write(ptr, "/testpath", bytes); /// /// // Assert that this succeeds -/// assert(code == OPENDAL_OK) +/// assert(err == NULL); /// ``` /// /// # Safety @@ -190,17 +190,16 @@ pub unsafe extern "C" fn opendal_operator_blocking_write( /// \brief Blockingly read the data from `path`. /// -/// Read the data out from `path` blockingly by operator, returns -/// an opendal_result_read with error code. +/// Read the data out from `path` blockingly by operator. /// /// @param ptr The opendal_operator_ptr created previously /// @param path The path you want to read the data out /// @see opendal_operator_ptr /// @see opendal_result_read -/// @see opendal_code +/// @see opendal_error /// @return Returns opendal_result_read, the `data` field is a pointer to a newly allocated -/// opendal_bytes, the `code` field contains the error code. If the `code` is not OPENDAL_OK, -/// the `data` field points to NULL. +/// opendal_bytes, the `error` field contains the error. If the `error` is not NULL, then +/// the operation failed and the `data` field is a nullptr. /// /// \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. @@ -212,7 +211,7 @@ pub unsafe extern "C" fn opendal_operator_blocking_write( /// // ... you have write "Hello, World!" to path "/testpath" /// /// opendal_result_read r = opendal_operator_blocking_read(ptr, "testpath"); -/// assert(r.code == OPENDAL_OK); +/// assert(r.error == NULL); /// /// opendal_bytes *bytes = r.data; /// assert(bytes->len == 13); @@ -259,14 +258,14 @@ pub unsafe extern "C" fn opendal_operator_blocking_read( /// \brief Blockingly delete the object in `path`. /// -/// Delete the object in `path` blockingly by `op_ptr`, returns the opendal_code OPENDAL_OK -/// if succeeds, others otherwise +/// Delete the object in `path` blockingly by `op_ptr`. +/// Error is NULL if successful, otherwise it contains the error code and error message. /// /// @param ptr The opendal_operator_ptr created previously /// @param path The designated path you want to delete /// @see opendal_operator_ptr -/// @see opendal_code -/// @return OPENDAL_OK if succeeds others otherwise +/// @see opendal_error +/// @return NULL if succeeds, otherwise it contains the error code and error message. /// /// # Example /// @@ -277,13 +276,15 @@ pub unsafe extern "C" fn opendal_operator_blocking_read( /// // prepare your data /// char* data = "Hello, World!"; /// opendal_bytes bytes = opendal_bytes { .data = (uint8_t*)data, .len = 13 }; -/// opendal_code code = opendal_operator_blocking_write(ptr, "/testpath", bytes); +/// opendal_error *error = opendal_operator_blocking_write(ptr, "/testpath", bytes); +/// +/// assert(error == NULL); /// /// // now you can delete! -/// opendal_code code = opendal_operator_blocking_delete(ptr, "/testpath"); +/// opendal_error *error = opendal_operator_blocking_delete(ptr, "/testpath"); /// /// // Assert that this succeeds -/// assert(code == OPENDAL_OK) +/// assert(error == NULL); /// ``` /// /// # Safety @@ -318,30 +319,28 @@ pub unsafe extern "C" fn opendal_operator_blocking_delete( /// \brief Check whether the path exists. /// /// If the operation succeeds, no matter the path exists or not, -/// the error code should be opendal_code::OPENDAL_OK. Otherwise, -/// the field `is_exist` is filled with false, and the error code -/// is set correspondingly. +/// the error should be a nullptr. Otherwise, the field `is_exist` +/// is filled with false, and the error is set /// /// @param ptr The opendal_operator_ptr created previously /// @param path The path you want to check existence /// @see opendal_operator_ptr /// @see opendal_result_is_exist -/// @see opendal_code +/// @see opendal_error /// @return Returns opendal_result_is_exist, the `is_exist` field contains whether the path exists. -/// However, it the operation fails, the `is_exist` will contains false and the error code will be -/// stored in the `code` field. +/// However, it the operation fails, the `is_exist` will contains false and the error will be set. /// /// # Example /// /// ```C /// // .. you previously wrote some data to path "/mytest/obj" /// opendal_result_is_exist e = opendal_operator_is_exist(ptr, "/mytest/obj"); -/// assert(e.code == OPENDAL_OK); +/// assert(e.error == NULL); /// assert(e.is_exist); /// /// // but you previously did **not** write any data to path "/yourtest/obj" -/// opendal_result_is_exist e = opendal_operator_is_exist(ptr, "yourtest/obj"); -/// assert(e.code == OPENDAL_OK); +/// opendal_result_is_exist e = opendal_operator_is_exist(ptr, "/yourtest/obj"); +/// assert(e.error == NULL); /// assert(!e.is_exist); /// ``` /// @@ -382,26 +381,24 @@ pub unsafe extern "C" fn opendal_operator_is_exist( /// \brief Stat the path, return its metadata. /// -/// If the operation succeeds, the error code should be -/// OPENDAL_OK. Otherwise, the field `meta` is filled with -/// a NULL pointer, and the error code is set correspondingly. +/// Error is NULL if successful, otherwise it contains the error code and error message. /// /// @param ptr The opendal_operator_ptr created previously /// @param path The path you want to stat /// @see opendal_operator_ptr /// @see opendal_result_stat /// @see opendal_metadata -/// @return Returns opendal_result_stat, containing a metadata and a opendal_code. +/// @return Returns opendal_result_stat, containing a metadata and an opendal_error. /// If the operation succeeds, the `meta` field would holds a valid metadata and -/// the `code` field should hold OPENDAL_OK. Otherwise the metadata will contain a -/// NULL pointer, i.e. invalid, and the `code` will be set correspondingly. +/// the `error` field should hold nullptr. Otherwise the metadata will contain a +/// NULL pointer, i.e. invalid, and the `error` will be set correspondingly. /// /// # Example /// /// ```C /// // ... previously you wrote "Hello, World!" to path "/testpath" /// opendal_result_stat s = opendal_operator_stat(ptr, "/testpath"); -/// assert(s.code == OPENDAL_OK); +/// assert(s.error == NULL); /// /// const opendal_metadata *meta = s.meta; /// @@ -453,7 +450,10 @@ pub unsafe extern "C" fn opendal_operator_stat( /// @param ptr The opendal_operator_ptr created previously /// @param path The designated path you want to delete /// @see opendal_blocking_lister -/// @return +/// @return Returns opendal_result_list, containing a lister and an opendal_error. +/// If the operation succeeds, the `lister` field would holds a valid lister and +/// the `error` field should hold nullptr. Otherwise the `lister`` will contain a +/// NULL pointer, i.e. invalid, and the `error` will be set correspondingly. /// /// # Example /// @@ -462,7 +462,7 @@ pub unsafe extern "C" fn opendal_operator_stat( /// // You have written some data into some files path "root/dir1" /// // Your opendal_operator_ptr was called ptr /// opendal_result_list l = opendal_operator_blocking_list(ptr, "root/dir1"); -/// assert(l.code == OPENDAL_OK); +/// assert(l.error == ERROR); /// /// opendal_blocking_lister *lister = l.lister; /// opendal_list_entry *entry; diff --git a/bindings/c/src/result.rs b/bindings/c/src/result.rs index 8ac76d6666cb..a01bd274b656 100644 --- a/bindings/c/src/result.rs +++ b/bindings/c/src/result.rs @@ -28,9 +28,9 @@ use crate::types::opendal_metadata; /// \brief The result type returned by opendal's read operation. /// /// The result type of read operation in opendal C binding, it contains -/// the data that the read operation returns and a error code. +/// the data that the read operation returns and an NULL error. /// If the read operation failed, the `data` fields should be a nullptr -/// and the error code is **NOT** OPENDAL_OK. +/// and the error is not NULL. #[repr(C)] pub struct opendal_result_read { /// The byte array with length returned by read operations @@ -42,8 +42,8 @@ pub struct opendal_result_read { /// \brief The result type returned by opendal_operator_is_exist(). /// /// The result type for opendal_operator_is_exist(), the field `is_exist` -/// contains whether the path exists, and the field `code` contains the -/// corresponding error code. +/// contains whether the path exists, and the field `error` contains the +/// corresponding error. If successful, the `error` field is null. /// /// \note If the opendal_operator_is_exist() fails, the `is_exist` field /// will be set to false. @@ -58,7 +58,8 @@ pub struct opendal_result_is_exist { /// \brief The result type returned by opendal_operator_stat(). /// /// The result type for opendal_operator_stat(), the field `meta` contains the metadata -/// of the path, the field `code` represents whether the stat operation is successful. +/// of the path, the field `error` represents whether the stat operation is successful. +/// If successful, the `error` field is null. #[repr(C)] pub struct opendal_result_stat { /// The metadata output of the stat @@ -70,8 +71,8 @@ pub struct opendal_result_stat { /// \brief The result type returned by opendal_operator_blocking_list(). /// /// The result type for opendal_operator_blocking_list(), the field `lister` contains the lister -/// of the path, which is an iterator of the objects under the path. the field `code` represents -/// whether the stat operation is successful. +/// of the path, which is an iterator of the objects under the path. the field `error` represents +/// whether the stat operation is successful. If successful, the `error` field is null. #[repr(C)] pub struct opendal_result_list { /// The lister, used for further listing operations diff --git a/bindings/c/src/types.rs b/bindings/c/src/types.rs index 89d02826e85b..591795d54527 100644 --- a/bindings/c/src/types.rs +++ b/bindings/c/src/types.rs @@ -168,7 +168,7 @@ impl opendal_metadata { /// ```C /// // ... previously you wrote "Hello, World!" to path "/testpath" /// opendal_result_stat s = opendal_operator_stat(ptr, "/testpath"); - /// assert(s.code == OPENDAL_OK); + /// assert(s.error == NULL); /// /// opendal_metadata *meta = s.meta; /// assert(opendal_metadata_content_length(meta) == 13); @@ -186,7 +186,7 @@ impl opendal_metadata { /// ```C /// // ... previously you wrote "Hello, World!" to path "/testpath" /// opendal_result_stat s = opendal_operator_stat(ptr, "/testpath"); - /// assert(s.code == OPENDAL_OK); + /// assert(s.error == NULL); /// /// opendal_metadata *meta = s.meta; /// assert(opendal_metadata_is_file(meta)); @@ -206,7 +206,7 @@ impl opendal_metadata { /// ```C /// // ... previously you wrote "Hello, World!" to path "/testpath" /// opendal_result_stat s = opendal_operator_stat(ptr, "/testpath"); - /// assert(s.code == OPENDAL_OK); + /// assert(s.error == NULL); /// /// opendal_metadata *meta = s.meta; /// From a1f74269e97f90c30d8b976e33bc0c9396148a66 Mon Sep 17 00:00:00 2001 From: Ji-Xinyou Date: Mon, 9 Oct 2023 14:46:27 -0400 Subject: [PATCH 03/19] tests --- bindings/c/Makefile | 5 +++++ bindings/c/examples/basicrw.c | 6 +++--- bindings/c/tests/bdd.cpp | 20 ++++++++++---------- bindings/c/tests/list.cpp | 8 ++++---- 4 files changed, 22 insertions(+), 17 deletions(-) diff --git a/bindings/c/Makefile b/bindings/c/Makefile index 6bc06bb0b090..e7b85ca12b9c 100644 --- a/bindings/c/Makefile +++ b/bindings/c/Makefile @@ -44,6 +44,11 @@ build: test: $(CXX) tests/bdd.cpp -o $(OBJ_DIR)/bdd $(CXXFLAGS) $(LDFLAGS) $(LIBS) $(CXX) tests/list.cpp -o $(OBJ_DIR)/list $(CXXFLAGS) $(LDFLAGS) $(LIBS) + ./$(OBJ_DIR)/bdd + ./$(OBJ_DIR)/list + +.PHONY: test_memory_leak +memory_leak: $(VALGRIND) $(OBJ_DIR)/bdd $(VALGRIND) $(OBJ_DIR)/list diff --git a/bindings/c/examples/basicrw.c b/bindings/c/examples/basicrw.c index 3c3cd75cddf9..4d2ce5c724d6 100644 --- a/bindings/c/examples/basicrw.c +++ b/bindings/c/examples/basicrw.c @@ -34,13 +34,13 @@ int main() }; /* Write this into path "/testpath" */ - opendal_code code = opendal_operator_blocking_write(op, "/testpath", data); - assert(code == OPENDAL_OK); + opendal_error *error = opendal_operator_blocking_write(op, "/testpath", data); + assert(error == NULL); /* We can read it out, make sure the data is the same */ opendal_result_read r = opendal_operator_blocking_read(op, "/testpath"); opendal_bytes* read_bytes = r.data; - assert(r.code == OPENDAL_OK); + assert(r.error == NULL); assert(read_bytes->len == 24); /* Lets print it out */ diff --git a/bindings/c/tests/bdd.cpp b/bindings/c/tests/bdd.cpp index c7d79717292f..dc1367350b51 100644 --- a/bindings/c/tests/bdd.cpp +++ b/bindings/c/tests/bdd.cpp @@ -58,17 +58,17 @@ TEST_F(OpendalBddTest, FeatureTest) .data = (uint8_t*)this->content.c_str(), .len = this->content.length(), }; - opendal_code code = opendal_operator_blocking_write(this->p, this->path.c_str(), data); - EXPECT_EQ(code, OPENDAL_OK); + opendal_error *error = opendal_operator_blocking_write(this->p, this->path.c_str(), data); + EXPECT_EQ(error, nullptr); // The blocking file "test" should exist opendal_result_is_exist e = opendal_operator_is_exist(this->p, this->path.c_str()); - EXPECT_EQ(e.code, OPENDAL_OK); + EXPECT_EQ(e.error, nullptr); EXPECT_TRUE(e.is_exist); // The blocking file "test" entry mode must be file opendal_result_stat s = opendal_operator_stat(this->p, this->path.c_str()); - EXPECT_EQ(s.code, OPENDAL_OK); + EXPECT_EQ(s.error, nullptr); opendal_metadata* meta = s.meta; EXPECT_TRUE(opendal_metadata_is_file(meta)); @@ -78,22 +78,22 @@ TEST_F(OpendalBddTest, FeatureTest) // The blocking file "test" must have content "Hello, World!" struct opendal_result_read r = opendal_operator_blocking_read(this->p, this->path.c_str()); - EXPECT_EQ(r.code, OPENDAL_OK); + 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])); } // The blocking file should be deleted - code = opendal_operator_blocking_delete(this->p, this->path.c_str()); - EXPECT_EQ(code, OPENDAL_OK); + error = opendal_operator_blocking_delete(this->p, this->path.c_str()); + EXPECT_EQ(error, nullptr); e = opendal_operator_is_exist(this->p, this->path.c_str()); - EXPECT_EQ(e.code, OPENDAL_OK); + EXPECT_EQ(e.error, nullptr); EXPECT_FALSE(e.is_exist); // The deletion operation should be idempotent - code = opendal_operator_blocking_delete(this->p, this->path.c_str()); - EXPECT_EQ(code, OPENDAL_OK); + error = opendal_operator_blocking_delete(this->p, this->path.c_str()); + EXPECT_EQ(error, nullptr); opendal_bytes_free(r.data); } diff --git a/bindings/c/tests/list.cpp b/bindings/c/tests/list.cpp index b6b44dd44d02..21300841fd9e 100644 --- a/bindings/c/tests/list.cpp +++ b/bindings/c/tests/list.cpp @@ -63,11 +63,11 @@ TEST_F(OpendalListTest, ListDirTest) // write must succeed EXPECT_EQ(opendal_operator_blocking_write(this->p, path.c_str(), data), - OPENDAL_OK); + nullptr); // list must succeed since the write succeeded opendal_result_list l = opendal_operator_blocking_list(this->p, (dname + "/").c_str()); - EXPECT_EQ(l.code, OPENDAL_OK); + EXPECT_EQ(l.error, nullptr); opendal_blocking_lister* lister = l.lister; @@ -80,7 +80,7 @@ TEST_F(OpendalListTest, ListDirTest) // stat must succeed opendal_result_stat s = opendal_operator_stat(this->p, de_path); - EXPECT_EQ(s.code, OPENDAL_OK); + EXPECT_EQ(s.error, nullptr); if (!strcmp(de_path, path.c_str())) { found = true; @@ -102,7 +102,7 @@ TEST_F(OpendalListTest, ListDirTest) // delete EXPECT_EQ(opendal_operator_blocking_delete(this->p, path.c_str()), - OPENDAL_OK); + nullptr); opendal_lister_free(lister); } From 59db58d5ab5deeed91fd26b5bcdcd2ca3a348973 Mon Sep 17 00:00:00 2001 From: Ji-Xinyou Date: Mon, 9 Oct 2023 17:05:55 -0400 Subject: [PATCH 04/19] error msg example --- bindings/c/.gitignore | 8 ++++++ bindings/c/examples/error_msg.c | 46 +++++++++++++++++++++++++++++++++ bindings/c/src/error.rs | 1 + bindings/c/src/lib.rs | 1 + 4 files changed, 56 insertions(+) create mode 100644 bindings/c/examples/error_msg.c diff --git a/bindings/c/.gitignore b/bindings/c/.gitignore index 567609b1234a..48efa3d51f8b 100644 --- a/bindings/c/.gitignore +++ b/bindings/c/.gitignore @@ -1 +1,9 @@ build/ + +# Ignore everything under 'examples' +examples/* +!examples/ +!examples/*.h +!examples/*.c +!examples/*.cc +!examples/*.cpp diff --git a/bindings/c/examples/error_msg.c b/bindings/c/examples/error_msg.c new file mode 100644 index 000000000000..cd9ca75f2465 --- /dev/null +++ b/bindings/c/examples/error_msg.c @@ -0,0 +1,46 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#include "assert.h" +#include "opendal.h" +#include "stdio.h" + +// this example shows how to get error message from opendal_error +int main() +{ + /* Initialize a operator for "memory" backend, with no options */ + const opendal_operator_ptr *op = opendal_operator_new("memory", 0); + assert(op->ptr != NULL); + + /* The read is supposed to fail */ + opendal_result_read r = opendal_operator_blocking_read(op, "/testpath"); + assert(r.error != NULL); + + /* Lets print the error message out */ + struct opendal_bytes *error_msg = &r.error->message; + for (int i = 0; i < error_msg->len; ++i) { + printf("%c", error_msg->data[i]); + } + + /* the opendal_bytes read is heap allocated, please free it */ + opendal_bytes_free(r.data); + + /* the operator_ptr is also heap allocated */ + opendal_operator_free(op); +} \ No newline at end of file diff --git a/bindings/c/src/error.rs b/bindings/c/src/error.rs index ac59fcda6f9e..5e32d17cd3c4 100644 --- a/bindings/c/src/error.rs +++ b/bindings/c/src/error.rs @@ -78,6 +78,7 @@ pub struct opendal_error { message: opendal_bytes, } +// todo: add free impl opendal_error { // The caller should sink the error to heap memory and return the pointer // that will not be freed by rustc diff --git a/bindings/c/src/lib.rs b/bindings/c/src/lib.rs index 0fc7329c8077..854bd7004e4b 100644 --- a/bindings/c/src/lib.rs +++ b/bindings/c/src/lib.rs @@ -46,6 +46,7 @@ use crate::types::opendal_metadata; use crate::types::opendal_operator_options; use crate::types::opendal_operator_ptr; +//todo: add error to this /// \brief Construct an operator based on `scheme` and `options` /// /// Uses an array of key-value pairs to initialize the operator based on provided `scheme` From 013875db035820b07a5c0782d813b145cef4ab6d Mon Sep 17 00:00:00 2001 From: Ji-Xinyou Date: Mon, 9 Oct 2023 17:48:30 -0400 Subject: [PATCH 05/19] free --- bindings/c/Makefile | 6 ++-- bindings/c/examples/basicrw.c | 4 +-- bindings/c/examples/error_msg.c | 8 ++--- bindings/c/include/opendal.h | 5 +++ bindings/c/src/error.rs | 17 ++++++++++ bindings/c/tests/bdd.cpp | 2 +- bindings/c/tests/error_msg.cpp | 55 +++++++++++++++++++++++++++++++++ 7 files changed, 88 insertions(+), 9 deletions(-) create mode 100644 bindings/c/tests/error_msg.cpp diff --git a/bindings/c/Makefile b/bindings/c/Makefile index e7b85ca12b9c..54655b7e3c5b 100644 --- a/bindings/c/Makefile +++ b/bindings/c/Makefile @@ -44,8 +44,10 @@ build: test: $(CXX) tests/bdd.cpp -o $(OBJ_DIR)/bdd $(CXXFLAGS) $(LDFLAGS) $(LIBS) $(CXX) tests/list.cpp -o $(OBJ_DIR)/list $(CXXFLAGS) $(LDFLAGS) $(LIBS) - ./$(OBJ_DIR)/bdd - ./$(OBJ_DIR)/list + $(CXX) tests/error_msg.cpp -o $(OBJ_DIR)/error_msg $(CXXFLAGS) $(LDFLAGS) $(LIBS) + $(OBJ_DIR)/bdd + $(OBJ_DIR)/list + $(OBJ_DIR)/error_msg .PHONY: test_memory_leak memory_leak: diff --git a/bindings/c/examples/basicrw.c b/bindings/c/examples/basicrw.c index 4d2ce5c724d6..906d13d1e191 100644 --- a/bindings/c/examples/basicrw.c +++ b/bindings/c/examples/basicrw.c @@ -24,7 +24,7 @@ int main() { /* Initialize a operator for "memory" backend, with no options */ - const opendal_operator_ptr *op = opendal_operator_new("memory", 0); + const opendal_operator_ptr* op = opendal_operator_new("memory", 0); assert(op->ptr != NULL); /* Prepare some data to be written */ @@ -34,7 +34,7 @@ int main() }; /* Write this into path "/testpath" */ - opendal_error *error = opendal_operator_blocking_write(op, "/testpath", data); + opendal_error* error = opendal_operator_blocking_write(op, "/testpath", data); assert(error == NULL); /* We can read it out, make sure the data is the same */ diff --git a/bindings/c/examples/error_msg.c b/bindings/c/examples/error_msg.c index cd9ca75f2465..05053f0184be 100644 --- a/bindings/c/examples/error_msg.c +++ b/bindings/c/examples/error_msg.c @@ -25,7 +25,7 @@ int main() { /* Initialize a operator for "memory" backend, with no options */ - const opendal_operator_ptr *op = opendal_operator_new("memory", 0); + const opendal_operator_ptr* op = opendal_operator_new("memory", 0); assert(op->ptr != NULL); /* The read is supposed to fail */ @@ -33,13 +33,13 @@ int main() assert(r.error != NULL); /* Lets print the error message out */ - struct opendal_bytes *error_msg = &r.error->message; + struct opendal_bytes* error_msg = &r.error->message; for (int i = 0; i < error_msg->len; ++i) { printf("%c", error_msg->data[i]); } - /* the opendal_bytes read is heap allocated, please free it */ - opendal_bytes_free(r.data); + /* free the error */ + opendal_error_free(r.error); /* 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 index a996a9576500..bf92942e670f 100644 --- a/bindings/c/include/opendal.h +++ b/bindings/c/include/opendal.h @@ -677,6 +677,11 @@ struct opendal_result_stat opendal_operator_stat(const struct opendal_operator_p struct opendal_result_list opendal_operator_blocking_list(const struct opendal_operator_ptr *ptr, const char *path); +/** + * \brief Frees the opendal_error + */ +void opendal_error_free(struct opendal_error *ptr); + /** * \brief Free the heap-allocated operator pointed by opendal_operator_ptr. * diff --git a/bindings/c/src/error.rs b/bindings/c/src/error.rs index 5e32d17cd3c4..992da0810518 100644 --- a/bindings/c/src/error.rs +++ b/bindings/c/src/error.rs @@ -89,4 +89,21 @@ impl opendal_error { let message = opendal_bytes::new(c_str.into_bytes()); opendal_error { code, message } } + + /// \brief Frees the 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 as *const opendal_bytes as *mut opendal_bytes; + if !message_ptr.is_null() { + let data_mut = unsafe { (*message_ptr).data as *mut u8 }; + let _ = unsafe { + Vec::from_raw_parts(data_mut, (*message_ptr).len, (*message_ptr).len) + }; + } + + // free the pointer + let _ = unsafe { Box::from_raw(ptr) }; + } + } } diff --git a/bindings/c/tests/bdd.cpp b/bindings/c/tests/bdd.cpp index dc1367350b51..4d4feb0860c8 100644 --- a/bindings/c/tests/bdd.cpp +++ b/bindings/c/tests/bdd.cpp @@ -58,7 +58,7 @@ TEST_F(OpendalBddTest, FeatureTest) .data = (uint8_t*)this->content.c_str(), .len = this->content.length(), }; - opendal_error *error = opendal_operator_blocking_write(this->p, this->path.c_str(), data); + opendal_error* error = opendal_operator_blocking_write(this->p, this->path.c_str(), data); EXPECT_EQ(error, nullptr); // The blocking file "test" should exist diff --git a/bindings/c/tests/error_msg.cpp b/bindings/c/tests/error_msg.cpp new file mode 100644 index 000000000000..dd6d48ef4b37 --- /dev/null +++ b/bindings/c/tests/error_msg.cpp @@ -0,0 +1,55 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#include "gtest/gtest.h" +extern "C" { +#include "opendal.h" +} + +// Test no memory leak of error message +TEST(ErrorMessageTest, ErrorMessageTest) +{ + // Initialize a operator for "memory" backend, with no options + const opendal_operator_ptr* op = opendal_operator_new("memory", 0); + ASSERT_NE(op->ptr, nullptr); + + // The read is supposed to fail + opendal_result_read r = opendal_operator_blocking_read(op, "/testpath"); + ASSERT_NE(r.error, nullptr); + + // Lets check the error message out + struct opendal_bytes* error_msg = &r.error->message; + ASSERT_NE(error_msg->data, nullptr); + ASSERT_GT(error_msg->len, 0); + + // the opendal_bytes read is heap allocated, please free it + opendal_bytes_free(r.data); + + // free the error + opendal_error_free(r.error); + + // the operator_ptr is also heap allocated + opendal_operator_free(op); +} + +int main(int argc, char** argv) +{ + ::testing::InitGoogleTest(&argc, argv); + return RUN_ALL_TESTS(); +} From b3e559f2f47b58ee594b374dea1165df30144320 Mon Sep 17 00:00:00 2001 From: Ji-Xinyou Date: Mon, 9 Oct 2023 17:49:54 -0400 Subject: [PATCH 06/19] separate the valgrind test w/ original test --- .github/workflows/bindings_c.yml | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/.github/workflows/bindings_c.yml b/.github/workflows/bindings_c.yml index faf616ba84df..efea91276eb6 100644 --- a/.github/workflows/bindings_c.yml +++ b/.github/workflows/bindings_c.yml @@ -63,6 +63,10 @@ jobs: - name: Check diff run: git diff --exit-code - - name: Build and Run BDD tests + - name: Build and Run tests working-directory: "bindings/c" run: make test + + - name: Build and Run memory-leak tests + working-directory: "bindings/c" + run: make memory_leak From 65f2b45c83b1c0c15f4701ba8ad507dd788942c8 Mon Sep 17 00:00:00 2001 From: Ji-Xinyou Date: Mon, 9 Oct 2023 17:56:08 -0400 Subject: [PATCH 07/19] docs --- bindings/c/include/opendal.h | 27 +++++++++++++++++++++++---- bindings/c/src/error.rs | 20 ++++++++++++++++++-- bindings/c/src/lib.rs | 2 +- bindings/c/src/types.rs | 4 ++-- 4 files changed, 44 insertions(+), 9 deletions(-) diff --git a/bindings/c/include/opendal.h b/bindings/c/include/opendal.h index bf92942e670f..259bb2645d12 100644 --- a/bindings/c/include/opendal.h +++ b/bindings/c/include/opendal.h @@ -229,6 +229,25 @@ typedef struct opendal_bytes { uintptr_t len; } opendal_bytes; +/** + * \brief The opendal error type for C binding, containing an error code and corresponding error + * message. + * + * The normal operations returns a pointer to the opendal_error, and the **nullptr normally + * 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 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 + * opendal_error_free. 2. The error message is not null terminated, so the caller should + * never use "%s" to print the error message. + * + * @see opendal_code + * @see opendal_bytes + * @see opendal_error_free + */ typedef struct opendal_error { enum opendal_code code; struct opendal_bytes message; @@ -409,7 +428,7 @@ const struct opendal_operator_ptr *opendal_operator_new(const char *scheme, * Write the `bytes` into the `path` blockingly by `op_ptr`. * Error is NULL if successful, otherwise it contains the error code and error message. * - * @NOTE It is important to notice that the `bytes` that is passes in will be consumed by this + * \note It is important to notice that the `bytes` that is passes in will be consumed by this * function. Therefore, you should not use the `bytes` after this function returns. * * @param ptr The opendal_operator_ptr created previously @@ -678,7 +697,7 @@ struct opendal_result_list opendal_operator_blocking_list(const struct opendal_o const char *path); /** - * \brief Frees the opendal_error + * \brief Frees the opendal_error, ok to call on NULL */ void opendal_error_free(struct opendal_error *ptr); @@ -820,7 +839,7 @@ void opendal_lister_free(const struct opendal_blocking_lister *p); * * Path is relative to operator's root. Only valid in current operator. * - * @NOTE To free the string, you can directly call free() + * \note To free the string, you can directly call free() */ char *opendal_list_entry_path(const struct opendal_list_entry *self); @@ -831,7 +850,7 @@ char *opendal_list_entry_path(const struct opendal_list_entry *self); * If this entry is a dir, `Name` MUST endswith `/` * Otherwise, `Name` MUST NOT endswith `/`. * - * @NOTE To free the string, you can directly call free() + * \note To free the string, you can directly call free() */ char *opendal_list_entry_name(const struct opendal_list_entry *self); diff --git a/bindings/c/src/error.rs b/bindings/c/src/error.rs index 992da0810518..751eb97b5da0 100644 --- a/bindings/c/src/error.rs +++ b/bindings/c/src/error.rs @@ -72,13 +72,29 @@ impl opendal_internal_error { } } +/// \brief The opendal error type for C binding, containing an error code and corresponding error +/// message. +/// +/// The normal operations returns a pointer to the opendal_error, and the **nullptr normally +/// 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 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 +/// opendal_error_free. 2. The error message is not null terminated, so the caller should +/// never use "%s" to print the error message. +/// +/// @see opendal_code +/// @see opendal_bytes +/// @see opendal_error_free #[repr(C)] pub struct opendal_error { code: opendal_code, message: opendal_bytes, } -// todo: add free impl opendal_error { // The caller should sink the error to heap memory and return the pointer // that will not be freed by rustc @@ -90,7 +106,7 @@ impl opendal_error { opendal_error { code, message } } - /// \brief Frees the opendal_error + /// \brief Frees the opendal_error, ok to call on NULL #[no_mangle] pub unsafe extern "C" fn opendal_error_free(ptr: *mut opendal_error) { if !ptr.is_null() { diff --git a/bindings/c/src/lib.rs b/bindings/c/src/lib.rs index 854bd7004e4b..801b2a47a158 100644 --- a/bindings/c/src/lib.rs +++ b/bindings/c/src/lib.rs @@ -129,7 +129,7 @@ pub unsafe extern "C" fn opendal_operator_new( /// Write the `bytes` into the `path` blockingly by `op_ptr`. /// Error is NULL if successful, otherwise it contains the error code and error message. /// -/// @NOTE It is important to notice that the `bytes` that is passes in will be consumed by this +/// \note It is important to notice that the `bytes` that is passes in will be consumed by this /// function. Therefore, you should not use the `bytes` after this function returns. /// /// @param ptr The opendal_operator_ptr created previously diff --git a/bindings/c/src/types.rs b/bindings/c/src/types.rs index 591795d54527..dbc0aa68ee9d 100644 --- a/bindings/c/src/types.rs +++ b/bindings/c/src/types.rs @@ -377,7 +377,7 @@ impl opendal_list_entry { /// /// Path is relative to operator's root. Only valid in current operator. /// - /// @NOTE To free the string, you can directly call free() + /// \note To free the string, you can directly call free() #[no_mangle] pub unsafe extern "C" fn opendal_list_entry_path(&self) -> *mut c_char { let s = (*self.inner).path(); @@ -391,7 +391,7 @@ impl opendal_list_entry { /// If this entry is a dir, `Name` MUST endswith `/` /// Otherwise, `Name` MUST NOT endswith `/`. /// - /// @NOTE To free the string, you can directly call free() + /// \note To free the string, you can directly call free() #[no_mangle] pub unsafe extern "C" fn opendal_list_entry_name(&self) -> *mut c_char { let s = (*self.inner).name(); From aaa2a2d5a441393820abbab8aaddb777eae693c1 Mon Sep 17 00:00:00 2001 From: Ji-Xinyou Date: Mon, 9 Oct 2023 18:07:18 -0400 Subject: [PATCH 08/19] add opendal_operator_new result --- bindings/c/include/opendal.h | 43 ++++++++++++++++++++---------------- bindings/c/src/error.rs | 14 ++++++++---- bindings/c/src/lib.rs | 37 ++++++++++++++++++++++++------- bindings/c/src/result.rs | 7 ++++++ 4 files changed, 70 insertions(+), 31 deletions(-) diff --git a/bindings/c/include/opendal.h b/bindings/c/include/opendal.h index 259bb2645d12..2b027a5d68d6 100644 --- a/bindings/c/include/opendal.h +++ b/bindings/c/include/opendal.h @@ -191,24 +191,6 @@ typedef struct opendal_operator_ptr { const struct BlockingOperator *ptr; } opendal_operator_ptr; -/** - * \brief The configuration for the initialization of opendal_operator_ptr. - * - * \note This is also a heap-allocated struct, please free it after you use it - * - * @see opendal_operator_new has an example of using opendal_operator_options - * @see opendal_operator_options_new This function construct the operator - * @see opendal_operator_options_free This function frees the heap memory of the operator - * @see opendal_operator_options_set This function allow you to set the options - */ -typedef struct opendal_operator_options { - /** - * The pointer to the Rust HashMap - * Only touch this on judging whether it is NULL. - */ - struct HashMap_String__String *inner; -} opendal_operator_options; - /** * \brief opendal_bytes carries raw-bytes with its length * @@ -253,6 +235,29 @@ typedef struct opendal_error { struct opendal_bytes message; } opendal_error; +typedef struct opendal_result_operator_new { + struct opendal_operator_ptr *operator_ptr; + struct opendal_error *error; +} opendal_result_operator_new; + +/** + * \brief The configuration for the initialization of opendal_operator_ptr. + * + * \note This is also a heap-allocated struct, please free it after you use it + * + * @see opendal_operator_new has an example of using opendal_operator_options + * @see opendal_operator_options_new This function construct the operator + * @see opendal_operator_options_free This function frees the heap memory of the operator + * @see opendal_operator_options_set This function allow you to set the options + */ +typedef struct opendal_operator_options { + /** + * The pointer to the Rust HashMap + * Only touch this on judging whether it is NULL. + */ + struct HashMap_String__String *inner; +} opendal_operator_options; + /** * \brief The result type returned by opendal's read operation. * @@ -419,7 +424,7 @@ extern "C" { * the string. * * The `scheme` points to NULL, this function simply returns you a null opendal_operator_ptr. */ -const struct opendal_operator_ptr *opendal_operator_new(const char *scheme, +struct opendal_result_operator_new opendal_operator_new(const char *scheme, const struct opendal_operator_options *options); /** diff --git a/bindings/c/src/error.rs b/bindings/c/src/error.rs index 751eb97b5da0..536991d949da 100644 --- a/bindings/c/src/error.rs +++ b/bindings/c/src/error.rs @@ -74,18 +74,18 @@ impl opendal_internal_error { /// \brief The opendal error type for C binding, containing an error code and corresponding error /// message. -/// +/// /// The normal operations returns a pointer to the opendal_error, and the **nullptr normally /// 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 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 /// opendal_error_free. 2. The error message is not null terminated, so the caller should /// never use "%s" to print the error message. -/// +/// /// @see opendal_code /// @see opendal_bytes /// @see opendal_error_free @@ -106,6 +106,12 @@ impl opendal_error { opendal_error { code, message } } + pub(crate) fn manual_error(code: opendal_code, message: impl Into) -> Self { + let message_str = message.into(); + let message = opendal_bytes::new(message_str.into_bytes()); + opendal_error { code, message } + } + /// \brief Frees the opendal_error, ok to call on NULL #[no_mangle] pub unsafe extern "C" fn opendal_error_free(ptr: *mut opendal_error) { diff --git a/bindings/c/src/lib.rs b/bindings/c/src/lib.rs index 801b2a47a158..ae59d60a6561 100644 --- a/bindings/c/src/lib.rs +++ b/bindings/c/src/lib.rs @@ -36,6 +36,7 @@ use std::str::FromStr; use ::opendal as od; use error::opendal_error; use result::opendal_result_list; +use result::opendal_result_operator_new; use types::opendal_blocking_lister; use crate::result::opendal_result_is_exist; @@ -91,16 +92,29 @@ use crate::types::opendal_operator_ptr; pub unsafe extern "C" fn opendal_operator_new( scheme: *const c_char, options: *const opendal_operator_options, -) -> *const opendal_operator_ptr { +) -> opendal_result_operator_new { if scheme.is_null() { - return std::ptr::null(); + let error = opendal_error::manual_error( + error::opendal_code::OPENDAL_CONFIG_INVALID, + "The scheme given is pointing at NULL", + ); + let result = opendal_result_operator_new { + operator_ptr: std::ptr::null_mut(), + error: Box::into_raw(Box::new(error)), + }; + return result; } let scheme_str = unsafe { std::ffi::CStr::from_ptr(scheme).to_str().unwrap() }; let scheme = match od::Scheme::from_str(scheme_str) { Ok(s) => s, - Err(_) => { - return std::ptr::null(); + Err(e) => { + let e = opendal_error::from_opendal_error(e); + let result = opendal_result_operator_new { + operator_ptr: std::ptr::null_mut(), + error: Box::into_raw(Box::new(e)), + }; + return result; } }; @@ -113,15 +127,22 @@ pub unsafe extern "C" fn opendal_operator_new( let op = match od::Operator::via_map(scheme, map) { Ok(o) => o.blocking(), - Err(_) => { - return std::ptr::null(); + Err(e) => { + let e = opendal_error::from_opendal_error(e); + let result = opendal_result_operator_new { + operator_ptr: std::ptr::null_mut(), + error: Box::into_raw(Box::new(e)), + }; + return result; } }; // this prevents the operator memory from being dropped by the Box let op = opendal_operator_ptr::from(Box::into_raw(Box::new(op))); - - Box::into_raw(Box::new(op)) + opendal_result_operator_new { + operator_ptr: Box::into_raw(Box::new(op)), + error: std::ptr::null_mut(), + } } /// \brief Blockingly write raw bytes to `path`. diff --git a/bindings/c/src/result.rs b/bindings/c/src/result.rs index a01bd274b656..5412be128c81 100644 --- a/bindings/c/src/result.rs +++ b/bindings/c/src/result.rs @@ -24,6 +24,13 @@ use crate::error::opendal_error; use crate::types::opendal_blocking_lister; use crate::types::opendal_bytes; use crate::types::opendal_metadata; +use crate::types::opendal_operator_ptr; + +#[repr(C)] +pub struct opendal_result_operator_new { + pub operator_ptr: *mut opendal_operator_ptr, + pub error: *mut opendal_error, +} /// \brief The result type returned by opendal's read operation. /// From 46afe5ae07914873e896e0c0387831aff63d3fb4 Mon Sep 17 00:00:00 2001 From: Ji-Xinyou Date: Mon, 9 Oct 2023 18:12:33 -0400 Subject: [PATCH 09/19] refactor the test to use new operator init logic --- bindings/c/tests/bdd.cpp | 5 ++++- bindings/c/tests/error_msg.cpp | 32 ++++++++++++++++++++++++-------- bindings/c/tests/list.cpp | 5 ++++- 3 files changed, 32 insertions(+), 10 deletions(-) diff --git a/bindings/c/tests/bdd.cpp b/bindings/c/tests/bdd.cpp index 4d4feb0860c8..049843601a6b 100644 --- a/bindings/c/tests/bdd.cpp +++ b/bindings/c/tests/bdd.cpp @@ -41,7 +41,10 @@ class OpendalBddTest : public ::testing::Test { opendal_operator_options_set(options, "root", "/myroot"); // Given A new OpenDAL Blocking Operator - this->p = opendal_operator_new(scheme.c_str(), options); + opendal_result_operator_new result = opendal_operator_new(scheme.c_str(), options); + EXPECT_TRUE(result.error == nullptr); + + this->p = result.operator_ptr; EXPECT_TRUE(this->p->ptr); opendal_operator_options_free(options); diff --git a/bindings/c/tests/error_msg.cpp b/bindings/c/tests/error_msg.cpp index dd6d48ef4b37..65e02cb5a9fe 100644 --- a/bindings/c/tests/error_msg.cpp +++ b/bindings/c/tests/error_msg.cpp @@ -22,15 +22,34 @@ extern "C" { #include "opendal.h" } +class OpendalErrorMsgTest : public ::testing::Test { +protected: + const opendal_operator_ptr* p; + + // set up a brand new operator + void SetUp() override + { + opendal_operator_options* options = opendal_operator_options_new(); + opendal_operator_options_set(options, "root", "/myroot"); + + opendal_result_operator_new result = opendal_operator_new("memory", options); + EXPECT_TRUE(result.error == nullptr); + + this->p = result.operator_ptr; + EXPECT_TRUE(this->p->ptr); + + opendal_operator_options_free(options); + } + + void TearDown() override { opendal_operator_free(this->p); } +}; + // Test no memory leak of error message -TEST(ErrorMessageTest, ErrorMessageTest) +TEST_F(OpendalErrorMsgTest, ErrorReadTest) { // Initialize a operator for "memory" backend, with no options - const opendal_operator_ptr* op = opendal_operator_new("memory", 0); - ASSERT_NE(op->ptr, nullptr); - // The read is supposed to fail - opendal_result_read r = opendal_operator_blocking_read(op, "/testpath"); + opendal_result_read r = opendal_operator_blocking_read(this->p, "/testpath"); ASSERT_NE(r.error, nullptr); // Lets check the error message out @@ -43,9 +62,6 @@ TEST(ErrorMessageTest, ErrorMessageTest) // free the error opendal_error_free(r.error); - - // the operator_ptr is also heap allocated - opendal_operator_free(op); } int main(int argc, char** argv) diff --git a/bindings/c/tests/list.cpp b/bindings/c/tests/list.cpp index 21300841fd9e..0ebeac4f9bde 100644 --- a/bindings/c/tests/list.cpp +++ b/bindings/c/tests/list.cpp @@ -35,7 +35,10 @@ class OpendalListTest : public ::testing::Test { opendal_operator_options* options = opendal_operator_options_new(); opendal_operator_options_set(options, "root", "/myroot"); - this->p = opendal_operator_new("memory", options); + opendal_result_operator_new result = opendal_operator_new("memory", options); + EXPECT_TRUE(result.error == nullptr); + + this->p = result.operator_ptr; EXPECT_TRUE(this->p->ptr); opendal_operator_options_free(options); From a4bd35528f6b119fb59971c53e6fea8905891fc5 Mon Sep 17 00:00:00 2001 From: Ji-Xinyou Date: Mon, 9 Oct 2023 18:16:07 -0400 Subject: [PATCH 10/19] refactor the examples to use the new operator_ptr init logic --- bindings/c/examples/basicrw.c | 7 +++++-- bindings/c/examples/error_msg.c | 7 +++++-- bindings/c/include/opendal.h | 27 ++++++++++++++++++--------- bindings/c/src/lib.rs | 16 ++++++---------- bindings/c/src/result.rs | 10 ++++++++++ bindings/c/tests/bdd.cpp | 2 +- bindings/c/tests/error_msg.cpp | 2 +- bindings/c/tests/list.cpp | 2 +- 8 files changed, 47 insertions(+), 26 deletions(-) diff --git a/bindings/c/examples/basicrw.c b/bindings/c/examples/basicrw.c index 906d13d1e191..88071847c345 100644 --- a/bindings/c/examples/basicrw.c +++ b/bindings/c/examples/basicrw.c @@ -24,8 +24,11 @@ int main() { /* Initialize a operator for "memory" backend, with no options */ - const opendal_operator_ptr* op = opendal_operator_new("memory", 0); - assert(op->ptr != NULL); + opendal_result_operator_new result = opendal_operator_new("memory", 0); + assert(result.operator_ptr != NULL); + assert(result.error == NULL); + + opendal_operator_ptr* op = result.operator_ptr; /* Prepare some data to be written */ opendal_bytes data = { diff --git a/bindings/c/examples/error_msg.c b/bindings/c/examples/error_msg.c index 05053f0184be..7a6d11697b7e 100644 --- a/bindings/c/examples/error_msg.c +++ b/bindings/c/examples/error_msg.c @@ -25,8 +25,11 @@ int main() { /* Initialize a operator for "memory" backend, with no options */ - const opendal_operator_ptr* op = opendal_operator_new("memory", 0); - assert(op->ptr != NULL); + opendal_result_operator_new result = opendal_operator_new("memory", 0); + assert(result.operator_ptr != NULL); + assert(result.error == NULL); + + opendal_operator_ptr* op = result.operator_ptr; /* The read is supposed to fail */ opendal_result_read r = opendal_operator_blocking_read(op, "/testpath"); diff --git a/bindings/c/include/opendal.h b/bindings/c/include/opendal.h index 2b027a5d68d6..82ecbca8f3cf 100644 --- a/bindings/c/include/opendal.h +++ b/bindings/c/include/opendal.h @@ -235,6 +235,18 @@ typedef struct opendal_error { struct opendal_bytes message; } opendal_error; +/** + * \brief The result type returned by opendal_operator_new() operation. + * + * If the init logic is successful, the `operator_ptr` field will be set to a valid + * pointer, and the `error` field will be set to null. If the init logic fails, the + * `operator_ptr` field will be set to null, and the `error` field will be set to a + * valid pointer with error code and error message. + * + * @see opendal_operator_new() + * @see opendal_operator_ptr + * @see opendal_error + */ typedef struct opendal_result_operator_new { struct opendal_operator_ptr *operator_ptr; struct opendal_error *error; @@ -394,10 +406,9 @@ extern "C" { * @param options the pointer to the options for this operators, it could be NULL, which means no * option is set * @see opendal_operator_options - * @return A valid opendal_operator_ptr setup with the `scheme` and `options` is the construction - * succeeds. A null opendal_operator_ptr if any error happens. - * - * \remark You may use the `ptr` field of opendal_operator_ptr to check if it is NULL. + * @return A valid opendal_result_operator_new setup with the `scheme` and `options` is the construction + * succeeds. On success the operator_ptr field is a valid pointer to a newly allocated opendal_operator_ptr, + * and the error field is NULL. Otherwise, the operator_ptr field is a NULL pointer and the error field. * * # Example * @@ -409,7 +420,8 @@ extern "C" { * opendal_operator_options_set(options, "root", "/myroot"); * * // Construct the operator based on the options and scheme - * const opendal_operator_ptr *ptr = opendal_operator_new("memory", options); + * opendal_result_operator_new result = opendal_operator_new("memory", options); + * opendal_operator_ptr* op = result.operator_ptr; * * // you could free the options right away since the options is not used afterwards * opendal_operator_options_free(options); @@ -419,10 +431,7 @@ extern "C" { * * # Safety * - * It is **safe** under two cases below - * * The memory pointed to by `scheme` contain a valid nul terminator at the end of - * the string. - * * The `scheme` points to NULL, this function simply returns you a null opendal_operator_ptr. + * The only unsafe case is passing a invalid c string pointer to the `scheme` argument. */ struct opendal_result_operator_new opendal_operator_new(const char *scheme, const struct opendal_operator_options *options); diff --git a/bindings/c/src/lib.rs b/bindings/c/src/lib.rs index ae59d60a6561..1d0fbe5072ce 100644 --- a/bindings/c/src/lib.rs +++ b/bindings/c/src/lib.rs @@ -47,7 +47,6 @@ use crate::types::opendal_metadata; use crate::types::opendal_operator_options; use crate::types::opendal_operator_ptr; -//todo: add error to this /// \brief Construct an operator based on `scheme` and `options` /// /// Uses an array of key-value pairs to initialize the operator based on provided `scheme` @@ -59,10 +58,9 @@ use crate::types::opendal_operator_ptr; /// @param options the pointer to the options for this operators, it could be NULL, which means no /// option is set /// @see opendal_operator_options -/// @return A valid opendal_operator_ptr setup with the `scheme` and `options` is the construction -/// succeeds. A null opendal_operator_ptr if any error happens. -/// -/// \remark You may use the `ptr` field of opendal_operator_ptr to check if it is NULL. +/// @return A valid opendal_result_operator_new setup with the `scheme` and `options` is the construction +/// succeeds. On success the operator_ptr field is a valid pointer to a newly allocated opendal_operator_ptr, +/// and the error field is NULL. Otherwise, the operator_ptr field is a NULL pointer and the error field. /// /// # Example /// @@ -74,7 +72,8 @@ use crate::types::opendal_operator_ptr; /// opendal_operator_options_set(options, "root", "/myroot"); /// /// // Construct the operator based on the options and scheme -/// const opendal_operator_ptr *ptr = opendal_operator_new("memory", options); +/// opendal_result_operator_new result = opendal_operator_new("memory", options); +/// opendal_operator_ptr* op = result.operator_ptr; /// /// // you could free the options right away since the options is not used afterwards /// opendal_operator_options_free(options); @@ -84,10 +83,7 @@ use crate::types::opendal_operator_ptr; /// /// # Safety /// -/// It is **safe** under two cases below -/// * The memory pointed to by `scheme` contain a valid nul terminator at the end of -/// the string. -/// * The `scheme` points to NULL, this function simply returns you a null opendal_operator_ptr. +/// The only unsafe case is passing a invalid c string pointer to the `scheme` argument. #[no_mangle] pub unsafe extern "C" fn opendal_operator_new( scheme: *const c_char, diff --git a/bindings/c/src/result.rs b/bindings/c/src/result.rs index 5412be128c81..aa95c1190b01 100644 --- a/bindings/c/src/result.rs +++ b/bindings/c/src/result.rs @@ -26,6 +26,16 @@ use crate::types::opendal_bytes; use crate::types::opendal_metadata; use crate::types::opendal_operator_ptr; +/// \brief The result type returned by opendal_operator_new() operation. +/// +/// If the init logic is successful, the `operator_ptr` field will be set to a valid +/// pointer, and the `error` field will be set to null. If the init logic fails, the +/// `operator_ptr` field will be set to null, and the `error` field will be set to a +/// valid pointer with error code and error message. +/// +/// @see opendal_operator_new() +/// @see opendal_operator_ptr +/// @see opendal_error #[repr(C)] pub struct opendal_result_operator_new { pub operator_ptr: *mut opendal_operator_ptr, diff --git a/bindings/c/tests/bdd.cpp b/bindings/c/tests/bdd.cpp index 049843601a6b..76d5047faf0a 100644 --- a/bindings/c/tests/bdd.cpp +++ b/bindings/c/tests/bdd.cpp @@ -45,7 +45,7 @@ class OpendalBddTest : public ::testing::Test { EXPECT_TRUE(result.error == nullptr); this->p = result.operator_ptr; - EXPECT_TRUE(this->p->ptr); + EXPECT_TRUE(this->p); opendal_operator_options_free(options); } diff --git a/bindings/c/tests/error_msg.cpp b/bindings/c/tests/error_msg.cpp index 65e02cb5a9fe..fe693e98aebe 100644 --- a/bindings/c/tests/error_msg.cpp +++ b/bindings/c/tests/error_msg.cpp @@ -36,7 +36,7 @@ class OpendalErrorMsgTest : public ::testing::Test { EXPECT_TRUE(result.error == nullptr); this->p = result.operator_ptr; - EXPECT_TRUE(this->p->ptr); + EXPECT_TRUE(this->p); opendal_operator_options_free(options); } diff --git a/bindings/c/tests/list.cpp b/bindings/c/tests/list.cpp index 0ebeac4f9bde..7b003e6a61fb 100644 --- a/bindings/c/tests/list.cpp +++ b/bindings/c/tests/list.cpp @@ -39,7 +39,7 @@ class OpendalListTest : public ::testing::Test { EXPECT_TRUE(result.error == nullptr); this->p = result.operator_ptr; - EXPECT_TRUE(this->p->ptr); + EXPECT_TRUE(this->p); opendal_operator_options_free(options); } From 2db19d88dfc822954e0995db2be75cff3f0a3387 Mon Sep 17 00:00:00 2001 From: Ji-Xinyou Date: Mon, 9 Oct 2023 18:24:19 -0400 Subject: [PATCH 11/19] add .gitignore --- bindings/c/.gitignore | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/bindings/c/.gitignore b/bindings/c/.gitignore index 48efa3d51f8b..2e5906714f53 100644 --- a/bindings/c/.gitignore +++ b/bindings/c/.gitignore @@ -1,6 +1,8 @@ build/ +docs/ + +*.o -# Ignore everything under 'examples' examples/* !examples/ !examples/*.h From 858f99ae6d37fd1251967993b576793dfa1e1782 Mon Sep 17 00:00:00 2001 From: Ji-Xinyou Date: Mon, 9 Oct 2023 20:07:07 -0400 Subject: [PATCH 12/19] add error_msg tests to valgrind --- bindings/c/Makefile | 1 + 1 file changed, 1 insertion(+) diff --git a/bindings/c/Makefile b/bindings/c/Makefile index 9e9032e6745a..e10f47bdf4b0 100644 --- a/bindings/c/Makefile +++ b/bindings/c/Makefile @@ -53,6 +53,7 @@ test: memory_leak: $(VALGRIND) $(OBJ_DIR)/bdd $(VALGRIND) $(OBJ_DIR)/list + $(VALGRIND) $(OBJ_DIR)/error_msg .PHONY: doc doc: From 6a5bd9c1bb1d7e3312f8ad22845169246252698d Mon Sep 17 00:00:00 2001 From: Ji-Xinyou Date: Tue, 10 Oct 2023 01:20:41 -0400 Subject: [PATCH 13/19] rename basicrw.c to basic.c --- bindings/c/examples/.gitignore | 1 - bindings/c/examples/{basicrw.c => basic.c} | 0 2 files changed, 1 deletion(-) delete mode 100644 bindings/c/examples/.gitignore rename bindings/c/examples/{basicrw.c => basic.c} (100%) diff --git a/bindings/c/examples/.gitignore b/bindings/c/examples/.gitignore deleted file mode 100644 index 887b0251a8fe..000000000000 --- a/bindings/c/examples/.gitignore +++ /dev/null @@ -1 +0,0 @@ -basicrw diff --git a/bindings/c/examples/basicrw.c b/bindings/c/examples/basic.c similarity index 100% rename from bindings/c/examples/basicrw.c rename to bindings/c/examples/basic.c From 9490b3751ec04d8a730c9515ec28b6ca922c7c81 Mon Sep 17 00:00:00 2001 From: Ji-Xinyou Date: Tue, 10 Oct 2023 01:29:13 -0400 Subject: [PATCH 14/19] resolve comments --- .../c/examples/{error_msg.c => error_handle.c} | 5 +++-- bindings/c/src/error.rs | 17 +++++++++-------- bindings/c/src/lib.rs | 2 +- bindings/c/tests/error_msg.cpp | 5 +++-- 4 files changed, 16 insertions(+), 13 deletions(-) rename bindings/c/examples/{error_msg.c => error_handle.c} (94%) diff --git a/bindings/c/examples/error_msg.c b/bindings/c/examples/error_handle.c similarity index 94% rename from bindings/c/examples/error_msg.c rename to bindings/c/examples/error_handle.c index 7a6d11697b7e..3bb3308e7978 100644 --- a/bindings/c/examples/error_msg.c +++ b/bindings/c/examples/error_handle.c @@ -34,6 +34,7 @@ int main() /* The read is supposed to fail */ opendal_result_read r = opendal_operator_blocking_read(op, "/testpath"); assert(r.error != NULL); + assert(r.error->code == OPENDAL_NOT_FOUND); /* Lets print the error message out */ struct opendal_bytes* error_msg = &r.error->message; @@ -41,9 +42,9 @@ int main() printf("%c", error_msg->data[i]); } - /* free the error */ + /* free the error since the error is not NULL */ opendal_error_free(r.error); /* the operator_ptr is also heap allocated */ opendal_operator_free(op); -} \ No newline at end of file +} diff --git a/bindings/c/src/error.rs b/bindings/c/src/error.rs index 536991d949da..aa1fd56c4d13 100644 --- a/bindings/c/src/error.rs +++ b/bindings/c/src/error.rs @@ -19,9 +19,11 @@ use ::opendal as od; use crate::types::opendal_bytes; -/// The wrapper type for opendal's error, wrapped because of the -/// orphan rule -struct opendal_internal_error(od::Error); +/// \brief The wrapper type for opendal's Rust core error, wrapped because of the +/// orphan rule. +/// +/// \note User should never use this type directly, use [`opendal_error`] instead. +struct raw_error(od::Error); /// \brief The error code for all opendal APIs in C binding. /// \todo The error handling is not complete, the error with error message will be @@ -50,7 +52,7 @@ pub(crate) enum opendal_code { OPENDAL_IS_SAME_FILE, } -impl opendal_internal_error { +impl raw_error { /// Convert the [`od::ErrorKind`] of [`od::Error`] to [`opendal_code`] pub(crate) fn error_code(&self) -> opendal_code { let e = &self.0; @@ -99,16 +101,15 @@ impl opendal_error { // The caller should sink the error to heap memory and return the pointer // that will not be freed by rustc pub(crate) fn from_opendal_error(error: od::Error) -> Self { - let error = opendal_internal_error(error); + let error = raw_error(error); let code = error.error_code(); let c_str = format!("{}", error.0); let message = opendal_bytes::new(c_str.into_bytes()); opendal_error { code, message } } - pub(crate) fn manual_error(code: opendal_code, message: impl Into) -> Self { - let message_str = message.into(); - let message = opendal_bytes::new(message_str.into_bytes()); + pub(crate) fn manual_error(code: opendal_code, message: String) -> Self { + let message = opendal_bytes::new(message.into_bytes()); opendal_error { code, message } } diff --git a/bindings/c/src/lib.rs b/bindings/c/src/lib.rs index 1d0fbe5072ce..013e2a307359 100644 --- a/bindings/c/src/lib.rs +++ b/bindings/c/src/lib.rs @@ -92,7 +92,7 @@ pub unsafe extern "C" fn opendal_operator_new( if scheme.is_null() { let error = opendal_error::manual_error( error::opendal_code::OPENDAL_CONFIG_INVALID, - "The scheme given is pointing at NULL", + "The scheme given is pointing at NULL".into(), ); let result = opendal_result_operator_new { operator_ptr: std::ptr::null_mut(), diff --git a/bindings/c/tests/error_msg.cpp b/bindings/c/tests/error_msg.cpp index fe693e98aebe..3d06f4532dc5 100644 --- a/bindings/c/tests/error_msg.cpp +++ b/bindings/c/tests/error_msg.cpp @@ -22,7 +22,7 @@ extern "C" { #include "opendal.h" } -class OpendalErrorMsgTest : public ::testing::Test { +class OpendalErrorTest : public ::testing::Test { protected: const opendal_operator_ptr* p; @@ -45,12 +45,13 @@ class OpendalErrorMsgTest : public ::testing::Test { }; // Test no memory leak of error message -TEST_F(OpendalErrorMsgTest, ErrorReadTest) +TEST_F(OpendalErrorTest, ErrorReadTest) { // Initialize a operator for "memory" backend, with no options // The read is supposed to fail opendal_result_read r = opendal_operator_blocking_read(this->p, "/testpath"); ASSERT_NE(r.error, nullptr); + ASSERT_EQ(r.error->code, OPENDAL_NOT_FOUND); // Lets check the error message out struct opendal_bytes* error_msg = &r.error->message; From 3207a8691a3e4ec33d59964a8768a3bbf73637f3 Mon Sep 17 00:00:00 2001 From: Ji-Xinyou Date: Tue, 10 Oct 2023 01:44:34 -0400 Subject: [PATCH 15/19] fix fmt --- bindings/c/src/error.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bindings/c/src/error.rs b/bindings/c/src/error.rs index aa1fd56c4d13..366d646ae0a4 100644 --- a/bindings/c/src/error.rs +++ b/bindings/c/src/error.rs @@ -21,7 +21,7 @@ use crate::types::opendal_bytes; /// \brief The wrapper type for opendal's Rust core error, wrapped because of the /// orphan rule. -/// +/// /// \note User should never use this type directly, use [`opendal_error`] instead. struct raw_error(od::Error); From ee1413cc2331f2de98a7857bbcc997b2b74b652e Mon Sep 17 00:00:00 2001 From: Xuanwo Date: Fri, 13 Oct 2023 17:22:47 +0800 Subject: [PATCH 16/19] Fix zig Signed-off-by: Xuanwo --- bindings/zig/README.md | 2 +- bindings/zig/src/opendal.zig | 3 --- bindings/zig/test/bdd.zig | 14 ++++++++------ 3 files changed, 9 insertions(+), 10 deletions(-) diff --git a/bindings/zig/README.md b/bindings/zig/README.md index 7f25eb470fe0..315c686cb2bf 100644 --- a/bindings/zig/README.md +++ b/bindings/zig/README.md @@ -16,7 +16,7 @@ To compile OpenDAL from source code, you need: # build libopendal_c (underneath call make -C ../c) zig build libopendal_c # build and run unit tests -zig build test -fsummary +zig build test --summary all ``` ## License diff --git a/bindings/zig/src/opendal.zig b/bindings/zig/src/opendal.zig index 93c5047428b0..02bfc41e90c9 100644 --- a/bindings/zig/src/opendal.zig +++ b/bindings/zig/src/opendal.zig @@ -19,8 +19,6 @@ pub const c = @cImport(@cInclude("opendal.h")); // Zig code get values C code pub const Code = enum(c.opendal_code) { - OK = c.OPENDAL_OK, - ERROR = c.OPENDAL_ERROR, UNEXPECTED = c.OPENDAL_UNEXPECTED, UNSUPPORTED = c.OPENDAL_UNSUPPORTED, CONFIG_INVALID = c.OPENDAL_CONFIG_INVALID, @@ -58,7 +56,6 @@ pub fn codeToError(code: c.opendal_code) OpendalError!c.opendal_code { c.OPENDAL_ALREADY_EXISTS => error.AlreadyExists, c.OPENDAL_RATE_LIMITED => error.RateLimited, c.OPENDAL_IS_SAME_FILE => error.IsSameFile, - else => c.OPENDAL_ERROR, }; } pub fn errorToCode(err: OpendalError) c_int { diff --git a/bindings/zig/test/bdd.zig b/bindings/zig/test/bdd.zig index 79748fa3b196..c835e45b3400 100644 --- a/bindings/zig/test/bdd.zig +++ b/bindings/zig/test/bdd.zig @@ -40,7 +40,9 @@ test "Opendal BDD test" { opendal.c.opendal_operator_options_set(options, "root", "/myroot"); // Given A new OpenDAL Blocking Operator - self.p = opendal.c.opendal_operator_new(self.scheme, options); + var result = opendal.c.opendal_operator_new(self.scheme, options); + testing.expectEqual(result.@"error", null) catch unreachable; + self.p = result.operator_ptr; return self; } @@ -61,17 +63,17 @@ test "Opendal BDD test" { // c_str does not have len field (.* is ptr) .len = std.mem.len(testkit.content), }; - const code = opendal.c.opendal_operator_blocking_write(testkit.p, testkit.path, data); - try testing.expectEqual(code, @intFromEnum(Code.OK)); + const result = opendal.c.opendal_operator_blocking_write(testkit.p, testkit.path, data); + try testing.expectEqual(result, null); // The blocking file "test" should exist var e: opendal.c.opendal_result_is_exist = opendal.c.opendal_operator_is_exist(testkit.p, testkit.path); - try testing.expectEqual(e.code, @intFromEnum(Code.OK)); + try testing.expectEqual(e.@"error", null); try testing.expect(e.is_exist); // The blocking file "test" entry mode must be file var s: opendal.c.opendal_result_stat = opendal.c.opendal_operator_stat(testkit.p, testkit.path); - try testing.expectEqual(s.code, @intFromEnum(Code.OK)); + try testing.expectEqual(s.@"error", null); var meta: [*c]opendal.c.opendal_metadata = s.meta; try testing.expect(opendal.c.opendal_metadata_is_file(meta)); @@ -82,7 +84,7 @@ test "Opendal BDD test" { // The blocking file "test" must have content "Hello, World!" var r: opendal.c.opendal_result_read = opendal.c.opendal_operator_blocking_read(testkit.p, testkit.path); defer opendal.c.opendal_bytes_free(r.data); - try testing.expect(r.code == @intFromEnum(Code.OK)); + try testing.expect(r.@"error" == null); try testing.expectEqual(std.mem.len(testkit.content), r.data.*.len); var count: usize = 0; From f2c6e99c9fe0cad404dd337846f982f38d27682a Mon Sep 17 00:00:00 2001 From: Xuanwo Date: Fri, 13 Oct 2023 17:45:09 +0800 Subject: [PATCH 17/19] Fix go Signed-off-by: Xuanwo --- bindings/go/opendal.go | 37 ++++++++++++++++++++++++++++--------- bindings/go/opendal_test.go | 4 ++-- 2 files changed, 30 insertions(+), 11 deletions(-) diff --git a/bindings/go/opendal.go b/bindings/go/opendal.go index 1494ab170837..4677a99dc675 100644 --- a/bindings/go/opendal.go +++ b/bindings/go/opendal.go @@ -49,9 +49,14 @@ func NewOperator(scheme string, opt Options) (*Operator, error) { for k, v := range opt { C.opendal_operator_options_set(opts, C.CString(k), C.CString(v)) } - op := C.opendal_operator_new(C.CString(scheme), opts) + ret := C.opendal_operator_new(C.CString(scheme), opts) + if ret.error != nil { + defer C.opendal_error_free(ret.error) + code, message := parseError(ret.error) + return nil, errors.New(fmt.Sprintf("create operator failed, error code: %d, error message: %s", code, message)) + } return &Operator{ - inner: op, + inner: ret.operator_ptr, }, nil } @@ -61,22 +66,36 @@ func (o *Operator) Write(key string, value []byte) error { } bytes := C.opendal_bytes{data: (*C.uchar)(unsafe.Pointer(&value[0])), len: C.ulong(len(value))} ret := C.opendal_operator_blocking_write(o.inner, C.CString(key), bytes) - if ret != 0 { - return errors.New(fmt.Sprintf("write failed, error code: %d", ret)) + if ret != nil { + defer C.opendal_error_free(ret) + code, message := parseError(ret) + return errors.New(fmt.Sprintf("write failed, error code: %d, error message: %s", code, message)) } return nil } func (o *Operator) Read(key string) ([]byte, error) { - result := C.opendal_operator_blocking_read(o.inner, C.CString(key)) - ret := int(result.code) - if ret != 0 { - return nil, errors.New(fmt.Sprintf("write failed, error code: %d", ret)) + ret := C.opendal_operator_blocking_read(o.inner, C.CString(key)) + if ret.error != nil { + defer C.opendal_error_free(ret.error) + code, message := parseError(ret.error) + return nil, errors.New(fmt.Sprintf("read failed, error code: %d, error message: %s", code, message)) } - return C.GoBytes(unsafe.Pointer(result.data.data), C.int(result.data.len)), nil + return C.GoBytes(unsafe.Pointer(ret.data.data), C.int(ret.data.len)), nil } func (o *Operator) Close() error { C.opendal_operator_free(o.inner) return nil } + +func decodeBytes(bs C.opendal_bytes) []byte { + return C.GoBytes(unsafe.Pointer(bs.data), C.int(bs.len)) +} + +func parseError(err *C.opendal_error) (int, string) { + code := int(err.code) + message := string(decodeBytes(err.message)) + + return code, message +} diff --git a/bindings/go/opendal_test.go b/bindings/go/opendal_test.go index 6723b67e2ce4..9ac76c273503 100644 --- a/bindings/go/opendal_test.go +++ b/bindings/go/opendal_test.go @@ -38,7 +38,7 @@ func TestOperations(t *testing.T) { assert.NoError(t, err) assert.Equal(t, "Hello World from OpenDAL CGO!", string(value)) - println(string(value)) + println(string(value)) } func BenchmarkOperator_Write(b *testing.B) { @@ -50,7 +50,7 @@ func BenchmarkOperator_Write(b *testing.B) { bs := bytes.Repeat([]byte("a"), 1024*1024) op.Write("test", bs) - b.SetBytes(1024*1024) + b.SetBytes(1024 * 1024) b.ResetTimer() for i := 0; i < b.N; i++ { op.Read("test") From 0b58540c7544e6ef0bf7d9adb2e05b17c3ed4e97 Mon Sep 17 00:00:00 2001 From: Xuanwo Date: Fri, 13 Oct 2023 19:55:56 +0800 Subject: [PATCH 18/19] Fix swift Signed-off-by: Xuanwo --- .../Sources/OpenDAL/Data+OpenDAL.swift | 18 ++-- .../OpenDAL/Sources/OpenDAL/Operator.swift | 87 ++++++++++--------- .../Tests/OpenDALTests/OpenDALTests.swift | 13 ++- 3 files changed, 58 insertions(+), 60 deletions(-) diff --git a/bindings/swift/OpenDAL/Sources/OpenDAL/Data+OpenDAL.swift b/bindings/swift/OpenDAL/Sources/OpenDAL/Data+OpenDAL.swift index 92d0c827095c..556b634d08bf 100644 --- a/bindings/swift/OpenDAL/Sources/OpenDAL/Data+OpenDAL.swift +++ b/bindings/swift/OpenDAL/Sources/OpenDAL/Data+OpenDAL.swift @@ -25,15 +25,13 @@ 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) { - guard let address = UnsafeRawPointer(openDALBytes.pointee.data) else { - return nil - } - let length = Int(openDALBytes.pointee.len) - self.init(bytesNoCopy: .init(mutating: address), - count: length, - deallocator: .custom({ _, _ in - opendal_bytes_free(openDALBytes) - })) + init(openDALBytes: UnsafeMutablePointer) { +let address = UnsafeRawPointer(openDALBytes.pointee.data)! + let length = Int(openDALBytes.pointee.len) + self.init(bytesNoCopy: .init(mutating: address), + count: length, + deallocator: .custom({ _, _ in + opendal_bytes_free(openDALBytes) + })) } } diff --git a/bindings/swift/OpenDAL/Sources/OpenDAL/Operator.swift b/bindings/swift/OpenDAL/Sources/OpenDAL/Operator.swift index 1f2f91d1a4b7..4482670b25fa 100644 --- a/bindings/swift/OpenDAL/Sources/OpenDAL/Operator.swift +++ b/bindings/swift/OpenDAL/Sources/OpenDAL/Operator.swift @@ -18,72 +18,75 @@ import Foundation import COpenDAL -public enum OperatorError: Error { - case failedToBuild - case operationError(UInt32) +public struct OperatorError: Error { + let code: UInt32 + let message: Data } -/// A type used to access almost all OpenDAL APIs. public class Operator { var nativeOp: UnsafePointer - + deinit { opendal_operator_free(nativeOp) } - - /// Creates an operator with the given options. - /// - /// - Parameter options: The option map for creating the operator. - /// - Throws: `OperatorError` value that indicates an error if failed. - public init(scheme: String, options: [String : String] = [:]) throws { + + public init(scheme: String, options: [String: String] = [:]) throws { let nativeOptions = opendal_operator_options_new() defer { opendal_operator_options_free(nativeOptions) } - + for option in options { opendal_operator_options_set(nativeOptions, option.key, option.value) } - - guard let nativeOp = opendal_operator_new(scheme, nativeOptions) else { - throw OperatorError.failedToBuild - } - - guard nativeOp.pointee.ptr != nil else { - throw OperatorError.failedToBuild + + let ret = opendal_operator_new(scheme, nativeOptions) + if let err = ret.error { + defer { + opendal_error_free(err) + } + let immutableErr = err.pointee + let messagePointer = withUnsafePointer(to: immutableErr.message) { $0 } + let messageLength = Int(immutableErr.message.len) + throw OperatorError(code: immutableErr.code.rawValue, + message: Data(bytes: messagePointer, count: messageLength)) } - self.nativeOp = nativeOp + self.nativeOp = UnsafePointer(ret.operator_ptr)! } - - /// Blockingly write the data to a given path. - /// - /// - Parameter data: The content to be written. - /// - Parameter path: The destination path for writing the data. - /// - Throws: `OperatorError` value that indicates an error if failed to write. + public func blockingWrite(_ data: Data, to path: String) throws { - let code = data.withUnsafeBytes { dataPointer in + let ret = data.withUnsafeBytes { dataPointer in let address = dataPointer.baseAddress!.assumingMemoryBound(to: UInt8.self) let bytes = opendal_bytes(data: address, len: UInt(dataPointer.count)) return opendal_operator_blocking_write(nativeOp, path, bytes) } - - guard code == OPENDAL_OK else { - throw OperatorError.operationError(code.rawValue) + + if let err = ret { + defer { + opendal_error_free(err) + } + let immutableErr = err.pointee + let messagePointer = withUnsafePointer(to: immutableErr.message) { $0 } + let messageLength = Int(immutableErr.message.len) + throw OperatorError(code: immutableErr.code.rawValue, + message: Data(bytes: messagePointer, count: messageLength)) } } - - /// Blockingly read the data from a given path. - /// - /// - Parameter path: Path of the data to read. - /// - Returns: `NativeData` object if the data exists. - /// - Throws: `OperatorError` value that indicates an error if failed to read. - public func blockingRead(_ path: String) throws -> Data? { - let result = opendal_operator_blocking_read(nativeOp, path) - guard result.code == OPENDAL_OK else { - throw OperatorError.operationError(result.code.rawValue) + + public func blockingRead(_ path: String) throws -> Data { + let ret = opendal_operator_blocking_read(nativeOp, path) + if let err = ret.error { + defer { + opendal_error_free(err) + } + let immutableErr = err.pointee + let messagePointer = withUnsafePointer(to: immutableErr.message) { $0 } + let messageLength = Int(immutableErr.message.len) + throw OperatorError(code: immutableErr.code.rawValue, + message: Data(bytes: messagePointer, count: messageLength)) } - - return .init(openDALBytes: result.data) + + return Data(openDALBytes: ret.data) } } diff --git a/bindings/swift/OpenDAL/Tests/OpenDALTests/OpenDALTests.swift b/bindings/swift/OpenDAL/Tests/OpenDALTests/OpenDALTests.swift index a776efdbdbb3..6508e0ac5874 100644 --- a/bindings/swift/OpenDAL/Tests/OpenDALTests/OpenDALTests.swift +++ b/bindings/swift/OpenDAL/Tests/OpenDALTests/OpenDALTests.swift @@ -23,19 +23,16 @@ final class OpenDALTests: XCTestCase { let op = try Operator(scheme: "memory", options: [ "root": "/myroot" ]) - + let testContents = Data([1, 2, 3, 4]) try op.blockingWrite(testContents, to: "test") - - guard let readContents = try op.blockingRead("test") else { - XCTFail("Expected a `Data`") - return - } - + + let readContents = try op.blockingRead("test") + for (testByte, readByte) in zip(testContents, readContents) { XCTAssertEqual(testByte, readByte) } - + XCTAssertThrowsError(try op.blockingRead("test_not_exists")) } } From 35e0340b9890a415c3d5fb189353c6bb61c058ab Mon Sep 17 00:00:00 2001 From: Xuanwo Date: Fri, 13 Oct 2023 20:10:36 +0800 Subject: [PATCH 19/19] Format code Signed-off-by: Xuanwo --- bindings/swift/.swift-format | 11 ++++++++++ bindings/swift/OpenDAL/Package.swift | 11 ++++++---- .../Sources/OpenDAL/Data+OpenDAL.swift | 18 +++++++++-------- .../OpenDAL/Sources/OpenDAL/Operator.swift | 20 ++++++++++++------- .../Tests/OpenDALTests/OpenDALTests.swift | 10 +++++++--- 5 files changed, 48 insertions(+), 22 deletions(-) create mode 100644 bindings/swift/.swift-format diff --git a/bindings/swift/.swift-format b/bindings/swift/.swift-format new file mode 100644 index 000000000000..d5d1e0158f7d --- /dev/null +++ b/bindings/swift/.swift-format @@ -0,0 +1,11 @@ +{ + "version": 1, + "lineLength": 100, + "indentation": { + "spaces": 4 + }, + "maximumBlankLines": 1, + "respectsExistingLineBreaks": true, + "lineBreakBeforeControlFlowKeywords": true, + "lineBreakBeforeEachArgument": true +} diff --git a/bindings/swift/OpenDAL/Package.swift b/bindings/swift/OpenDAL/Package.swift index d31439f32cca..f861d76979ba 100644 --- a/bindings/swift/OpenDAL/Package.swift +++ b/bindings/swift/OpenDAL/Package.swift @@ -27,7 +27,8 @@ let package = Package( products: [ .library( name: "OpenDAL", - targets: ["OpenDAL"]), + targets: ["OpenDAL"] + ) ], targets: [ .systemLibrary(name: "COpenDAL"), @@ -35,10 +36,12 @@ let package = Package( name: "OpenDAL", dependencies: ["COpenDAL"], linkerSettings: [ - .unsafeFlags(["-L\(packageRoot)/Sources/COpenDAL/lib"]), - ]), + .unsafeFlags(["-L\(packageRoot)/Sources/COpenDAL/lib"]) + ] + ), .testTarget( name: "OpenDALTests", - dependencies: ["OpenDAL"]), + dependencies: ["OpenDAL"] + ), ] ) diff --git a/bindings/swift/OpenDAL/Sources/OpenDAL/Data+OpenDAL.swift b/bindings/swift/OpenDAL/Sources/OpenDAL/Data+OpenDAL.swift index 556b634d08bf..8e6193375469 100644 --- a/bindings/swift/OpenDAL/Sources/OpenDAL/Data+OpenDAL.swift +++ b/bindings/swift/OpenDAL/Sources/OpenDAL/Data+OpenDAL.swift @@ -15,8 +15,8 @@ // specific language governing permissions and limitations // under the License. -import Foundation import COpenDAL +import Foundation extension Data { /// Creates a new data by managing `opendal_bytes` as its @@ -26,12 +26,14 @@ extension Data { /// The underlying buffer will be freed when the data gets /// deallocated. init(openDALBytes: UnsafeMutablePointer) { -let address = UnsafeRawPointer(openDALBytes.pointee.data)! - let length = Int(openDALBytes.pointee.len) - self.init(bytesNoCopy: .init(mutating: address), - count: length, - deallocator: .custom({ _, _ in - opendal_bytes_free(openDALBytes) - })) + let address = UnsafeRawPointer(openDALBytes.pointee.data)! + let length = Int(openDALBytes.pointee.len) + self.init( + bytesNoCopy: .init(mutating: address), + count: length, + deallocator: .custom({ _, _ in + opendal_bytes_free(openDALBytes) + }) + ) } } diff --git a/bindings/swift/OpenDAL/Sources/OpenDAL/Operator.swift b/bindings/swift/OpenDAL/Sources/OpenDAL/Operator.swift index 4482670b25fa..6104c17c15d5 100644 --- a/bindings/swift/OpenDAL/Sources/OpenDAL/Operator.swift +++ b/bindings/swift/OpenDAL/Sources/OpenDAL/Operator.swift @@ -15,8 +15,8 @@ // specific language governing permissions and limitations // under the License. -import Foundation import COpenDAL +import Foundation public struct OperatorError: Error { let code: UInt32 @@ -48,8 +48,10 @@ public class Operator { let immutableErr = err.pointee let messagePointer = withUnsafePointer(to: immutableErr.message) { $0 } let messageLength = Int(immutableErr.message.len) - throw OperatorError(code: immutableErr.code.rawValue, - message: Data(bytes: messagePointer, count: messageLength)) + throw OperatorError( + code: immutableErr.code.rawValue, + message: Data(bytes: messagePointer, count: messageLength) + ) } self.nativeOp = UnsafePointer(ret.operator_ptr)! @@ -69,8 +71,10 @@ public class Operator { let immutableErr = err.pointee let messagePointer = withUnsafePointer(to: immutableErr.message) { $0 } let messageLength = Int(immutableErr.message.len) - throw OperatorError(code: immutableErr.code.rawValue, - message: Data(bytes: messagePointer, count: messageLength)) + throw OperatorError( + code: immutableErr.code.rawValue, + message: Data(bytes: messagePointer, count: messageLength) + ) } } @@ -83,8 +87,10 @@ public class Operator { let immutableErr = err.pointee let messagePointer = withUnsafePointer(to: immutableErr.message) { $0 } let messageLength = Int(immutableErr.message.len) - throw OperatorError(code: immutableErr.code.rawValue, - message: Data(bytes: messagePointer, count: messageLength)) + throw OperatorError( + code: immutableErr.code.rawValue, + message: Data(bytes: messagePointer, count: messageLength) + ) } return Data(openDALBytes: ret.data) diff --git a/bindings/swift/OpenDAL/Tests/OpenDALTests/OpenDALTests.swift b/bindings/swift/OpenDAL/Tests/OpenDALTests/OpenDALTests.swift index 6508e0ac5874..ef17c7887486 100644 --- a/bindings/swift/OpenDAL/Tests/OpenDALTests/OpenDALTests.swift +++ b/bindings/swift/OpenDAL/Tests/OpenDALTests/OpenDALTests.swift @@ -16,13 +16,17 @@ // under the License. import XCTest + @testable import OpenDAL final class OpenDALTests: XCTestCase { func testSimple() throws { - let op = try Operator(scheme: "memory", options: [ - "root": "/myroot" - ]) + let op = try Operator( + scheme: "memory", + options: [ + "root": "/myroot" + ] + ) let testContents = Data([1, 2, 3, 4]) try op.blockingWrite(testContents, to: "test")