You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@celix.apache.org by "PengZheng (via GitHub)" <gi...@apache.org> on 2023/12/14 07:42:35 UTC
[PR] Feature/dfi cleanup (celix)
PengZheng opened a new pull request, #699:
URL: https://github.com/apache/celix/pull/699
This PR replaces deprecated dfi_log_util.h by celix_err.h.
It also fixed the broken Conan2 coverage build (we are now using Conan1 for our CI coverage build).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@celix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Feature/dfi cleanup (celix)
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #699:
URL: https://github.com/apache/celix/pull/699#issuecomment-1855343335
## [Codecov](https://app.codecov.io/gh/apache/celix/pull/699?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: `36 lines` in your changes are missing coverage. Please review.
> Comparison is base [(`5393c2a`)](https://app.codecov.io/gh/apache/celix/commit/5393c2a84d73e46824188baeeff6eb8a5954ece4?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 84.27% compared to head [(`7943ae7`)](https://app.codecov.io/gh/apache/celix/pull/699?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 84.34%.
> Report is 2 commits behind head on master.
> :exclamation: Current head 7943ae7 differs from pull request most recent head 452bfb3. Consider uploading reports for the commit 452bfb3 to get more accurate results
| [Files](https://app.codecov.io/gh/apache/celix/pull/699?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [libs/dfi/src/dyn\_type.c](https://app.codecov.io/gh/apache/celix/pull/699?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-bGlicy9kZmkvc3JjL2R5bl90eXBlLmM=) | 30.76% | [18 Missing :warning: ](https://app.codecov.io/gh/apache/celix/pull/699?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [libs/dfi/src/dyn\_function.c](https://app.codecov.io/gh/apache/celix/pull/699?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-bGlicy9kZmkvc3JjL2R5bl9mdW5jdGlvbi5j) | 33.33% | [4 Missing :warning: ](https://app.codecov.io/gh/apache/celix/pull/699?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [libs/dfi/src/dyn\_interface.c](https://app.codecov.io/gh/apache/celix/pull/699?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-bGlicy9kZmkvc3JjL2R5bl9pbnRlcmZhY2UuYw==) | 55.55% | [4 Missing :warning: ](https://app.codecov.io/gh/apache/celix/pull/699?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [libs/dfi/src/dyn\_message.c](https://app.codecov.io/gh/apache/celix/pull/699?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-bGlicy9kZmkvc3JjL2R5bl9tZXNzYWdlLmM=) | 62.50% | [3 Missing :warning: ](https://app.codecov.io/gh/apache/celix/pull/699?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [libs/dfi/src/json\_serializer.c](https://app.codecov.io/gh/apache/celix/pull/699?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-bGlicy9kZmkvc3JjL2pzb25fc2VyaWFsaXplci5j) | 84.61% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/celix/pull/699?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [libs/dfi/src/dyn\_common.c](https://app.codecov.io/gh/apache/celix/pull/699?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-bGlicy9kZmkvc3JjL2R5bl9jb21tb24uYw==) | 66.66% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/celix/pull/699?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [libs/dfi/src/dyn\_type\_common.c](https://app.codecov.io/gh/apache/celix/pull/699?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-bGlicy9kZmkvc3JjL2R5bl90eXBlX2NvbW1vbi5j) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/celix/pull/699?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [libs/dfi/src/json\_rpc.c](https://app.codecov.io/gh/apache/celix/pull/699?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-bGlicy9kZmkvc3JjL2pzb25fcnBjLmM=) | 87.50% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/celix/pull/699?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [libs/framework/include/celix/BundleActivator.h](https://app.codecov.io/gh/apache/celix/pull/699?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-bGlicy9mcmFtZXdvcmsvaW5jbHVkZS9jZWxpeC9CdW5kbGVBY3RpdmF0b3IuaA==) | 50.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/celix/pull/699?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...ramework/include/celix/dm/ServiceDependency\_Impl.h](https://app.codecov.io/gh/apache/celix/pull/699?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-bGlicy9mcmFtZXdvcmsvaW5jbHVkZS9jZWxpeC9kbS9TZXJ2aWNlRGVwZW5kZW5jeV9JbXBsLmg=) | 50.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/celix/pull/699?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #699 +/- ##
==========================================
+ Coverage 84.27% 84.34% +0.06%
==========================================
Files 248 248
Lines 31124 31096 -28
==========================================
- Hits 26229 26227 -2
+ Misses 4895 4869 -26
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/celix/pull/699?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@celix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WIP]Feature/dfi cleanup (celix)
Posted by "PengZheng (via GitHub)" <gi...@apache.org>.
PengZheng commented on PR #699:
URL: https://github.com/apache/celix/pull/699#issuecomment-1893910675
When I modified `TEST_F(JsonSerializerTests, ParseTests)` as following, a crash happened:
```C++
type = nullptr;
inst = nullptr;
rc = dynType_parseWithStr(example5_descriptor, nullptr, nullptr, &type);
ASSERT_EQ(0, rc);
rc = jsonSerializer_deserialize(type, example5_input, strlen(example5_input), &inst);
ASSERT_EQ(0, rc);
json_auto_t* result = nullptr;
// crash happens HERE!
rc = jsonSerializer_serializeJson(type, inst, &result);
ASSERT_EQ(0, rc);
```
I found two issues:
1. In `example5_input`, the `node` pointed to by `head` misses `value` member. Similarly, the `node` pointed to by `head->left` misses `right` member. 2. nullptr is not dealt in `jsonSerializer_serializeJson`, which leads to the crash.
For the first issue, IMO all struct members should be present in the json object, because C struct is very rigid, unlike json object.
Our data model is based on C struct not on flexible json object.
For the second issue, I think nullptr should also be serialized.
WDYT? @pnoltes
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@celix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Feature/dfi cleanup (celix)
Posted by "pnoltes (via GitHub)" <gi...@apache.org>.
pnoltes commented on code in PR #699:
URL: https://github.com/apache/celix/pull/699#discussion_r1471676998
##########
libs/dfi/include/dyn_type.h:
##########
@@ -363,15 +361,25 @@ CELIX_DFI_EXPORT int dynType_typedPointer_getTypedType(dyn_type *type, dyn_type
* @return 0 if successful.
* @retval 1 if Cannot allocate memory.
*/
-CELIX_DFI_EXPORT int dynType_text_allocAndInit(dyn_type *type, void *textLoc, const char *value);
+CELIX_DFI_EXPORT int dynType_text_allocAndInit(const dyn_type* type, void* textLoc, const char* value);
/**
* @brief Sets the value of a simple type.
* @param[in] type The dyn type. Must be a simple type.
* @param[out] inst The instance of the simple type.
* @param[in] in The value to set.
*/
-CELIX_DFI_EXPORT void dynType_simple_setValue(dyn_type *type, void *inst, void *in);
+CELIX_DFI_EXPORT void dynType_simple_setValue(const dyn_type* type, void* inst, const void* in);
+
+/**
+ * @brief Test whether the given type is trivial.
+ * A trivial type does NOT involve any pointer type, and thus can always be safely shallow-copied.
+ * Any non-pointer simple type is trivial, while typed/untyped pointer is non-trivial.
+ * A sequence is non-trivial since it contains a pointer to the buffer.
+ * Text is nontrivial.
+ * A complex is trivial iff each field is trivial.
Review Comment:
```suggestion
* A complex is trivial if each field is trivial.
```
##########
libs/dfi/include/dyn_interface.h:
##########
@@ -50,9 +46,8 @@ typedef struct _dyn_interface_type dyn_interface_type;
TAILQ_HEAD(methods_head, method_entry);
struct method_entry {
int index;
- char *id;
- char *name;
- dyn_function_type *dynFunc;
+ char* id;
Review Comment:
It has been a while since I looked into libdfi. Is the 'id' generally the name or signature of a method?.
I think it would be good to document (doxygen) this more explicit.
##########
libs/dfi/src/dyn_type.c:
##########
@@ -185,265 +187,200 @@ static int dynType_parseAny(FILE *stream, dyn_type *type) {
return status;
}
-static int dynType_parseMetaInfo(FILE *stream, dyn_type *type) {
+static int dynType_parseMetaInfo(FILE* stream, dyn_type* type) {
int status = OK;
- char *name = NULL;
- char *value = NULL;
-
- struct meta_entry *entry = calloc(1, sizeof(*entry));
- if (entry == NULL) {
- status = ERROR;
- }
+ celix_autofree char* name = NULL;
+ celix_autofree char* value = NULL;
- if (status == OK) {
- status = dynCommon_parseName(stream, &name);
+ if (dynCommon_parseName(stream, &name) != OK) {
+ status = PARSE_ERROR;
+ goto bail_out;
}
-
- if (status == OK) {
- status = dynCommon_eatChar(stream, '=');
+ if (dynCommon_eatChar(stream, '=') != OK) {
+ status = PARSE_ERROR;
+ goto bail_out;
}
-
- if (status == OK) {
- status = dynCommon_parseName(stream, &value);
+ if (dynCommon_parseName(stream, &value) != OK) {
+ status = PARSE_ERROR;
+ goto bail_out;
}
-
- if (status == OK) {
- status = dynCommon_eatChar(stream, ';');
+ if (dynCommon_eatChar(stream, ';') != OK) {
+ status = PARSE_ERROR;
+ goto bail_out;
}
-
- if (status == OK) {
- entry->name = name;
- entry->value = value;
- TAILQ_INSERT_TAIL(&type->metaProperties, entry, entries);
- } else {
- LOG_ERROR("Failed to parse meta properties");
- free(name);
- free(value);
- free(entry);
+ struct meta_entry *entry = calloc(1, sizeof(*entry));
+ if (entry == NULL) {
+ status = MEM_ERROR;
+ goto bail_out;
}
+ entry->name = celix_steal_ptr(name);
+ entry->value = celix_steal_ptr(value);
+ TAILQ_INSERT_TAIL(&type->metaProperties, entry, entries);
+ return OK;
+bail_out:
+ celix_err_push("Failed to parse meta properties");
return status;
}
-static int dynType_parseText(FILE *stream, dyn_type *type) {
- int status = OK;
+static int dynType_parseText(FILE* stream, dyn_type* type) {
type->type = DYN_TYPE_TEXT;
type->descriptor = 't';
+ type->trivial = false;
type->ffiType = &ffi_type_pointer;
- return status;
+ return OK;
}
-static int dynType_parseEnum(FILE *stream, dyn_type *type) {
- int status = OK;
+static int dynType_parseEnum(FILE* stream, dyn_type* type) {
type->ffiType = &ffi_type_sint32;
Review Comment:
IMO sint32 for a enum is good enough. But if I recall correctly the underlying type of a C enum is (compiler) implementation dependent.
##########
libs/dfi/src/dyn_common.c:
##########
@@ -18,124 +18,92 @@
*/
#include "dyn_common.h"
+#include "celix_err.h"
+#include "celix_stdio_cleanup.h"
+#include "celix_stdlib_cleanup.h"
#include <stdlib.h>
#include <stdio.h>
#include <ctype.h>
#include <stdbool.h>
-#if CELIX_UTILS_NO_MEMSTREAM_AVAILABLE
-#include "open_memstream.h"
-#include "fmemopen.h"
-#endif
-
static const int OK = 0;
static const int ERROR = 1;
-DFI_SETUP_LOG(dynCommon)
-
-static bool dynCommon_charIn(int c, const char *acceptedChars);
-int dynCommon_parseName(FILE *stream, char **result) {
+int dynCommon_parseName(FILE* stream, char** result) {
return dynCommon_parseNameAlsoAccept(stream, NULL, result);
}
-int dynCommon_parseNameAlsoAccept(FILE *stream, const char *acceptedChars, char **result) {
- int status = OK;
-
- char *buf = NULL;
+int dynCommon_parseNameAlsoAccept(FILE* stream, const char* acceptedChars, char** result) {
+ celix_autofree char* buf = NULL;
size_t size = 0;
- int strLen = 0;
- FILE *name = open_memstream(&buf, &size);
-
- if (name != NULL) {
- int c = getc(stream);
- while (isalnum(c) || c == '_' || dynCommon_charIn(c, acceptedChars)) {
- fputc(c, name);
- c = getc(stream);
- strLen += 1;
- }
- fflush(name);
- fclose(name);
- ungetc(c, stream);
- } else {
- status = ERROR;
- LOG_ERROR("Error creating mem stream for name. %s", strerror(errno));
+ celix_autoptr(FILE) name = open_memstream(&buf, &size);
+ if (name == NULL) {
+ celix_err_pushf("Error creating mem stream for name. %s", strerror(errno));
+ return ERROR;
}
- if (status == OK) {
- if (strLen == 0) {
- status = ERROR;
- LOG_ERROR("Parsed empty name");
+ int c = getc(stream);
+ while (c != EOF && (isalnum(c) || c == '_' || (acceptedChars != NULL && strchr(acceptedChars, c) != NULL))) {
+ if(fputc(c, name) == EOF) {
+ celix_err_push("Error writing to mem stream for name.");
+ return ERROR;
}
+ c = getc(stream);
+ }
+ if (c != EOF) {
+ ungetc(c, stream);
+ }
+ if(fclose(celix_steal_ptr(name)) != 0) {
+ celix_err_pushf("Error creating mem stream for name. %s", strerror(errno));
Review Comment:
```suggestion
celix_err_pushf("Error closing mem stream for name. %s", strerror(errno));
```
##########
libs/dfi/src/dyn_type.c:
##########
@@ -185,265 +187,200 @@ static int dynType_parseAny(FILE *stream, dyn_type *type) {
return status;
}
-static int dynType_parseMetaInfo(FILE *stream, dyn_type *type) {
+static int dynType_parseMetaInfo(FILE* stream, dyn_type* type) {
int status = OK;
- char *name = NULL;
- char *value = NULL;
-
- struct meta_entry *entry = calloc(1, sizeof(*entry));
- if (entry == NULL) {
- status = ERROR;
- }
+ celix_autofree char* name = NULL;
+ celix_autofree char* value = NULL;
- if (status == OK) {
- status = dynCommon_parseName(stream, &name);
+ if (dynCommon_parseName(stream, &name) != OK) {
+ status = PARSE_ERROR;
+ goto bail_out;
}
-
- if (status == OK) {
- status = dynCommon_eatChar(stream, '=');
+ if (dynCommon_eatChar(stream, '=') != OK) {
+ status = PARSE_ERROR;
+ goto bail_out;
}
-
- if (status == OK) {
- status = dynCommon_parseName(stream, &value);
+ if (dynCommon_parseName(stream, &value) != OK) {
+ status = PARSE_ERROR;
+ goto bail_out;
}
-
- if (status == OK) {
- status = dynCommon_eatChar(stream, ';');
+ if (dynCommon_eatChar(stream, ';') != OK) {
+ status = PARSE_ERROR;
+ goto bail_out;
}
-
- if (status == OK) {
- entry->name = name;
- entry->value = value;
- TAILQ_INSERT_TAIL(&type->metaProperties, entry, entries);
- } else {
- LOG_ERROR("Failed to parse meta properties");
- free(name);
- free(value);
- free(entry);
+ struct meta_entry *entry = calloc(1, sizeof(*entry));
+ if (entry == NULL) {
+ status = MEM_ERROR;
+ goto bail_out;
}
+ entry->name = celix_steal_ptr(name);
+ entry->value = celix_steal_ptr(value);
+ TAILQ_INSERT_TAIL(&type->metaProperties, entry, entries);
+ return OK;
+bail_out:
+ celix_err_push("Failed to parse meta properties");
return status;
}
-static int dynType_parseText(FILE *stream, dyn_type *type) {
- int status = OK;
+static int dynType_parseText(FILE* stream, dyn_type* type) {
type->type = DYN_TYPE_TEXT;
type->descriptor = 't';
+ type->trivial = false;
type->ffiType = &ffi_type_pointer;
- return status;
+ return OK;
}
-static int dynType_parseEnum(FILE *stream, dyn_type *type) {
- int status = OK;
+static int dynType_parseEnum(FILE* stream, dyn_type* type) {
type->ffiType = &ffi_type_sint32;
type->descriptor = 'E';
+ type->trivial = true;
type->type = DYN_TYPE_SIMPLE;
- return status;
+ return OK;
}
-static int dynType_parseComplex(FILE *stream, dyn_type *type) {
+static int dynType_parseComplex(FILE* stream, dyn_type* type) {
+ size_t nbEntries = 0;
int status = OK;
type->type = DYN_TYPE_COMPLEX;
type->descriptor = '{';
+ type->trivial = true;
type->ffiType = &type->complex.structType;
TAILQ_INIT(&type->complex.entriesHead);
int c = fgetc(stream);
- struct complex_type_entry *entry = NULL;
+ struct complex_type_entry* entry = NULL;
while (c != ' ' && c != '}') {
ungetc(c,stream);
+ celix_autoptr(dyn_type) subType = NULL;
+ status = dynType_parseWithStreamOfName(stream, NULL, type, NULL, &subType, dynType_parseAny);
+ if (status != OK) {
+ return status;
+ }
entry = calloc(1, sizeof(*entry));
- if (entry != NULL) {
- entry->type = calloc(1, sizeof(*entry->type));
+ if (entry == NULL) {
+ celix_err_push("Error allocating memory for complex_type_entry");
+ return MEM_ERROR;
}
- if (entry != NULL && entry->type != NULL) {
- entry->type->parent = type;
- entry->type->type = DYN_TYPE_INVALID;
- TAILQ_INIT(&entry->type->nestedTypesHead);
- TAILQ_INIT(&entry->type->metaProperties);
- TAILQ_INSERT_TAIL(&type->complex.entriesHead, entry, entries);
- status = dynType_parseAny(stream, entry->type);
- } else {
- free(entry);
- status = MEM_ERROR;
- LOG_ERROR("Error allocating memory for type");
+ entry->type = celix_steal_ptr(subType);
+ if (!dynType_isTrivial(entry->type)) {
+ type->trivial = false;
Review Comment:
```suggestion
type->trivial = dynType_isTrivial(entry->type);
```
##########
libs/dfi/src/dyn_common.c:
##########
@@ -18,124 +18,92 @@
*/
#include "dyn_common.h"
+#include "celix_err.h"
+#include "celix_stdio_cleanup.h"
+#include "celix_stdlib_cleanup.h"
#include <stdlib.h>
#include <stdio.h>
#include <ctype.h>
#include <stdbool.h>
-#if CELIX_UTILS_NO_MEMSTREAM_AVAILABLE
-#include "open_memstream.h"
-#include "fmemopen.h"
-#endif
-
static const int OK = 0;
static const int ERROR = 1;
-DFI_SETUP_LOG(dynCommon)
-
-static bool dynCommon_charIn(int c, const char *acceptedChars);
-int dynCommon_parseName(FILE *stream, char **result) {
+int dynCommon_parseName(FILE* stream, char** result) {
return dynCommon_parseNameAlsoAccept(stream, NULL, result);
}
-int dynCommon_parseNameAlsoAccept(FILE *stream, const char *acceptedChars, char **result) {
- int status = OK;
-
- char *buf = NULL;
+int dynCommon_parseNameAlsoAccept(FILE* stream, const char* acceptedChars, char** result) {
+ celix_autofree char* buf = NULL;
size_t size = 0;
- int strLen = 0;
- FILE *name = open_memstream(&buf, &size);
-
- if (name != NULL) {
- int c = getc(stream);
- while (isalnum(c) || c == '_' || dynCommon_charIn(c, acceptedChars)) {
- fputc(c, name);
- c = getc(stream);
- strLen += 1;
- }
- fflush(name);
- fclose(name);
- ungetc(c, stream);
- } else {
- status = ERROR;
- LOG_ERROR("Error creating mem stream for name. %s", strerror(errno));
+ celix_autoptr(FILE) name = open_memstream(&buf, &size);
+ if (name == NULL) {
+ celix_err_pushf("Error creating mem stream for name. %s", strerror(errno));
+ return ERROR;
}
- if (status == OK) {
- if (strLen == 0) {
- status = ERROR;
- LOG_ERROR("Parsed empty name");
+ int c = getc(stream);
+ while (c != EOF && (isalnum(c) || c == '_' || (acceptedChars != NULL && strchr(acceptedChars, c) != NULL))) {
+ if(fputc(c, name) == EOF) {
+ celix_err_push("Error writing to mem stream for name.");
+ return ERROR;
}
+ c = getc(stream);
+ }
+ if (c != EOF) {
+ ungetc(c, stream);
+ }
+ if(fclose(celix_steal_ptr(name)) != 0) {
+ celix_err_pushf("Error creating mem stream for name. %s", strerror(errno));
+ return ERROR;
}
- if (status == OK) {
- *result = buf;
- } else if (buf != NULL) {
- free(buf);
+ if (size == 0) {
+ celix_err_push("Parsed empty name");
+ return ERROR;
}
- return status;
+ *result = celix_steal_ptr(buf);
+
+ return OK;
}
-int dynCommon_parseNameValue(FILE *stream, char **outName, char **outValue) {
+static int dynCommon_parseNameValue(FILE* stream, char** outName, char** outValue) {
int status;
- char *name = NULL;
- char *value = NULL;
-
- status = dynCommon_parseName(stream, &name);
- if (status == OK) {
- status = dynCommon_eatChar(stream, '=');
- }
- if (status == OK) {
- const char *valueAcceptedChars = ".<>{}[]?;:~!@#$%^&*()_+-=,./\\'\"";
-
- status = dynCommon_parseNameAlsoAccept(stream, valueAcceptedChars, &value); //NOTE use different more lenient function e.g. only stop at '\n' ?
- }
-
- if (status == OK) {
- *outName = name;
- *outValue = value;
- } else {
- if (name != NULL) {
- free(name);
+ celix_autofree char* name = NULL;
+ celix_autofree char* value = NULL;
+ do {
+ if ((status = dynCommon_parseName(stream, &name)) != OK) {
+ break;
Review Comment:
why a do/while with breaks on error and not an if with an early return?
##########
libs/dfi/src/dyn_descriptor.c:
##########
@@ -0,0 +1,178 @@
+/*
+ * 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 "dyn_descriptor.h"
+#include "celix_err.h"
+#include "celix_stdlib_cleanup.h"
+
+#include <assert.h>
+
+static const int OK = 0;
+static const int ERROR = 1;
+
+static int celix_dynDescriptor_parseTypes(celix_descriptor_t* descriptor, FILE *stream) {
+ int status;
+
+ //expected input (Name)=<Type>\n
+ int peek = fgetc(stream);
+ while (peek != ':' && peek != EOF) {
+ ungetc(peek, stream);
+
+ celix_autofree char* name = NULL;
+ if ((status = dynCommon_parseName(stream, &name)) != OK) {
+ return status;
+ }
+
+ if ((status = dynCommon_eatChar(stream, '=')) != OK) {
+ return status;
+ }
+
+ celix_autoptr(dyn_type) type = NULL;
+ if ((status = dynType_parseOfName(stream, celix_steal_ptr(name), &descriptor->types, &type)) != OK) {
+ return status;
+ }
+ if ((status = dynCommon_eatChar(stream, '\n')) != OK) {
+ return status;
+ }
+
+ struct type_entry *entry = NULL;
+ entry = calloc(1, sizeof(*entry));
+ if (entry == NULL) {
+ celix_err_pushf("Error allocating memory for type entry");
+ return ERROR;
+ }
+ entry->type = celix_steal_ptr(type);
+ TAILQ_INSERT_TAIL(&descriptor->types, entry, entries);
+
+ peek = fgetc(stream);
+ }
+ if (peek != EOF) {
+ ungetc(peek, stream);
+ }
+
+ return OK;
+}
+
+static int celix_dynDescriptor_parseSection(celix_descriptor_t* descriptor, FILE *stream,
+ int (*parseSection)(celix_descriptor_t* descriptor, const char* secName, FILE *stream)) {
+ int status = OK;
+ celix_autofree char *sectionName = NULL;
+
+ if ((status = dynCommon_eatChar(stream, ':')) != OK) {
+ return status;
+ }
+
+ if ((status = dynCommon_parseName(stream, §ionName)) != OK) {
+ return status;
+ }
+
+ if ((status = dynCommon_eatChar(stream, '\n')) != OK) {
+ return status;
+ }
+
+ if (strcmp("header", sectionName) == 0) {
+ status = dynCommon_parseNameValueSection(stream, &descriptor->header);
+ } else if (strcmp("annotations", sectionName) == 0) {
+ status = dynCommon_parseNameValueSection(stream, &descriptor->annotations);
+ } else if (strcmp("types", sectionName) == 0) {
+ status = celix_dynDescriptor_parseTypes(descriptor, stream);
+ } else {
+ status = parseSection(descriptor, sectionName, stream);
+ }
+
+ return status;
+}
+
+static int celix_dynDescriptor_checkInterface(celix_descriptor_t* descriptor) {
+ //check header section
+ bool foundType = false;
+ bool foundVersion = false;
+ bool foundName = false;
+ struct namval_entry *entry = NULL;
+ TAILQ_FOREACH(entry, &descriptor->header, entries) {
+ if (strcmp(entry->name, "type") == 0) {
+ foundType = true;
+ } else if (strcmp(entry->name, "version") == 0) {
+ foundVersion = true;
+ } else if (strcmp(entry->name, "name") == 0) {
+ foundName = true;
+ }
+ }
+
+ if (!foundType || !foundVersion || !foundName) {
+ celix_err_pushf("Parse Error. There must be a header section with a type, version and name entry");
Review Comment:
👍
##########
libs/dfi/src/dyn_type.c:
##########
@@ -185,265 +187,200 @@ static int dynType_parseAny(FILE *stream, dyn_type *type) {
return status;
}
-static int dynType_parseMetaInfo(FILE *stream, dyn_type *type) {
+static int dynType_parseMetaInfo(FILE* stream, dyn_type* type) {
int status = OK;
- char *name = NULL;
- char *value = NULL;
-
- struct meta_entry *entry = calloc(1, sizeof(*entry));
- if (entry == NULL) {
- status = ERROR;
- }
+ celix_autofree char* name = NULL;
+ celix_autofree char* value = NULL;
- if (status == OK) {
- status = dynCommon_parseName(stream, &name);
+ if (dynCommon_parseName(stream, &name) != OK) {
+ status = PARSE_ERROR;
+ goto bail_out;
Review Comment:
goto is needed to reuse the celix_err_push(msg) + return status?
##########
libs/dfi/src/dyn_common.c:
##########
@@ -18,124 +18,92 @@
*/
#include "dyn_common.h"
+#include "celix_err.h"
+#include "celix_stdio_cleanup.h"
+#include "celix_stdlib_cleanup.h"
#include <stdlib.h>
#include <stdio.h>
#include <ctype.h>
#include <stdbool.h>
-#if CELIX_UTILS_NO_MEMSTREAM_AVAILABLE
-#include "open_memstream.h"
-#include "fmemopen.h"
-#endif
-
static const int OK = 0;
static const int ERROR = 1;
-DFI_SETUP_LOG(dynCommon)
-
-static bool dynCommon_charIn(int c, const char *acceptedChars);
-int dynCommon_parseName(FILE *stream, char **result) {
+int dynCommon_parseName(FILE* stream, char** result) {
Review Comment:
Given that celix_status_t now support a custom facility error part, I think the int return can be upgraded to a celix_status_t. And because celix_status_t is a typedef to int, this maybe is even backwards compatible.
But this would be nice as a future enhancement and port of this pull request.
##########
libs/dfi/src/dyn_type.c:
##########
@@ -452,64 +389,60 @@ static int dynType_parseSequence(FILE *stream, dyn_type *type) {
type->sequence.seqType.size = 0;
type->sequence.seqType.alignment = 0;
- status = dynType_parseWithStream(stream, NULL, type, NULL, &type->sequence.itemType);
-
- if (status == OK) {
- type->ffiType = &type->sequence.seqType;
- dynType_prepCif(&type->sequence.seqType);
+ status = dynType_parseWithStreamOfName(stream, NULL, type, NULL, &type->sequence.itemType, dynType_parseAny);
+ if (status != OK) {
+ return status;
}
- return status;
+ type->ffiType = &type->sequence.seqType;
+ (void)ffi_get_struct_offsets(FFI_DEFAULT_ABI, type->ffiType, NULL);
Review Comment:
Nice. Previously this was not done.. was this not needed in most case due to an eventual call to `ffi_prep_cif`?
##########
libs/dfi/src/dyn_type.c:
##########
@@ -185,265 +187,200 @@ static int dynType_parseAny(FILE *stream, dyn_type *type) {
return status;
}
-static int dynType_parseMetaInfo(FILE *stream, dyn_type *type) {
+static int dynType_parseMetaInfo(FILE* stream, dyn_type* type) {
int status = OK;
- char *name = NULL;
- char *value = NULL;
-
- struct meta_entry *entry = calloc(1, sizeof(*entry));
- if (entry == NULL) {
- status = ERROR;
- }
+ celix_autofree char* name = NULL;
+ celix_autofree char* value = NULL;
- if (status == OK) {
- status = dynCommon_parseName(stream, &name);
+ if (dynCommon_parseName(stream, &name) != OK) {
+ status = PARSE_ERROR;
+ goto bail_out;
}
-
- if (status == OK) {
- status = dynCommon_eatChar(stream, '=');
+ if (dynCommon_eatChar(stream, '=') != OK) {
+ status = PARSE_ERROR;
+ goto bail_out;
}
-
- if (status == OK) {
- status = dynCommon_parseName(stream, &value);
+ if (dynCommon_parseName(stream, &value) != OK) {
+ status = PARSE_ERROR;
+ goto bail_out;
}
-
- if (status == OK) {
- status = dynCommon_eatChar(stream, ';');
+ if (dynCommon_eatChar(stream, ';') != OK) {
+ status = PARSE_ERROR;
+ goto bail_out;
}
-
- if (status == OK) {
- entry->name = name;
- entry->value = value;
- TAILQ_INSERT_TAIL(&type->metaProperties, entry, entries);
- } else {
- LOG_ERROR("Failed to parse meta properties");
- free(name);
- free(value);
- free(entry);
+ struct meta_entry *entry = calloc(1, sizeof(*entry));
+ if (entry == NULL) {
+ status = MEM_ERROR;
+ goto bail_out;
}
+ entry->name = celix_steal_ptr(name);
+ entry->value = celix_steal_ptr(value);
+ TAILQ_INSERT_TAIL(&type->metaProperties, entry, entries);
+ return OK;
+bail_out:
+ celix_err_push("Failed to parse meta properties");
return status;
}
-static int dynType_parseText(FILE *stream, dyn_type *type) {
- int status = OK;
+static int dynType_parseText(FILE* stream, dyn_type* type) {
type->type = DYN_TYPE_TEXT;
type->descriptor = 't';
+ type->trivial = false;
type->ffiType = &ffi_type_pointer;
- return status;
+ return OK;
}
-static int dynType_parseEnum(FILE *stream, dyn_type *type) {
- int status = OK;
+static int dynType_parseEnum(FILE* stream, dyn_type* type) {
type->ffiType = &ffi_type_sint32;
type->descriptor = 'E';
+ type->trivial = true;
type->type = DYN_TYPE_SIMPLE;
- return status;
+ return OK;
}
-static int dynType_parseComplex(FILE *stream, dyn_type *type) {
+static int dynType_parseComplex(FILE* stream, dyn_type* type) {
+ size_t nbEntries = 0;
int status = OK;
type->type = DYN_TYPE_COMPLEX;
type->descriptor = '{';
+ type->trivial = true;
Review Comment:
nitpick: not needed, will be set in line 266
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@celix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Feature/dfi cleanup (celix)
Posted by "pnoltes (via GitHub)" <gi...@apache.org>.
pnoltes commented on code in PR #699:
URL: https://github.com/apache/celix/pull/699#discussion_r1476106806
##########
libs/dfi/src/dyn_function.c:
##########
@@ -30,141 +33,134 @@ static const int MEM_ERROR = 1;
static const int PARSE_ERROR = 2;
static const int ERROR = 2;
-DFI_SETUP_LOG(dynFunction)
+static int dynFunction_initCif(dyn_function_type* dynFunc);
+static int dynFunction_parseDescriptor(dyn_function_type* dynFunc, FILE* descriptor);
+static void dynFunction_ffiBind(ffi_cif* cif, void* ret, void* args[], void* userData);
-static int dynFunction_initCif(dyn_function_type *dynFunc);
-static int dynFunction_parseDescriptor(dyn_function_type *dynFunc, FILE *descriptor);
-static void dynFunction_ffiBind(ffi_cif *cif, void *ret, void *args[], void *userData);
-
-ffi_type * dynType_ffiType(dyn_type *type);
-
-int dynFunction_parse(FILE *descriptor, struct types_head *refTypes, dyn_function_type **out) {
+int dynFunction_parse(FILE* descriptor, struct types_head* refTypes, dyn_function_type** out) {
int status = OK;
- dyn_function_type *dynFunc = NULL;
-
- dynFunc = calloc(1, sizeof(*dynFunc));
+ celix_autoptr(dyn_function_type) dynFunc = calloc(1, sizeof(*dynFunc));
+ if (dynFunc == NULL) {
+ celix_err_pushf("Error allocating memory for dyn function");
+ return MEM_ERROR;
+ }
- if (dynFunc != NULL) {
- TAILQ_INIT(&dynFunc->arguments);
- dynFunc->refTypes = refTypes;
- status = dynFunction_parseDescriptor(dynFunc, descriptor);
- if (status == 0) {
- int rc = dynFunction_initCif(dynFunc);
- if (rc != 0) {
- LOG_ERROR("Error initializing cif");
- status = ERROR;
- }
- }
- } else {
- LOG_ERROR("Error allocating memory for dyn function\n");
- status = MEM_ERROR;
+ TAILQ_INIT(&dynFunc->arguments);
+ dynFunc->refTypes = refTypes;
+ status = dynFunction_parseDescriptor(dynFunc, descriptor);
+ if (status != OK) {
+ celix_err_pushf("Error parsing descriptor");
+ return status;
+ }
+ status = dynFunction_initCif(dynFunc);
+ if (status != OK) {
+ return status;
}
- if (status == OK) {
- dyn_function_argument_type *arg = NULL;
- TAILQ_FOREACH(arg, &dynFunc->arguments, entries) {
- const char *meta = dynType_getMetaInfo(arg->type, "am");
- if (meta == NULL) {
- arg->argumentMeta = DYN_FUNCTION_ARGUMENT_META__STD;
- } else if (strcmp(meta, "handle") == 0) {
- arg->argumentMeta = DYN_FUNCTION_ARGUMENT_META__HANDLE;
- } else if (strcmp(meta, "pre") == 0) {
- arg->argumentMeta = DYN_FUNCTION_ARGUMENT_META__PRE_ALLOCATED_OUTPUT;
- } else if (strcmp(meta, "out") == 0) {
- arg->argumentMeta = DYN_FUNCTION_ARGUMENT_META__OUTPUT;
- } else {
- arg->argumentMeta = DYN_FUNCTION_ARGUMENT_META__STD;
+ dyn_function_argument_type* arg = NULL;
+ TAILQ_FOREACH(arg, &dynFunc->arguments, entries) {
+ const dyn_type* real = dynType_realType(arg->type);
+ const char* meta = dynType_getMetaInfo(arg->type, "am");
+ if (meta == NULL) {
+ arg->argumentMeta = DYN_FUNCTION_ARGUMENT_META__STD;
+ } else if (strcmp(meta, "handle") == 0) {
+ if (dynType_descriptorType(real) != 'P') {
+ celix_err_pushf("Error 'handle' is only allowed for untyped pointer not '%c'", dynType_descriptorType(real));
+ return PARSE_ERROR;
+ }
+ arg->argumentMeta = DYN_FUNCTION_ARGUMENT_META__HANDLE;
+ } else if (strcmp(meta, "pre") == 0) {
+ if (dynType_type(real) != DYN_TYPE_TYPED_POINTER) {
+ celix_err_pushf("Error 'pre' is only allowed for typed pointer not '%c'", dynType_descriptorType(real));
+ return PARSE_ERROR;
+ }
+ const dyn_type* sub = dynType_typedPointer_getTypedType(real);
+ if (!dynType_isTrivial(sub)) {
+ celix_err_pushf("Error 'pre' is only allowed for pointer to trivial types not non-trivial '%c'", dynType_descriptorType(sub));
Review Comment:
👍
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@celix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Feature/dfi cleanup (celix)
Posted by "pnoltes (via GitHub)" <gi...@apache.org>.
pnoltes commented on code in PR #699:
URL: https://github.com/apache/celix/pull/699#discussion_r1476105050
##########
libs/dfi/include/dyn_type.h:
##########
@@ -64,8 +59,8 @@ extern "C" {
* //Extended
* b unsigned char
* i uint32_t
- * j uint62_t
- * s uint64_t
+ * j uint64_t
+ * s uint16_t
Review Comment:
nice catch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@celix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Feature/dfi cleanup (celix)
Posted by "pnoltes (via GitHub)" <gi...@apache.org>.
pnoltes commented on PR #699:
URL: https://github.com/apache/celix/pull/699#issuecomment-1913683356
I will try to find to time to review this PR, this week.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@celix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WIP]Feature/dfi cleanup (celix)
Posted by "PengZheng (via GitHub)" <gi...@apache.org>.
PengZheng commented on PR #699:
URL: https://github.com/apache/celix/pull/699#issuecomment-1905582200
> I agree with both. And making a nullptr possible is good improvement I had not considered.
> Concerning JSON, If you treat the descriptor as a low level (JSON) schema, it is also logical that the expected JSON should have the expected members.
These two issues have been addressed. I have questions regarding function argument meta info: what are the precise definitions of these argument types?
For `am=handle`, it is clear that a handle should be untyped pointer, with descriptor 'P'.
For `am=pre`, the Doxygen documentation says that the type should be pre-allocated by the function caller. Let me call these types "pre-allocatable". The following plain old C struct should qualify as pre-allocatable:
```C
struct preallocatable {
int a;
int b;
}
```
Is text pre-allocatable? I guess not, since the caller can not predict how long a string will be.
```C
struct not_sure {
int a;
int b;
struct preallocatable* c;
}
```
Is `not_sure` pre-allocatable? On the service provider side, the callee could allocate a struct preallocatable and fill the c field.
Then `not_sure` can be serialized successfully. However, on the service consumer side, it can caused a use-after-free crash.
In `jsonRpc_handleReply`, we deal with pre-allocatable argument like following:
```C
if (meta == DYN_FUNCTION_ARGUMENT_META__PRE_ALLOCATED_OUTPUT) {
void* tmp = NULL;
void** out = (void **) args[i];
size_t size = 0;
if (dynType_descriptorType(argType) == 't') {
status = jsonSerializer_deserializeJson(argType, result, &tmp);
if (tmp != NULL) {
size = strnlen(((char *) *(char**) tmp), 1024 * 1024);
memcpy(*out, *(void**) tmp, size);
}
} else {
argType = dynType_typedPointer_getTypedType(argType);
status = jsonSerializer_deserializeJson(argType, result, &tmp);
if (tmp != NULL) {
size = dynType_size(argType);
memcpy(*out, tmp, size); // address of freed struct preallocatable is copied
}
}
dynType_free(argType, tmp); // preallocatable is freed
}
```
If we do have a precise definition of pre-allocatability, such errors can be catched in `dynFunction_parse`:
```C
if (strcmp(meta, "handle") == 0) {
if (dynType_descriptorType(real) != 'P') {
celix_err_pushf("Error 'handle' is only allowed for untyped pointer not '%c'", dynType_descriptorType(real));
return PARSE_ERROR;
}
arg->argumentMeta = DYN_FUNCTION_ARGUMENT_META__HANDLE;
} else if (strcmp(meta, "pre") == 0) {
if (!dynType_isPreallocatable(real)) {
celix_err_pushf("Error 'am=pre' is only allowed for preallocatable type");
return PARSE_ERROR;
}
arg->argumentMeta = DYN_FUNCTION_ARGUMENT_META__PRE_ALLOCATED_OUTPUT;
}
```
Similar considerations also apply to other types. For example, for a standard argument to work, it should at least be serializable.
For exmaple, `int` is serializable, `int *` is serailizable, while `int**` is not, because we can not tell from a json null whether `int* == NULL` or `int** == NULL`.
Due to the lack of precise definitions of these argument types, I am stuck in json_rpc.c. Though it is already 100% line covered, it is still very fragile IMHO. @pnoltes
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@celix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Feature/dfi cleanup (celix)
Posted by "pnoltes (via GitHub)" <gi...@apache.org>.
pnoltes commented on code in PR #699:
URL: https://github.com/apache/celix/pull/699#discussion_r1476096383
##########
libs/dfi/gtest/descriptors/invalids/outArgAtWrongPosition.descriptor:
##########
Review Comment:
nice to see the large set of invalid descriptors test cases
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@celix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Feature/dfi cleanup (celix)
Posted by "PengZheng (via GitHub)" <gi...@apache.org>.
PengZheng commented on code in PR #699:
URL: https://github.com/apache/celix/pull/699#discussion_r1472339359
##########
libs/dfi/src/dyn_common.c:
##########
@@ -18,124 +18,92 @@
*/
#include "dyn_common.h"
+#include "celix_err.h"
+#include "celix_stdio_cleanup.h"
+#include "celix_stdlib_cleanup.h"
#include <stdlib.h>
#include <stdio.h>
#include <ctype.h>
#include <stdbool.h>
-#if CELIX_UTILS_NO_MEMSTREAM_AVAILABLE
-#include "open_memstream.h"
-#include "fmemopen.h"
-#endif
-
static const int OK = 0;
static const int ERROR = 1;
-DFI_SETUP_LOG(dynCommon)
-
-static bool dynCommon_charIn(int c, const char *acceptedChars);
-int dynCommon_parseName(FILE *stream, char **result) {
+int dynCommon_parseName(FILE* stream, char** result) {
Review Comment:
Totally agree!
I actually resisted the temptation of a uniform error code in this PR and instead focused on understanding libdfi.
It will be done in another PR, before descriptor generation happens (#724).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@celix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Feature/dfi cleanup (celix)
Posted by "PengZheng (via GitHub)" <gi...@apache.org>.
PengZheng commented on code in PR #699:
URL: https://github.com/apache/celix/pull/699#discussion_r1472768662
##########
libs/dfi/src/dyn_type.c:
##########
@@ -185,265 +187,200 @@ static int dynType_parseAny(FILE *stream, dyn_type *type) {
return status;
}
-static int dynType_parseMetaInfo(FILE *stream, dyn_type *type) {
+static int dynType_parseMetaInfo(FILE* stream, dyn_type* type) {
int status = OK;
- char *name = NULL;
- char *value = NULL;
-
- struct meta_entry *entry = calloc(1, sizeof(*entry));
- if (entry == NULL) {
- status = ERROR;
- }
+ celix_autofree char* name = NULL;
+ celix_autofree char* value = NULL;
- if (status == OK) {
- status = dynCommon_parseName(stream, &name);
+ if (dynCommon_parseName(stream, &name) != OK) {
+ status = PARSE_ERROR;
+ goto bail_out;
}
-
- if (status == OK) {
- status = dynCommon_eatChar(stream, '=');
+ if (dynCommon_eatChar(stream, '=') != OK) {
+ status = PARSE_ERROR;
+ goto bail_out;
}
-
- if (status == OK) {
- status = dynCommon_parseName(stream, &value);
+ if (dynCommon_parseName(stream, &value) != OK) {
+ status = PARSE_ERROR;
+ goto bail_out;
}
-
- if (status == OK) {
- status = dynCommon_eatChar(stream, ';');
+ if (dynCommon_eatChar(stream, ';') != OK) {
+ status = PARSE_ERROR;
+ goto bail_out;
}
-
- if (status == OK) {
- entry->name = name;
- entry->value = value;
- TAILQ_INSERT_TAIL(&type->metaProperties, entry, entries);
- } else {
- LOG_ERROR("Failed to parse meta properties");
- free(name);
- free(value);
- free(entry);
+ struct meta_entry *entry = calloc(1, sizeof(*entry));
+ if (entry == NULL) {
+ status = MEM_ERROR;
+ goto bail_out;
}
+ entry->name = celix_steal_ptr(name);
+ entry->value = celix_steal_ptr(value);
+ TAILQ_INSERT_TAIL(&type->metaProperties, entry, entries);
+ return OK;
+bail_out:
+ celix_err_push("Failed to parse meta properties");
return status;
}
-static int dynType_parseText(FILE *stream, dyn_type *type) {
- int status = OK;
+static int dynType_parseText(FILE* stream, dyn_type* type) {
type->type = DYN_TYPE_TEXT;
type->descriptor = 't';
+ type->trivial = false;
type->ffiType = &ffi_type_pointer;
- return status;
+ return OK;
}
-static int dynType_parseEnum(FILE *stream, dyn_type *type) {
- int status = OK;
+static int dynType_parseEnum(FILE* stream, dyn_type* type) {
type->ffiType = &ffi_type_sint32;
Review Comment:
After consulting gcc's manual ([1]) and C/C++ standards ([2]), my conclusion is: we'd better stick with sint32 until C23 becomes widely used. It's unfortunate that enumeration of fixed underlying type won't be available to C until C23, which is at least five years away.
That is not to say we are not able to help our users to avoid pitfalls. For example, we can provide a auxiliary macso that can be used to decorate enum declarations. Once decorated, an implicit enumerator will be inserted in the end of enumerator-list with a value of `INT32_MAX` together with a build-time assertion claiming `sizeof(enum)==4`. That should help catch most errors of this kind, like the ones caused by GCC option `-fshort-enums`.
I suggest we reconsider this when implementing descriptor generator. At that time, we are at a better position, since we can see things from a point of view of compiler. IMHO, such rules as the ones current implemented by dyn_function/dyn_interface/serializer should be enforced as much as possible by the descriptor generator.
[1] https://gcc.gnu.org/onlinedocs/gcc-13.2.0/gcc/Structures-unions-enumerations-and-bit-fields-implementation.html
[2] https://en.cppreference.com/w/c/language/enum
##########
libs/dfi/src/dyn_type.c:
##########
@@ -185,265 +187,200 @@ static int dynType_parseAny(FILE *stream, dyn_type *type) {
return status;
}
-static int dynType_parseMetaInfo(FILE *stream, dyn_type *type) {
+static int dynType_parseMetaInfo(FILE* stream, dyn_type* type) {
int status = OK;
- char *name = NULL;
- char *value = NULL;
-
- struct meta_entry *entry = calloc(1, sizeof(*entry));
- if (entry == NULL) {
- status = ERROR;
- }
+ celix_autofree char* name = NULL;
+ celix_autofree char* value = NULL;
- if (status == OK) {
- status = dynCommon_parseName(stream, &name);
+ if (dynCommon_parseName(stream, &name) != OK) {
+ status = PARSE_ERROR;
+ goto bail_out;
}
-
- if (status == OK) {
- status = dynCommon_eatChar(stream, '=');
+ if (dynCommon_eatChar(stream, '=') != OK) {
+ status = PARSE_ERROR;
+ goto bail_out;
}
-
- if (status == OK) {
- status = dynCommon_parseName(stream, &value);
+ if (dynCommon_parseName(stream, &value) != OK) {
+ status = PARSE_ERROR;
+ goto bail_out;
}
-
- if (status == OK) {
- status = dynCommon_eatChar(stream, ';');
+ if (dynCommon_eatChar(stream, ';') != OK) {
+ status = PARSE_ERROR;
+ goto bail_out;
}
-
- if (status == OK) {
- entry->name = name;
- entry->value = value;
- TAILQ_INSERT_TAIL(&type->metaProperties, entry, entries);
- } else {
- LOG_ERROR("Failed to parse meta properties");
- free(name);
- free(value);
- free(entry);
+ struct meta_entry *entry = calloc(1, sizeof(*entry));
+ if (entry == NULL) {
+ status = MEM_ERROR;
+ goto bail_out;
}
+ entry->name = celix_steal_ptr(name);
+ entry->value = celix_steal_ptr(value);
+ TAILQ_INSERT_TAIL(&type->metaProperties, entry, entries);
+ return OK;
+bail_out:
+ celix_err_push("Failed to parse meta properties");
return status;
}
-static int dynType_parseText(FILE *stream, dyn_type *type) {
- int status = OK;
+static int dynType_parseText(FILE* stream, dyn_type* type) {
type->type = DYN_TYPE_TEXT;
type->descriptor = 't';
+ type->trivial = false;
type->ffiType = &ffi_type_pointer;
- return status;
+ return OK;
}
-static int dynType_parseEnum(FILE *stream, dyn_type *type) {
- int status = OK;
+static int dynType_parseEnum(FILE* stream, dyn_type* type) {
type->ffiType = &ffi_type_sint32;
Review Comment:
After consulting GCC's manual ([1]) and C/C++ standards ([2]), my conclusion is: we'd better stick with sint32 until C23 becomes widely used. It's unfortunate that enumeration of fixed underlying type won't be available to C until C23, which is at least five years away.
That is not to say we are not able to help our users to avoid pitfalls. For example, we can provide a auxiliary macso that can be used to decorate enum declarations. Once decorated, an implicit enumerator will be inserted in the end of enumerator-list with a value of `INT32_MAX` together with a build-time assertion claiming `sizeof(enum)==4`. That should help catch most errors of this kind, like the ones caused by GCC option `-fshort-enums`.
I suggest we reconsider this when implementing descriptor generator. At that time, we are at a better position, since we can see things from a point of view of compiler. IMHO, such rules as the ones current implemented by dyn_function/dyn_interface/serializer should be enforced as much as possible by the descriptor generator.
[1] https://gcc.gnu.org/onlinedocs/gcc-13.2.0/gcc/Structures-unions-enumerations-and-bit-fields-implementation.html
[2] https://en.cppreference.com/w/c/language/enum
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@celix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Feature/dfi cleanup (celix)
Posted by "PengZheng (via GitHub)" <gi...@apache.org>.
PengZheng merged PR #699:
URL: https://github.com/apache/celix/pull/699
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@celix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WIP]Feature/dfi cleanup (celix)
Posted by "pnoltes (via GitHub)" <gi...@apache.org>.
pnoltes commented on PR #699:
URL: https://github.com/apache/celix/pull/699#issuecomment-1894351370
> When I modified `TEST_F(JsonSerializerTests, ParseTests)` as following, a crash happened:
>
> ```c++
> type = nullptr;
> inst = nullptr;
> rc = dynType_parseWithStr(example5_descriptor, nullptr, nullptr, &type);
> ASSERT_EQ(0, rc);
> rc = jsonSerializer_deserialize(type, example5_input, strlen(example5_input), &inst);
> ASSERT_EQ(0, rc);
>
> json_auto_t* result = nullptr;
> // crash happens HERE!
> rc = jsonSerializer_serializeJson(type, inst, &result);
> ASSERT_EQ(0, rc);
> ```
>
> I found two issues:
>
> 1. In `example5_input`, the `node` pointed to by `head` misses `value` member. Similarly, the `node` pointed to by `head->left` misses `right` member.
> 2. `nullptr` is not dealt in `jsonSerializer_serializeJson`, which leads to the crash.
>
> ```c++
> case '*' :
> subType = dynType_typedPointer_getTypedType(type);
> status = jsonSerializer_writeAny(subType, *(void **)input, &val); // dereference null ptr
> break;
> ```
>
> For the first issue, IMO all struct members should be present in the json object, because C struct is very rigid, unlike json object. Our data model is based on C struct not on flexible json object.
>
> For the second issue, I think `nullptr` should also be serialized/deserialized.
>
> WDYT? @pnoltes
I agree with both. And making a nullptr possible is good improvement I had not considered.
Concerning JSON, If you treat the descriptor as a low level (JSON) schema, it is also logical that the expected JSON should have the expected members.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@celix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org