From 2a332f3f154d6b3d24f289d8a31b990c8bca2082 Mon Sep 17 00:00:00 2001 From: tison Date: Mon, 14 Oct 2024 03:15:26 -0600 Subject: [PATCH 1/2] refactor: resolve c pointers const (#5171) * refactor: resolve c pointers const Signed-off-by: tison * fix zig cases Signed-off-by: tison * fix swift cases Signed-off-by: tison * fix go cases Signed-off-by: tison * pass pointer to opendal_bytes_free Signed-off-by: tison * try fix go case Signed-off-by: tison * fix(bindings/go): make opendal_bytes pointer Signed-off-by: Hanchin Hsieh --------- Signed-off-by: tison Signed-off-by: Hanchin Hsieh Co-authored-by: Hanchin Hsieh --- bindings/c/README.md | 8 +-- bindings/c/examples/basic.c | 8 +-- bindings/c/include/opendal.h | 17 +++--- bindings/c/src/error.rs | 16 +----- bindings/c/src/operator.rs | 22 ++++---- bindings/c/src/result.rs | 2 +- bindings/c/src/types.rs | 52 ++++++++++++------- bindings/c/src/writer.rs | 2 +- bindings/c/tests/bdd.cpp | 12 ++--- bindings/c/tests/error_msg.cpp | 2 +- bindings/c/tests/list.cpp | 2 +- bindings/go/error.go | 2 +- bindings/go/reader.go | 9 ++-- bindings/go/stat.go | 1 - bindings/go/types.go | 14 ++--- bindings/go/write.go | 4 +- .../OpenDAL/Sources/OpenDAL/Operator.swift | 14 ++--- .../Tests/OpenDALTests/OpenDALTests.swift | 4 +- bindings/zig/test/bdd.zig | 22 ++++---- 19 files changed, 107 insertions(+), 106 deletions(-) diff --git a/bindings/c/README.md b/bindings/c/README.md index bd225ceb14b9..398f1a1f198b 100644 --- a/bindings/c/README.md +++ b/bindings/c/README.md @@ -32,18 +32,18 @@ int main() /* We can read it out, make sure the data is the same */ opendal_result_read r = opendal_operator_read(op, "/testpath"); - opendal_bytes* read_bytes = r.data; + opendal_bytes read_bytes = r.data; assert(r.error == NULL); - assert(read_bytes->len == 24); + assert(read_bytes.len == 24); /* Lets print it out */ for (int i = 0; i < 24; ++i) { - printf("%c", read_bytes->data[i]); + printf("%c", read_bytes.data[i]); } printf("\n"); /* the opendal_bytes read is heap allocated, please free it */ - opendal_bytes_free(read_bytes); + opendal_bytes_free(&read_bytes); /* the operator_ptr is also heap allocated */ opendal_operator_free(&op); diff --git a/bindings/c/examples/basic.c b/bindings/c/examples/basic.c index 5e4e7925ba77..af55faeea0df 100644 --- a/bindings/c/examples/basic.c +++ b/bindings/c/examples/basic.c @@ -42,18 +42,18 @@ int main() /* We can read it out, make sure the data is the same */ opendal_result_read r = opendal_operator_read(op, "/testpath"); - opendal_bytes* read_bytes = r.data; + opendal_bytes read_bytes = r.data; assert(r.error == NULL); - assert(read_bytes->len == 24); + assert(read_bytes.len == 24); /* Lets print it out */ for (int i = 0; i < 24; ++i) { - printf("%c", read_bytes->data[i]); + printf("%c", read_bytes.data[i]); } printf("\n"); /* the opendal_bytes read is heap allocated, please free it */ - opendal_bytes_free(read_bytes); + opendal_bytes_free(&read_bytes); /* the operator_ptr is also heap allocated */ opendal_operator_free(op); diff --git a/bindings/c/include/opendal.h b/bindings/c/include/opendal.h index 4fba90916344..8eeacf7c5121 100644 --- a/bindings/c/include/opendal.h +++ b/bindings/c/include/opendal.h @@ -94,7 +94,7 @@ typedef struct opendal_bytes { /** * Pointing to the byte array on heap */ - const uint8_t *data; + uint8_t *data; /** * The length of the byte array */ @@ -113,7 +113,7 @@ typedef struct opendal_bytes { * represents no error has taken placed**. If any error has taken place, the caller should check * the error code and print the error message. * - * The error code is represented in opendal_code, which is a enum on different type of errors. + * The error code is represented in opendal_code, which is an enum on different type of errors. * The error messages is represented in opendal_bytes, which is a non-null terminated byte array. * * \note 1. The error message is on heap, so the error needs to be freed by the caller, by calling @@ -217,7 +217,7 @@ typedef struct opendal_operator { * The pointer to the opendal::BlockingOperator in the Rust code. * Only touch this on judging whether it is NULL. */ - const void *inner; + void *inner; } opendal_operator; /** @@ -273,7 +273,7 @@ typedef struct opendal_result_read { /** * The byte array with length returned by read operations */ - struct opendal_bytes *data; + struct opendal_bytes data; /** * The error, if ok, it is null */ @@ -814,7 +814,7 @@ struct opendal_result_operator_new opendal_operator_new(const char *scheme, */ struct opendal_error *opendal_operator_write(const struct opendal_operator *op, const char *path, - struct opendal_bytes bytes); + const struct opendal_bytes *bytes); /** * \brief Blocking read the data from `path`. @@ -842,8 +842,9 @@ struct opendal_error *opendal_operator_write(const struct opendal_operator *op, * opendal_result_read r = opendal_operator_read(op, "testpath"); * assert(r.error == NULL); * - * opendal_bytes *bytes = r.data; - * assert(bytes->len == 13); + * opendal_bytes bytes = r.data; + * assert(bytes.len == 13); + * opendal_bytes_free(&bytes); * ``` * * # Safety @@ -1394,7 +1395,7 @@ void opendal_reader_free(struct opendal_reader *ptr); * \brief Write data to the writer. */ struct opendal_result_writer_write opendal_writer_write(struct opendal_writer *self, - struct opendal_bytes bytes); + const struct opendal_bytes *bytes); /** * \brief Frees the heap memory used by the opendal_writer. diff --git a/bindings/c/src/error.rs b/bindings/c/src/error.rs index d1c6155ee72e..97388ddfe240 100644 --- a/bindings/c/src/error.rs +++ b/bindings/c/src/error.rs @@ -82,7 +82,7 @@ impl From for opendal_code { /// represents no error has taken placed**. If any error has taken place, the caller should check /// the error code and print the error message. /// -/// The error code is represented in opendal_code, which is a enum on different type of errors. +/// The error code is represented in opendal_code, which is an enum on different type of errors. /// The error messages is represented in opendal_bytes, which is a non-null terminated byte array. /// /// \note 1. The error message is on heap, so the error needs to be freed by the caller, by calling @@ -114,19 +114,7 @@ impl opendal_error { #[no_mangle] pub unsafe extern "C" fn opendal_error_free(ptr: *mut opendal_error) { if !ptr.is_null() { - let message_ptr = &(*ptr).message; - let message_ptr = message_ptr as *const opendal_bytes as *mut opendal_bytes; - if !message_ptr.is_null() { - let data_mut = (*message_ptr).data as *mut u8; - drop(Vec::from_raw_parts( - data_mut, - (*message_ptr).len, - (*message_ptr).len, - )); - } - - // free the pointer - drop(Box::from_raw(ptr)) + drop(Box::from_raw(ptr)); } } } diff --git a/bindings/c/src/operator.rs b/bindings/c/src/operator.rs index 04df68ec455f..ba03f9c2ae92 100644 --- a/bindings/c/src/operator.rs +++ b/bindings/c/src/operator.rs @@ -47,7 +47,7 @@ static RUNTIME: Lazy = Lazy::new(|| { pub struct opendal_operator { /// The pointer to the opendal::BlockingOperator in the Rust code. /// Only touch this on judging whether it is NULL. - inner: *const c_void, + inner: *mut c_void, } impl opendal_operator { @@ -226,7 +226,7 @@ pub unsafe extern "C" fn opendal_operator_new( pub unsafe extern "C" fn opendal_operator_write( op: &opendal_operator, path: *const c_char, - bytes: opendal_bytes, + bytes: &opendal_bytes, ) -> *mut opendal_error { assert!(!path.is_null()); let path = std::ffi::CStr::from_ptr(path) @@ -263,8 +263,9 @@ pub unsafe extern "C" fn opendal_operator_write( /// opendal_result_read r = opendal_operator_read(op, "testpath"); /// assert(r.error == NULL); /// -/// opendal_bytes *bytes = r.data; -/// assert(bytes->len == 13); +/// opendal_bytes bytes = r.data; +/// assert(bytes.len == 13); +/// opendal_bytes_free(&bytes); /// ``` /// /// # Safety @@ -286,15 +287,12 @@ pub unsafe extern "C" fn opendal_operator_read( .to_str() .expect("malformed path"); match op.deref().read(path) { - Ok(d) => { - let v = Box::new(opendal_bytes::new(d)); - opendal_result_read { - data: Box::into_raw(v), - error: std::ptr::null_mut(), - } - } + Ok(b) => opendal_result_read { + data: opendal_bytes::new(b), + error: std::ptr::null_mut(), + }, Err(e) => opendal_result_read { - data: std::ptr::null_mut(), + data: opendal_bytes::empty(), error: opendal_error::new(e), }, } diff --git a/bindings/c/src/result.rs b/bindings/c/src/result.rs index ba7e1c64a028..e9aede5d47fb 100644 --- a/bindings/c/src/result.rs +++ b/bindings/c/src/result.rs @@ -49,7 +49,7 @@ pub struct opendal_result_operator_new { #[repr(C)] pub struct opendal_result_read { /// The byte array with length returned by read operations - pub data: *mut opendal_bytes, + pub data: opendal_bytes, /// The error, if ok, it is null pub error: *mut opendal_error, } diff --git a/bindings/c/src/types.rs b/bindings/c/src/types.rs index 795c7a73b5c1..cba8f2540063 100644 --- a/bindings/c/src/types.rs +++ b/bindings/c/src/types.rs @@ -31,7 +31,7 @@ use opendal::Buffer; #[repr(C)] pub struct opendal_bytes { /// Pointing to the byte array on heap - pub data: *const u8, + pub data: *mut u8, /// The length of the byte array pub len: usize, /// The capacity of the byte array @@ -39,17 +39,21 @@ pub struct opendal_bytes { } impl opendal_bytes { + pub(crate) fn empty() -> Self { + Self { + data: std::ptr::null_mut(), + len: 0, + capacity: 0, + } + } + /// Construct a [`opendal_bytes`] from the Rust [`Vec`] of bytes - pub(crate) fn new(buf: Buffer) -> Self { - let vec = buf.to_vec(); - let mut buf = std::mem::ManuallyDrop::new(vec); - let data = buf.as_mut_ptr(); - let len = buf.len(); - let capacity = buf.capacity(); + pub(crate) fn new(b: Buffer) -> Self { + let mut b = std::mem::ManuallyDrop::new(b.to_vec()); Self { - data, - len, - capacity, + data: b.as_mut_ptr(), + len: b.len(), + capacity: b.capacity(), } } @@ -57,20 +61,28 @@ impl opendal_bytes { #[no_mangle] pub unsafe extern "C" fn opendal_bytes_free(ptr: *mut opendal_bytes) { if !ptr.is_null() { - // transmuting `*const u8` to `*mut u8` is undefined behavior in any cases - // however, fields type of `opendal_bytes` is already related to the zig binding - // it should be fixed later - let _ = Vec::from_raw_parts((*ptr).data as *mut u8, (*ptr).len, (*ptr).capacity); - // it is too weird that call `Box::new` outside `opendal_bytes::new` but dealloc it here - // also, boxing `opendal_bytes` might not be necessary - // `data` points to heap, so `opendal_bytes` could be passed as a stack value - let _ = Box::from_raw(ptr); + let bs = &mut *ptr; + if !bs.data.is_null() { + drop(Vec::from_raw_parts(bs.data, bs.len, bs.capacity)); + bs.data = std::ptr::null_mut(); + bs.len = 0; + bs.capacity = 0; + } + } + } +} + +impl Drop for opendal_bytes { + fn drop(&mut self) { + unsafe { + // Safety: the pointer is always valid + Self::opendal_bytes_free(self); } } } -impl From for Buffer { - fn from(v: opendal_bytes) -> Self { +impl From<&opendal_bytes> for Buffer { + fn from(v: &opendal_bytes) -> Self { let slice = unsafe { std::slice::from_raw_parts(v.data, v.len) }; Buffer::from(bytes::Bytes::copy_from_slice(slice)) } diff --git a/bindings/c/src/writer.rs b/bindings/c/src/writer.rs index 67ac7d53f45b..a72dbbbb6934 100644 --- a/bindings/c/src/writer.rs +++ b/bindings/c/src/writer.rs @@ -49,7 +49,7 @@ impl opendal_writer { #[no_mangle] pub unsafe extern "C" fn opendal_writer_write( &mut self, - bytes: opendal_bytes, + bytes: &opendal_bytes, ) -> opendal_result_writer_write { let size = bytes.len; match self.deref_mut().write(bytes) { diff --git a/bindings/c/tests/bdd.cpp b/bindings/c/tests/bdd.cpp index 39856e81e5f0..65e97f52bf44 100644 --- a/bindings/c/tests/bdd.cpp +++ b/bindings/c/tests/bdd.cpp @@ -61,7 +61,7 @@ TEST_F(OpendalBddTest, FeatureTest) .data = (uint8_t*)this->content.c_str(), .len = this->content.length(), }; - opendal_error* error = opendal_operator_write(this->p, this->path.c_str(), data); + opendal_error* error = opendal_operator_write(this->p, this->path.c_str(), &data); EXPECT_EQ(error, nullptr); // The blocking file "test" should exist @@ -85,9 +85,9 @@ TEST_F(OpendalBddTest, FeatureTest) // The blocking file "test" must have content "Hello, World!" struct opendal_result_read r = opendal_operator_read(this->p, this->path.c_str()); EXPECT_EQ(r.error, nullptr); - EXPECT_EQ(r.data->len, this->content.length()); - for (int i = 0; i < r.data->len; i++) { - EXPECT_EQ(this->content[i], (char)(r.data->data[i])); + EXPECT_EQ(r.data.len, this->content.length()); + for (int i = 0; i < r.data.len; i++) { + EXPECT_EQ(this->content[i], (char)(r.data.data[i])); } // The blocking file should be deleted @@ -99,7 +99,7 @@ TEST_F(OpendalBddTest, FeatureTest) opendal_result_operator_writer writer = opendal_operator_writer(this->p, this->path.c_str()); EXPECT_EQ(writer.error, nullptr); - opendal_result_writer_write w = opendal_writer_write(writer.writer, data); + opendal_result_writer_write w = opendal_writer_write(writer.writer, &data); EXPECT_EQ(w.error, nullptr); EXPECT_EQ(w.size, this->content.length()); opendal_writer_free(writer.writer); @@ -120,7 +120,7 @@ TEST_F(OpendalBddTest, FeatureTest) error = opendal_operator_delete(this->p, this->path.c_str()); EXPECT_EQ(error, nullptr); - opendal_bytes_free(r.data); + opendal_bytes_free(&r.data); // The directory "tmpdir/" should exist and should be a directory error = opendal_operator_create_dir(this->p, "tmpdir/"); diff --git a/bindings/c/tests/error_msg.cpp b/bindings/c/tests/error_msg.cpp index f2087ea3b0bf..516d27865aaf 100644 --- a/bindings/c/tests/error_msg.cpp +++ b/bindings/c/tests/error_msg.cpp @@ -59,7 +59,7 @@ TEST_F(OpendalErrorTest, ErrorReadTest) ASSERT_GT(error_msg->len, 0); // the opendal_bytes read is heap allocated, please free it - opendal_bytes_free(r.data); + opendal_bytes_free(&r.data); // free the error opendal_error_free(r.error); diff --git a/bindings/c/tests/list.cpp b/bindings/c/tests/list.cpp index d4e31cf79e5b..3b64fef460b6 100644 --- a/bindings/c/tests/list.cpp +++ b/bindings/c/tests/list.cpp @@ -65,7 +65,7 @@ TEST_F(OpendalListTest, ListDirTest) }; // write must succeed - EXPECT_EQ(opendal_operator_write(this->p, path.c_str(), data), + EXPECT_EQ(opendal_operator_write(this->p, path.c_str(), &data), nullptr); // list must succeed since the write succeeded diff --git a/bindings/go/error.go b/bindings/go/error.go index 33ed9b8c28f4..37ea26f07073 100644 --- a/bindings/go/error.go +++ b/bindings/go/error.go @@ -77,7 +77,7 @@ func parseError(ctx context.Context, err *opendalError) error { defer free(err) return &Error{ code: ErrorCode(err.code), - message: string(parseBytes(&err.message)), + message: string(parseBytes(err.message)), } } diff --git a/bindings/go/reader.go b/bindings/go/reader.go index c6fbb68238a8..ce26ce663ecd 100644 --- a/bindings/go/reader.go +++ b/bindings/go/reader.go @@ -68,8 +68,7 @@ func (op *Operator) Read(path string) ([]byte, error) { data := parseBytes(bytes) if len(data) > 0 { free := getFFI[bytesFree](op.ctx, symBytesFree) - free(bytes) - + free(&bytes) } return data, nil } @@ -203,17 +202,17 @@ func (r *Reader) Close() error { const symOperatorRead = "opendal_operator_read" -type operatorRead func(op *opendalOperator, path string) (*opendalBytes, error) +type operatorRead func(op *opendalOperator, path string) (opendalBytes, error) var withOperatorRead = withFFI(ffiOpts{ sym: symOperatorRead, rType: &typeResultRead, aTypes: []*ffi.Type{&ffi.TypePointer, &ffi.TypePointer}, }, func(ctx context.Context, ffiCall func(rValue unsafe.Pointer, aValues ...unsafe.Pointer)) operatorRead { - return func(op *opendalOperator, path string) (*opendalBytes, error) { + return func(op *opendalOperator, path string) (opendalBytes, error) { bytePath, err := unix.BytePtrFromString(path) if err != nil { - return nil, err + return opendalBytes{}, err } var result resultRead ffiCall( diff --git a/bindings/go/stat.go b/bindings/go/stat.go index 6b83ad605c80..7eb7345939a0 100644 --- a/bindings/go/stat.go +++ b/bindings/go/stat.go @@ -96,7 +96,6 @@ func (op *Operator) Stat(path string) (*Metadata, error) { // } else { // fmt.Println("The file does not exist") // } -// func (op *Operator) IsExist(path string) (bool, error) { isExist := getFFI[operatorIsExist](op.ctx, symOperatorIsExist) return isExist(op.inner, path) diff --git a/bindings/go/types.go b/bindings/go/types.go index 613825af5104..5751f0cdb05d 100644 --- a/bindings/go/types.go +++ b/bindings/go/types.go @@ -38,7 +38,7 @@ var ( typeResultRead = ffi.Type{ Type: ffi.Struct, Elements: &[]*ffi.Type{ - &ffi.TypePointer, + &typeBytes, &ffi.TypePointer, nil, }[0], @@ -217,7 +217,7 @@ type resultOperatorNew struct { type opendalOperator struct{} type resultRead struct { - data *opendalBytes + data opendalBytes error *opendalError } @@ -284,7 +284,7 @@ type opendalResultListerNext struct { type opendalEntry struct{} -func toOpendalBytes(data []byte) opendalBytes { +func toOpendalBytes(data []byte) *opendalBytes { var ptr *byte l := len(data) if l > 0 { @@ -293,15 +293,15 @@ func toOpendalBytes(data []byte) opendalBytes { var b byte ptr = &b } - return opendalBytes{ + return &opendalBytes{ data: ptr, len: uintptr(l), - capacity: uintptr(l), + capacity: uintptr(cap(data)), } } -func parseBytes(b *opendalBytes) (data []byte) { - if b == nil || b.len == 0 { +func parseBytes(b opendalBytes) (data []byte) { + if b.len == 0 { return nil } data = make([]byte, b.len) diff --git a/bindings/go/write.go b/bindings/go/write.go index d287524416c5..f0c23b816f5e 100644 --- a/bindings/go/write.go +++ b/bindings/go/write.go @@ -196,7 +196,7 @@ type operatorWrite func(op *opendalOperator, path string, data []byte) error var withOperatorWrite = withFFI(ffiOpts{ sym: symOperatorWrite, rType: &ffi.TypePointer, - aTypes: []*ffi.Type{&ffi.TypePointer, &ffi.TypePointer, &typeBytes}, + aTypes: []*ffi.Type{&ffi.TypePointer, &ffi.TypePointer, &ffi.TypePointer}, }, func(ctx context.Context, ffiCall func(rValue unsafe.Pointer, aValues ...unsafe.Pointer)) operatorWrite { return func(op *opendalOperator, path string, data []byte) error { bytePath, err := unix.BytePtrFromString(path) @@ -290,7 +290,7 @@ type writerWrite func(r *opendalWriter, buf []byte) (size int, err error) var withWriterWrite = withFFI(ffiOpts{ sym: symWriterWrite, rType: &typeResultWriterWrite, - aTypes: []*ffi.Type{&ffi.TypePointer, &typeBytes}, + aTypes: []*ffi.Type{&ffi.TypePointer, &ffi.TypePointer}, }, func(ctx context.Context, ffiCall func(rValue unsafe.Pointer, aValues ...unsafe.Pointer)) writerWrite { return func(r *opendalWriter, data []byte) (size int, err error) { bytes := toOpendalBytes(data) diff --git a/bindings/swift/OpenDAL/Sources/OpenDAL/Operator.swift b/bindings/swift/OpenDAL/Sources/OpenDAL/Operator.swift index 2ce27551e39e..6e2bb3a7bd1c 100644 --- a/bindings/swift/OpenDAL/Sources/OpenDAL/Operator.swift +++ b/bindings/swift/OpenDAL/Sources/OpenDAL/Operator.swift @@ -57,11 +57,13 @@ public class Operator { self.nativeOp = UnsafePointer(ret.op)! } - public func blockingWrite(_ data: Data, to path: String) throws { - let ret = data.withUnsafeBytes { dataPointer in + public func blockingWrite(_ data: inout Data, to path: String) throws { + let ret = data.withUnsafeMutableBytes { dataPointer in let address = dataPointer.baseAddress!.assumingMemoryBound(to: UInt8.self) - let bytes = opendal_bytes(data: address, len: UInt(dataPointer.count)) - return opendal_operator_write(nativeOp, path, bytes) + let bytes = opendal_bytes(data: address, len: UInt(dataPointer.count), capacity: UInt(dataPointer.count)) + return withUnsafePointer(to: bytes) { bytesPointer in + opendal_operator_write(nativeOp, path, bytesPointer) + } } if let err = ret { @@ -79,7 +81,7 @@ public class Operator { } public func blockingRead(_ path: String) throws -> Data { - let ret = opendal_operator_read(nativeOp, path) + var ret = opendal_operator_read(nativeOp, path) if let err = ret.error { defer { opendal_error_free(err) @@ -93,6 +95,6 @@ public class Operator { ) } - return Data(openDALBytes: ret.data) + return withUnsafeMutablePointer(to: &ret.data) { Data(openDALBytes: $0) } } } diff --git a/bindings/swift/OpenDAL/Tests/OpenDALTests/OpenDALTests.swift b/bindings/swift/OpenDAL/Tests/OpenDALTests/OpenDALTests.swift index ef17c7887486..304e6d5a27d8 100644 --- a/bindings/swift/OpenDAL/Tests/OpenDALTests/OpenDALTests.swift +++ b/bindings/swift/OpenDAL/Tests/OpenDALTests/OpenDALTests.swift @@ -28,8 +28,8 @@ final class OpenDALTests: XCTestCase { ] ) - let testContents = Data([1, 2, 3, 4]) - try op.blockingWrite(testContents, to: "test") + var testContents = Data([1, 2, 3, 4]) + try op.blockingWrite(&testContents, to: "test") let readContents = try op.blockingRead("test") diff --git a/bindings/zig/test/bdd.zig b/bindings/zig/test/bdd.zig index 98d49525ce0c..8d2f6ce79f6c 100644 --- a/bindings/zig/test/bdd.zig +++ b/bindings/zig/test/bdd.zig @@ -24,7 +24,7 @@ test "Opendal BDD test" { const c_str = [*:0]const u8; // define a type for 'const char*' in C const OpendalBDDTest = struct { - p: [*c]const opendal.c.opendal_operator, + p: [*c]opendal.c.opendal_operator, scheme: c_str, path: c_str, content: c_str, @@ -57,13 +57,15 @@ test "Opendal BDD test" { var testkit = OpendalBDDTest.init(); defer testkit.deinit(); + const allocator = std.heap.page_allocator; + const dupe_content = try allocator.dupeZ(u8, std.mem.span(testkit.content)); // When Blocking write path "test" with content "Hello, World!" const data: opendal.c.opendal_bytes = .{ - .data = testkit.content, - // c_str does not have len field (.* is ptr) - .len = std.mem.len(testkit.content), + .data = dupe_content.ptr, + .len = dupe_content.len, + .capacity = dupe_content.len, }; - const result = opendal.c.opendal_operator_write(testkit.p, testkit.path, data); + const result = opendal.c.opendal_operator_write(testkit.p, testkit.path, &data); try testing.expectEqual(result, null); // The blocking file "test" should exist @@ -82,14 +84,14 @@ test "Opendal BDD test" { defer opendal.c.opendal_metadata_free(meta); // The blocking file "test" must have content "Hello, World!" - const r: opendal.c.opendal_result_read = opendal.c.opendal_operator_read(testkit.p, testkit.path); - defer opendal.c.opendal_bytes_free(r.data); + var r: opendal.c.opendal_result_read = opendal.c.opendal_operator_read(testkit.p, testkit.path); + defer opendal.c.opendal_bytes_free(&r.data); try testing.expect(r.@"error" == null); - try testing.expectEqual(std.mem.len(testkit.content), r.data.*.len); + try testing.expectEqual(std.mem.len(testkit.content), r.data.len); var count: usize = 0; - while (count < r.data.*.len) : (count += 1) { - try testing.expectEqual(testkit.content[count], r.data.*.data[count]); + while (count < r.data.len) : (count += 1) { + try testing.expectEqual(testkit.content[count], r.data.data[count]); } } From 737012f6e7bc57469a06e37b1f3b5ce2da9b8d62 Mon Sep 17 00:00:00 2001 From: Twice Date: Tue, 15 Oct 2024 19:08:27 +0800 Subject: [PATCH 2/2] build(bindings/c): replace the build system with CMake (#5182) --- .github/workflows/ci_bindings_c.yml | 21 ++----- bindings/c/CMakeLists.txt | 90 +++++++++++++++++++++++++++++ bindings/c/Makefile | 58 ------------------- bindings/c/examples/basic.c | 2 +- bindings/c/tests/bdd.cpp | 6 -- bindings/c/tests/error_msg.cpp | 6 -- bindings/c/tests/list.cpp | 6 -- bindings/c/tests/opinfo.cpp | 6 -- bindings/zig/build.zig | 11 ++-- 9 files changed, 101 insertions(+), 105 deletions(-) create mode 100644 bindings/c/CMakeLists.txt diff --git a/.github/workflows/ci_bindings_c.yml b/.github/workflows/ci_bindings_c.yml index 33b2ecbcc910..28cd852b6827 100644 --- a/.github/workflows/ci_bindings_c.yml +++ b/.github/workflows/ci_bindings_c.yml @@ -44,31 +44,20 @@ jobs: runs-on: ubuntu-latest steps: - uses: actions/checkout@v4 - - name: Install gtest manually - run: | - sudo apt-get update - sudo apt-get install libgtest-dev valgrind - cd /usr/src/gtest - sudo cmake CMakeLists.txt - sudo make - sudo cp lib/*.a /usr/lib - sudo ln -s /usr/lib/libgtest.a /usr/local/lib/libgtest.a - sudo ln -s /usr/lib/libgtest_main.a /usr/local/lib/libgtest_main.a - name: Setup Rust toolchain uses: ./.github/actions/setup - name: Build C binding working-directory: "bindings/c" - run: make build + run: | + mkdir build && cd build + cmake .. -DTEST_ENABLE_ASAN=ON + make -j$(nproc) - name: Check diff run: git diff --exit-code - 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 + run: ./build/tests diff --git a/bindings/c/CMakeLists.txt b/bindings/c/CMakeLists.txt new file mode 100644 index 000000000000..3c5a54019f4e --- /dev/null +++ b/bindings/c/CMakeLists.txt @@ -0,0 +1,90 @@ +# 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. + +cmake_minimum_required(VERSION 3.22) +project(opendal-c) + +if (NOT CMAKE_BUILD_TYPE) + set(CMAKE_BUILD_TYPE "Debug") +endif() + +option(TEST_ENABLE_ASAN "Enable AddressSanitizer for tests" OFF) +set(GOOGLETEST_VERSION 1.15.2) + +# force the compiler to support these standards +set(CMAKE_C_STANDARD 11) +set(CMAKE_C_STANDARD_REQUIRED ON) + +# for GoogleTest, it should be no less than C++14 +set(CMAKE_CXX_STANDARD 14) +set(CMAKE_CXX_STANDARD_REQUIRED ON) + +# fetch google test via GitHub +include(FetchContent) +FetchContent_Declare( + googletest + URL https://github.com/google/googletest/archive/refs/tags/v${GOOGLETEST_VERSION}.zip +) +# For Windows: Prevent overriding the parent project's compiler/linker settings +set(gtest_force_shared_crt ON CACHE BOOL "" FORCE) +FetchContent_MakeAvailable(googletest) + +set(CARGO_DIST_DIR "${PROJECT_SOURCE_DIR}/target/debug") +if (NOT CMAKE_BUILD_TYPE STREQUAL "Debug") + set(CARGO_BUILD_TYPE "--release") + set(CARGO_DIST_DIR "${PROJECT_SOURCE_DIR}/target/release") +endif() + +set(OPENDAL_STATIC_LIB "${CARGO_DIST_DIR}/libopendal_c.a") +set(OPENDAL_SHARED_LIB "${CARGO_DIST_DIR}/libopendal_c.so") +message(NOTICE "-- OpenDAL C static lib: ${OPENDAL_STATIC_LIB}") +message(NOTICE "-- OpenDAL C shared lib: ${OPENDAL_SHARED_LIB}") + +# custom target for cargo build +add_custom_target(cargo_build + COMMAND sh -c "cargo build ${CARGO_BUILD_TYPE}" + BYPRODUCTS ${OPENDAL_STATIC_LIB} ${OPENDAL_SHARED_LIB} + WORKING_DIRECTORY ${PROJECT_SOURCE_DIR} +) + +# cmake target for static lib +add_library(opendal_c_static INTERFACE) +target_link_libraries(opendal_c_static INTERFACE ${OPENDAL_STATIC_LIB}) +target_include_directories(opendal_c_static INTERFACE include) +add_dependencies(opendal_c_static cargo_build) + +# cmake target for shared lib +add_library(opendal_c_shared INTERFACE) +target_link_libraries(opendal_c_shared INTERFACE ${OPENDAL_SHARED_LIB}) +target_include_directories(opendal_c_shared INTERFACE include) +add_dependencies(opendal_c_shared cargo_build) + +# example targets +add_executable(basic examples/basic.c) +target_link_libraries(basic opendal_c_shared) + +add_executable(error_handle examples/error_handle.c) +target_link_libraries(error_handle opendal_c_shared) + +# test targets +file(GLOB TEST_SRCS tests/*.cpp) +add_executable(tests ${TEST_SRCS}) +target_link_libraries(tests opendal_c_shared gtest_main) +if (TEST_ENABLE_ASAN) + target_compile_options(tests PRIVATE -fsanitize=address) + target_link_options(tests PRIVATE -fsanitize=address) +endif() diff --git a/bindings/c/Makefile b/bindings/c/Makefile index a9b13d842e56..5acf1e908d22 100644 --- a/bindings/c/Makefile +++ b/bindings/c/Makefile @@ -15,75 +15,17 @@ # specific language governing permissions and limitations # under the License. -RPATH=$(PWD)/target/debug -OBJ_DIR=./build DOC_DIR=./docs -CCFLAGS=-I./include -CXXFLAGS=-I./include -std=c++14 -LDFLAGS=-L$(RPATH) -Wl,-rpath,$(RPATH) - -LIBS=-lopendal_c -lgtest -lpthread - -VALGRIND=valgrind --error-exitcode=1 --leak-check=full -- - -.PHONY: all -all: build test examples - .PHONY: format format: cargo fmt find . -name '*.cpp' -exec clang-format -i --style=WebKit --verbose {} \; find . -name '*.c' -exec clang-format -i --style=WebKit --verbose {} \; -.PHONY: build -build: - mkdir -p $(OBJ_DIR) - cargo build - -.PHONY: test -test: - $(CXX) tests/bdd.cpp -o $(OBJ_DIR)/bdd $(CXXFLAGS) $(LDFLAGS) $(LIBS) - $(CXX) tests/list.cpp -o $(OBJ_DIR)/list $(CXXFLAGS) $(LDFLAGS) $(LIBS) - $(CXX) tests/error_msg.cpp -o $(OBJ_DIR)/error_msg $(CXXFLAGS) $(LDFLAGS) $(LIBS) - $(CXX) tests/opinfo.cpp -o $(OBJ_DIR)/opinfo $(CXXFLAGS) $(LDFLAGS) $(LIBS) - $(OBJ_DIR)/bdd - $(OBJ_DIR)/list - $(OBJ_DIR)/error_msg - $(OBJ_DIR)/opinfo - -.PHONY: test_memory_leak -memory_leak: - $(VALGRIND) $(OBJ_DIR)/bdd - $(VALGRIND) $(OBJ_DIR)/list - $(VALGRIND) $(OBJ_DIR)/error_msg - .PHONY: doc doc: mkdir -p $(DOC_DIR) curl --proto '=https' --tlsv1.2 -sSf https://cdn.jsdelivr.net/gh/jothepro/doxygen-awesome-css@2.2.1/doxygen-awesome.min.css \ -o $(DOC_DIR)/doxygen-awesome.css doxygen Doxyfile - -# build examples begin -EXAMPLES=$(wildcard ./examples/*.c) -EXAMPLE_OBJECTS=$(EXAMPLES:.c=.o) -EXAMPLE_TARGETS=$(EXAMPLES:.c=) -.PHONY: examples -examples: $(EXAMPLE_TARGETS) - -$(EXAMPLE_TARGETS): % : %.o - $(CC) $(CCFLAGS) -o $@ $< $(LDFLAGS) $(LIBS) - -%.o: %.c - $(CC) $(CCFLAGS) -c $< -o $@ -# build examples end - -.PHONY: clean -clean: - cargo clean - rm -rf $(EXAMPLE_OBJECTS) - rm -rf $(EXAMPLE_TARGETS) - rm -rf $(OBJ_DIR) - rm -rf $(DOC_DIR) - diff --git a/bindings/c/examples/basic.c b/bindings/c/examples/basic.c index af55faeea0df..199403ca53b0 100644 --- a/bindings/c/examples/basic.c +++ b/bindings/c/examples/basic.c @@ -37,7 +37,7 @@ int main() }; /* Write this into path "/testpath" */ - opendal_error* error = opendal_operator_write(op, "/testpath", data); + opendal_error* error = opendal_operator_write(op, "/testpath", &data); assert(error == NULL); /* We can read it out, make sure the data is the same */ diff --git a/bindings/c/tests/bdd.cpp b/bindings/c/tests/bdd.cpp index 65e97f52bf44..16bea5f10e00 100644 --- a/bindings/c/tests/bdd.cpp +++ b/bindings/c/tests/bdd.cpp @@ -133,9 +133,3 @@ TEST_F(OpendalBddTest, FeatureTest) error = opendal_operator_delete(this->p, "tmpdir/"); EXPECT_EQ(error, nullptr); } - -int main(int argc, char** argv) -{ - ::testing::InitGoogleTest(&argc, argv); - return RUN_ALL_TESTS(); -} diff --git a/bindings/c/tests/error_msg.cpp b/bindings/c/tests/error_msg.cpp index 516d27865aaf..16bc57aaed96 100644 --- a/bindings/c/tests/error_msg.cpp +++ b/bindings/c/tests/error_msg.cpp @@ -64,9 +64,3 @@ TEST_F(OpendalErrorTest, ErrorReadTest) // free the error opendal_error_free(r.error); } - -int main(int argc, char** argv) -{ - ::testing::InitGoogleTest(&argc, argv); - return RUN_ALL_TESTS(); -} diff --git a/bindings/c/tests/list.cpp b/bindings/c/tests/list.cpp index 3b64fef460b6..533fda7a8135 100644 --- a/bindings/c/tests/list.cpp +++ b/bindings/c/tests/list.cpp @@ -119,9 +119,3 @@ TEST_F(OpendalListTest, ListEmptyDirTest) { } // todo: Try list a directory that does not exist TEST_F(OpendalListTest, ListNotExistDirTest) { } - -int main(int argc, char** argv) -{ - ::testing::InitGoogleTest(&argc, argv); - return RUN_ALL_TESTS(); -} diff --git a/bindings/c/tests/opinfo.cpp b/bindings/c/tests/opinfo.cpp index 9684b0360473..4312a152a842 100644 --- a/bindings/c/tests/opinfo.cpp +++ b/bindings/c/tests/opinfo.cpp @@ -97,9 +97,3 @@ TEST_F(OpendalOperatorInfoTest, InfoTest) free(scheme); free(root); } - -int main(int argc, char** argv) -{ - ::testing::InitGoogleTest(&argc, argv); - return RUN_ALL_TESTS(); -} diff --git a/bindings/zig/build.zig b/bindings/zig/build.zig index fc1623ddcee0..b5d26bfa473c 100644 --- a/bindings/zig/build.zig +++ b/bindings/zig/build.zig @@ -31,13 +31,12 @@ pub fn build(b: *std.Build) void { opendal_module.addIncludePath(b.path("../c/include")); // Creates a step for building the dependent C bindings - const libopendal_c = b.addSystemCommand(&[_][]const u8{ - "make", - "-C", - "../c", - "build", - }); + const libopendal_c_cmake = b.addSystemCommand(&[_][]const u8{ "cmake", "-S", "../c", "-B", "../c/build" }); + const config_libopendal_c = b.step("libopendal_c_cmake", "Generate OpenDAL C binding CMake files"); + config_libopendal_c.dependOn(&libopendal_c_cmake.step); + const libopendal_c = b.addSystemCommand(&[_][]const u8{ "make", "-C", "../c/build" }); const build_libopendal_c = b.step("libopendal_c", "Build OpenDAL C bindings"); + libopendal_c.step.dependOn(config_libopendal_c); build_libopendal_c.dependOn(&libopendal_c.step); // Creates a step for unit testing. This only builds the test executable