Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add celix_properties_t and celix_array_list_t pointer type to dfi #782

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Add celix_properties_t and celix_array_list_t pointer type to dfi #782

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
547075e
Add celix_properties_t and celix_array_list_t pointer type in dfi
xuzhenbao Jan 10, 2025
87fd9e2
Modify libdfi version
xuzhenbao Jan 14, 2025
d1b7959
Merge branch 'apache:master' into add_dyn_type
xuzhenbao Jan 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion libs/dfi/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ if (CELIX_DFI)
target_link_libraries(dfi PUBLIC jansson::jansson Celix::utils)##The public header file(dyn_interface.h) Celix::utils(celix_version.h)
set_target_properties(dfi PROPERTIES
C_VISIBILITY_PRESET hidden
VERSION "1.3.1"
VERSION "1.4.0"
"SOVERSION" 1
OUTPUT_NAME "celix_dfi")

Expand Down
18 changes: 9 additions & 9 deletions libs/dfi/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,9 +71,9 @@ The data types supported by the interface description include:

*Type schema*:

|**Identifier**|B |D |F |I |J |S |V |Z |b | i | j | s |P | t |N |
|---------|---|------|-----|-------|-------|-------|----|--------------|-----|--------|--------|--------|------|------------------|---|
|**Types**|char|double|float|int32_t|int64_t|int16_t|void|boolean(uint8)|uchar|uint32_t|uint64_t|uint16_t|void *| char *(C string) |int|
|**Identifier**|B |D |F |I |J |S |V |Z |b | i | j | s |P | t |N | p | a |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is a descriptor a enough for array list?

With the current implementation of array list, the array element type is fixed at creation. Using the array list wrong (e.g. getLong, when it is a string) will lead to an assert failure.

With dfi you can serializer / deserialize array list of any element type, but the underlying code will assume a element type (or needs to use defense programming by checking the element type before continuing).

Maybe it is better to use different dfi descriptor for the different array lists (string, long, double, boolean and version).

@xuzhenbao and @PengZheng WDYT?

Copy link
Contributor

@PengZheng PengZheng Jan 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it is better to use different dfi descriptor for the different array lists (string, long, double, boolean and version).

Or a together with type annotation using meta-info.
And then the dfi could do the type-checking.

Will it make the local service different from the remote ones?
For a local service, we must do the type-checking anyway.
Or shall we make array element type assertions instead so that local/remote services could be unified?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A a with annotations sound good.

