-
Notifications
You must be signed in to change notification settings - Fork 91
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.
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall nice improvement for libdfi :)
I am not sure if a single descriptor for array list (a
) is enough.
@@ -1199,7 +1199,8 @@ CELIX_UTILS_EXPORT celix_status_t celix_properties_loadFromStream(FILE* stream, | |||
* | |||
* For a overview of the possible decode flags, see the CELIX_PROPERTIES_DECODE_* flags documentation. | |||
* | |||
* If an error occurs, the error status is returned and a message is logged to celix_err. | |||
* If an error occurs, the error status is returned and a message is logg |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: word logged is split over 2 lines
|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. | |
There was a problem hiding this comment.
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.
|**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 | |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
- Let the service implementation perform type checking manually, calling
json_is_array
etc. - 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.
There was a problem hiding this comment.
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.
This PR aims to add
celix_properties_t*
andcelix_array_list_t*
to thedfi
. In addition, in order to reuse the serialization code ofcelix_array_list_t
, this PR moves the serialization code ofcelix_array_list_t
inproperties_encoding.c
tocelix_array_list_encoding.c
.There is an issue that needs to be discussed in the PR. When libdfi serializes
celix_properties_t
orcelix_array_list_t
, it will callcelix_properties_saveToString
orcelix_arrayList_saveToString
to convert them into a JSON representation string, and then convert the string into a JSON-object. (See thejsonSerializer_writeProperties
function for specific code)There is a process of converting JSON-object to a string and then converting the string to a JSON-object, which may affect performance. If add a new interface
celix_properties_saveToJson(const celix_properties_t* properties, int encodeFlags, json_t** out)
tocelix_properties.h
, which directly convertscelix_properties_t
to a JSON-object, this can reduce the operation of converting JSON-object to a string. But it will make thejson_t
type part of theCelix::utils
external interface.