IMO there is a different between local calls and remote calls. Generally speaking local calls can be trusted (although here is also room for improvement (static bundles / signed bundles, because you can dynamically install new bundles). A remote call is less trustworthy, so IMO crashing (assert fail) is then not an option.

To be honest, I am still unsure if the current array list implementation is the correct approach. Another option could be to really split up the public array list API in a long, double, string, pointer, boolean, version array. So something like: celix_long_array_list_t with functions like celix_longArrayList_get, etc.

The underlying code can still be the current array_list, but the public api only provided wrapper around the generic impl.

If we want to do that, this can and should be done separate from this pull request.

Copy link
Contributor

@PengZheng PengZheng Jan 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A remote call is less trustworthy, so IMO crashing (assert fail) is then not an option.

I totally agree.

To be honest, I am still unsure if the current array list implementation is the correct approach. Another option could be to really split up the public array list API in a long, double, string, pointer, boolean, version array. So something like: celix_long_array_list_t with functions like celix_longArrayList_get, etc.

Imagine that we are dealing with json rather than array list or properties, there are at least two options:

  1. Let the service implementation perform type checking manually, calling json_is_array etc.
  2. Delegate type checking to the framework via json schema, which is essentially the same as type annotation approach. Then it will be OK to add various type assertions to the service implementation (for documentation purpose), which may be remote or local.

Both should work.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using JSON Schema is indeed a good option. It helps avoid overly defensive programming and ensures validation occurs before messages are deserialized.

However, isn’t the DFI descriptor already a form of serialization schema? But less focused on the serialized format itself and more on the memory representation (excluding padding and alignment details, which are handled by libffi).

Given that libjansson does not support JSON Schema, I believe it would be better to extend the DFI type system to be more explicit. This could also open the door to future enhancements, such as adding support for annotations like min/max.

One potential downside of this approach is that it would no longer be possible to generate a descriptor string directly from a header file. Currently, - I think - this is possible (e.g., parsing a celix_array_list_t* field by adding an a descriptor type). This is why I think it might be better to replace celix_array_list_t with type-specific versions.

For now, my proposal is to keep the a descriptor for this pull request. If we decide to replace celix_array_list_t with type-specific versions in the future, both the DFI and serialization logic would need to be updated accordingly.

|---------|---|------|-----|-------|-------|-------|----|--------------|-----|--------|--------|--------|------|------------------|---|---------------------|---------------------|
|**Types**|char|double|float|int32_t|int64_t|int16_t|void|boolean(uint8)|uchar|uint32_t|uint64_t|uint16_t|void *| char *(C string) |int| celix_properties_t* | celix_array_list_t* |


- **Complex Types(Struct)**
Expand Down Expand Up @@ -236,12 +236,12 @@ The data types supported by the interface description include:
~~~
In order to represent the properties of function parameters (eg: in, out...), function parameters support the following metadata annotations:

|Meta-info| Description|
|---------|------------|
|am=handle| void pointer for the handle.|
|am=pre | output pointer with memory pre-allocated, it should be pointer to [trivially copyable type](#notion-definitions).|
|am=out | output pointer, the caller should use `free` to release the memory, and it should be pointer to text(t) or double pointer to [serializable types](#notion-definitions).|
|const=true| text argument(t) can use it, Normally a text argument will be handled as char*, meaning that the callee is expected to take of ownership.If a const=true annotation is used the text argument will be handled as a const char*, meaning that the caller keeps ownership of the string.|
|Meta-info| Description |
|---------|--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
|am=handle| void pointer for the handle. |
|am=pre | output pointer with memory pre-allocated, it should be pointer to [trivially copyable type](#notion-definitions). |
|am=out | output pointer, the caller should use `free` to release the memory, and it should be pointer to text(t) or double pointer to [serializable types](#notion-definitions). |
|const=true| text argument(t) and `celix_properties_t*`(p) and `celix_array_list_t*`(a) can use it, Normally a text argument will be handled as char*, meaning that the callee is expected to take of ownership.If a const=true annotation is used the text argument will be handled as a const char*, meaning that the caller keeps ownership of the string. |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: The properties and array_list types have been added as potential targets for const=true, but the accompanying text currently only references char*. I suggest we rephrase it for clarity, something like:

Normally, a text, properties, or array list argument will be handled respectively as `char*`, `celix_properties_t*`, or `celix_array_list_t*`, implying that the callee is expected to take ownership. However, if the `const=true` annotation is used, these arguments will be handled as `const char*`, `const celix_properties_t*`, or `const celix_array_list_t*`, indicating that the caller retains ownership of the string/object.


If there is no metadata annotation, the default is standard argument(input parameter). And it can be any serializable type.

Expand Down
10 changes: 10 additions & 0 deletions libs/dfi/gtest/descriptors/example8.descriptor
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
:header
type=interface
name=example8
version=1.0.0
:annotations
:types
:methods
getProps=getProps(#am=handle;P#am=out;*p)N
setProps=setProps(#am=handle;Pp)N
setConstProps=setConstProps(#am=handle;P#const=true;p)N
10 changes: 10 additions & 0 deletions libs/dfi/gtest/descriptors/example9.descriptor
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
:header
type=interface
name=example8
version=1.0.0
:annotations
:types
:methods
getArrayList=getArrayList(#am=handle;P#am=out;*a)N
setArrayList=setArrayList(#am=handle;Pa)N
setConstArrayList=setConstArrayList(#am=handle;P#const=true;a)N
88 changes: 87 additions & 1 deletion libs/dfi/gtest/src/dyn_function_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@
#include "dyn_example_functions.h"
#include "dyn_common.h"
#include "dyn_function.h"
#include "celix_properties.h"
#include "celix_array_list.h"
#include "celix_err.h"

#include <ffi.h>
Expand Down Expand Up @@ -308,7 +310,7 @@ TEST_F(DynFunctionTests, WrongArgumentMetaTest) {

int rc4 = dynFunction_parseWithStr("example(#am=handle;P#am=out;*D)N", nullptr, &dynFunc);
EXPECT_NE(0, rc4);
EXPECT_STREQ("Error 'out' is only allowed for pointer to text or typed pointer not to 'D'", celix_err_popLastError());
EXPECT_STREQ("Error 'out' is only allowed for pointer to text or built-in object or typed pointer not to 'D'", celix_err_popLastError());

// #am=pre argument is not allowed for non pointer types
int rc5 = dynFunction_parseWithStr("example(#am=pre;I)N", nullptr, &dynFunc);
Expand All @@ -319,4 +321,88 @@ TEST_F(DynFunctionTests, WrongArgumentMetaTest) {
int rc6 = dynFunction_parseWithStr("example(#am=pre;**D)N", nullptr, &dynFunc);
EXPECT_NE(0, rc6);
EXPECT_STREQ("Error 'pre' is only allowed for pointer to trivial types not non-trivial '*'", celix_err_popLastError());

int rc7 = dynFunction_parseWithStr("example(#am=handle;P#am=out;p)N", nullptr, &dynFunc);
EXPECT_NE(0, rc7);
EXPECT_STREQ("Error 'out' is only allowed for typed pointer not 'p'", celix_err_popLastError());

int rc8 = dynFunction_parseWithStr("example(#am=handle;P#am=out;a)N", nullptr, &dynFunc);
EXPECT_NE(0, rc8);
EXPECT_STREQ("Error 'out' is only allowed for typed pointer not 'a'", celix_err_popLastError());

int rc9 = dynFunction_parseWithStr("example(#am=handle;P#am=pre;p)N", nullptr, &dynFunc);
EXPECT_NE(0, rc9);
EXPECT_STREQ("Error 'pre' is only allowed for typed pointer not 'p'", celix_err_popLastError());

int rc10 = dynFunction_parseWithStr("example(#am=handle;P#am=pre;a)N", nullptr, &dynFunc);
EXPECT_NE(0, rc10);
EXPECT_STREQ("Error 'pre' is only allowed for typed pointer not 'a'", celix_err_popLastError());

int rc11 = dynFunction_parseWithStr("example(#am=handle;P#am=pre;*p)N", nullptr, &dynFunc);
EXPECT_NE(0, rc11);
EXPECT_STREQ("Error 'pre' is only allowed for pointer to trivial types not non-trivial 'p'", celix_err_popLastError());

int rc12 = dynFunction_parseWithStr("example(#am=handle;P#am=pre;*a)N", nullptr, &dynFunc);
EXPECT_NE(0, rc12);
EXPECT_STREQ("Error 'pre' is only allowed for pointer to trivial types not non-trivial 'a'", celix_err_popLastError());
}

TEST_F(DynFunctionTests, DynFuncWithPropertiesArgTest) {
dyn_function_type *dynFunc = nullptr;

void (*fp)(const celix_properties_t *constProps, celix_properties_t *props, celix_properties_t **out) =
[](const celix_properties_t *constProps, celix_properties_t *props, celix_properties_t **out) {
EXPECT_TRUE(celix_properties_get(constProps, "key1", nullptr) != nullptr);
*out = celix_properties_copy(props);
celix_properties_destroy(props);
return;
};
int rc = dynFunction_parseWithStr("example(#const=true;pp#am=out;*p)V", nullptr, &dynFunc);
ASSERT_EQ(0, rc);
EXPECT_EQ(3, dynFunction_nrOfArguments(dynFunc));

celix_autoptr(celix_properties_t) constProps = celix_properties_create();//owner is caller
celix_properties_set(constProps, "key1", "value1");
celix_properties_t* props = celix_properties_create();//owner is callee
celix_properties_set(props, "key2", "value2");
void *args[3];
args[0] = &constProps;
args[1] = &props;
celix_autoptr(celix_properties_t) result = nullptr;
celix_properties_t** ptrToResult = &result;
args[2] = &ptrToResult;
rc = dynFunction_call(dynFunc, (void (*)(void))fp, nullptr, args);
EXPECT_EQ(0, rc);
dynFunction_destroy(dynFunc);
EXPECT_TRUE(celix_properties_get(result, "key2", nullptr) != nullptr);
}

TEST_F(DynFunctionTests, DynFuncWithArrayListArgTest) {
dyn_function_type *dynFunc = nullptr;

void (*fp) (const celix_array_list_t* constProps, celix_array_list_t* props, celix_array_list_t** out) =
[](const celix_array_list_t* constProps, celix_array_list_t* props, celix_array_list_t** out) {
EXPECT_EQ(1, celix_arrayList_getLong(constProps, 0));
*out = celix_arrayList_copy(props);
celix_arrayList_destroy(props);
return;
};
int rc = dynFunction_parseWithStr("example(#const=true;aa#am=out;*a)V", nullptr, &dynFunc);
ASSERT_EQ(0, rc);
EXPECT_EQ(3, dynFunction_nrOfArguments(dynFunc));

celix_autoptr(celix_array_list_t) constList = celix_arrayList_createLongArray();//owner is caller
celix_arrayList_addLong(constList, 1);
celix_array_list_t* list = celix_arrayList_createLongArray();//owner is callee
celix_arrayList_addLong(list, 2);
void *args[3];
args[0] = &constList;
args[1] = &list;
celix_autoptr(celix_array_list_t) result = nullptr;
celix_array_list_t** ptrToResult = &result;
args[2] = &ptrToResult;
rc = dynFunction_call(dynFunc, (void (*)(void))fp, nullptr, args);
EXPECT_EQ(0, rc);
dynFunction_destroy(dynFunc);
EXPECT_EQ(2, celix_arrayList_getLong(result, 0));
}
16 changes: 16 additions & 0 deletions libs/dfi/gtest/src/dyn_type_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,8 @@ class DynTypeTests : public ::testing::Test {
#define EX18 "Ttext=t;ltext;"
#define EX19 "Tsample={DD vala valb};Tref=lsample;;lref;"
#define EX20 "TINTEGER=I;Tsample={DlINTEGER; vala valb};Tref=lsample;;lref;"
#define EX21 "Tprops=p;lprops;"
#define EX22 "Tarraylist=a;larraylist;"

#define CREATE_EXAMPLES_TEST(DESC) \
TEST_F(DynTypeTests, ParseTestExample ## DESC) { \
Expand Down Expand Up @@ -116,6 +118,8 @@ CREATE_EXAMPLES_TEST(EX17)
CREATE_EXAMPLES_TEST(EX18)
CREATE_EXAMPLES_TEST(EX19)
CREATE_EXAMPLES_TEST(EX20)
CREATE_EXAMPLES_TEST(EX21)
CREATE_EXAMPLES_TEST(EX22)

TEST_F(DynTypeTests, ParseRandomGarbageTest) {
/*
Expand Down Expand Up @@ -638,4 +642,16 @@ TEST_F(DynTypeTests, TrivialityTesT) {
ASSERT_EQ(0, rc);
EXPECT_FALSE(dynType_isTrivial(type));
dynType_destroy(type);

// celix_properties_t* is non-trivial
rc = dynType_parseWithStr("p", NULL, NULL, &type);
ASSERT_EQ(0, rc);
EXPECT_FALSE(dynType_isTrivial(type));
dynType_destroy(type);

// celix_array_list_t* is non-trivial
rc = dynType_parseWithStr("a", NULL, NULL, &type);
ASSERT_EQ(0, rc);
EXPECT_FALSE(dynType_isTrivial(type));
dynType_destroy(type);
}
Loading
Loading