You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@celix.apache.org by "xuzhenbao (via GitHub)" <gi...@apache.org> on 2023/05/31 02:47:53 UTC
[PR] Add logHelper function with tss errors (celix)
xuzhenbao opened a new pull request, #565:
URL: https://github.com/apache/celix/pull/565
1.Add a logHelper function with tss erros
2.Fix #539 #503
--
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] Add logHelper function with tss errors (celix)
Posted by "PengZheng (via GitHub)" <gi...@apache.org>.
PengZheng commented on code in PR #565:
URL: https://github.com/apache/celix/pull/565#discussion_r1211056498
##########
bundles/logging/log_helper/src/celix_log_helper.c:
##########
@@ -161,6 +162,35 @@ void celix_logHelper_vlogDetails(celix_log_helper_t* logHelper, celix_log_level_
}
}
+void celix_logHelper_logWithTssErrors(celix_log_helper_t* logHelper, celix_log_level_e level, const char *format, ...) {
+ va_list args;
+ va_start(args, format);
+ celix_logHelper_vlogDetailsWithTssErrors(logHelper, level, NULL, NULL, 0, format, args);
+ va_end(args);
+}
+
+void celix_logHelper_vlogDetailsWithTssErrors(celix_log_helper_t* logHelper, celix_log_level_e level, const char* file, const char* function, int line, const char *format, va_list formatArgs) {
+ if (celix_err_getErrorCount() == 0) {
+ celix_logHelper_vlogDetails(logHelper, level, file, function, line, format, formatArgs);
+ return;
+ }
+ char buf[512] = {0};
+ int ret;
+ int bytes = vsnprintf(buf, sizeof(buf), format, formatArgs);
+ if (bytes < 0) {
+ bytes = 0;
Review Comment:
No test case for this condition.
##########
bundles/logging/log_helper/src/celix_log_helper.c:
##########
@@ -161,6 +162,35 @@ void celix_logHelper_vlogDetails(celix_log_helper_t* logHelper, celix_log_level_
}
}
+void celix_logHelper_logWithTssErrors(celix_log_helper_t* logHelper, celix_log_level_e level, const char *format, ...) {
+ va_list args;
+ va_start(args, format);
+ celix_logHelper_vlogDetailsWithTssErrors(logHelper, level, NULL, NULL, 0, format, args);
+ va_end(args);
+}
+
+void celix_logHelper_vlogDetailsWithTssErrors(celix_log_helper_t* logHelper, celix_log_level_e level, const char* file, const char* function, int line, const char *format, va_list formatArgs) {
+ if (celix_err_getErrorCount() == 0) {
+ celix_logHelper_vlogDetails(logHelper, level, file, function, line, format, formatArgs);
+ return;
+ }
+ char buf[512] = {0};
+ int ret;
+ int bytes = vsnprintf(buf, sizeof(buf), format, formatArgs);
+ if (bytes < 0) {
+ bytes = 0;
+ }
+ for (const char *errMsg = celix_err_popLastError(); errMsg != NULL && bytes < sizeof(buf); errMsg = celix_err_popLastError()) {
+ ret = snprintf(buf + bytes, sizeof(buf) - bytes, "\n%s", errMsg);
+ if (ret > 0) {
+ bytes += ret;
+ }
+ }
+ celix_err_resetErrors();
+ //celix_logHelper_logDetails will add '\n' at the end of the message, so no need to add it here.
+ celix_logHelper_logDetails(logHelper, level, file, function, line, "%s", buf);
Review Comment:
A test case illustrating buf is always nul-terminated even in boundary conditions will be very informative.
##########
bundles/logging/log_helper/src/celix_log_helper.c:
##########
@@ -161,6 +162,35 @@ void celix_logHelper_vlogDetails(celix_log_helper_t* logHelper, celix_log_level_
}
}
+void celix_logHelper_logWithTssErrors(celix_log_helper_t* logHelper, celix_log_level_e level, const char *format, ...) {
+ va_list args;
+ va_start(args, format);
+ celix_logHelper_vlogDetailsWithTssErrors(logHelper, level, NULL, NULL, 0, format, args);
+ va_end(args);
+}
+
+void celix_logHelper_vlogDetailsWithTssErrors(celix_log_helper_t* logHelper, celix_log_level_e level, const char* file, const char* function, int line, const char *format, va_list formatArgs) {
+ if (celix_err_getErrorCount() == 0) {
+ celix_logHelper_vlogDetails(logHelper, level, file, function, line, format, formatArgs);
+ return;
+ }
+ char buf[512] = {0};
+ int ret;
+ int bytes = vsnprintf(buf, sizeof(buf), format, formatArgs);
+ if (bytes < 0) {
+ bytes = 0;
+ }
+ for (const char *errMsg = celix_err_popLastError(); errMsg != NULL && bytes < sizeof(buf); errMsg = celix_err_popLastError()) {
Review Comment:
No test case for this boundary conditions.
##########
libs/dfi/include/avrobin_serializer.h:
##########
@@ -32,12 +32,65 @@ extern "C" {
//logging
DFI_SETUP_LOG_HEADER(avrobinSerializer);
+/**
+ * @brief Deserialize data in AVRO format.
+ *
+ * The caller is the owner of the deserialized data and use `dynType_free` deallocate the memory.
+ *
+ * In case of a error, an error message is added to celix_err.
Review Comment:
```suggestion
* In case of an error, an error message is added to celix_err.
```
Typo. I guess this is not the only one.
##########
libs/dfi/src/json_rpc.c:
##########
@@ -345,7 +339,7 @@ int jsonRpc_handleReply(dyn_function_type *func, const char *reply, void *args[]
size_t size = 0;
if (result == NULL) {
- LOG_WARNING("Expected result in reply. got '%s'", reply);
+ celix_err_pushf("Expected result in reply. got '%s'", reply);
Review Comment:
If this does not change return status, should it be pushed into ERR?
##########
libs/dfi/src/dyn_type.c:
##########
@@ -731,7 +732,7 @@ void dynType_deepFree(dyn_type *type, void *loc, bool alsoDeleteSelf) {
//nop
break;
default:
- LOG_ERROR("Unexpected switch case. cannot free dyn type %c\n", type->descriptor);
+ celix_err_pushf("Unexpected switch case. cannot free dyn type %c\n", type->descriptor);
Review Comment:
Why not `#define LOG_ERROR celix_err_pushf`?
##########
bundles/logging/log_helper/gtest/src/LogHelperTestSuite.cc:
##########
@@ -53,7 +54,13 @@ TEST_F(LogHelperTestSuite, LogToStdOut) {
celix_logHelper_warning(helper, "testing %i", 3);
celix_logHelper_error(helper, "testing %i", 4);
celix_logHelper_fatal(helper, "testing %i", 5);
- EXPECT_EQ(5, celix_logHelper_logCount(helper));
+ celix_logHelper_logWithTssErrors(helper, CELIX_LOG_LEVEL_ERROR, "testing %i", 6);
+ for (int i = 0; i < 32; ++i) {
+ celix_err_pushf("celix error message%d", i);
+ }
+ celix_logHelper_logWithTssErrors(helper, CELIX_LOG_LEVEL_ERROR, "testing %i", 7);
+ celix_logHelper_logWithTssErrors(helper, CELIX_LOG_LEVEL_ERROR, "testing %i", 8);
+ EXPECT_EQ(8, celix_logHelper_logCount(helper));
Review Comment:
If there is a test case verifying the output format, our user can easily know what the output will look like, which is very helpful.
--
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] Add logHelper function with tss errors (celix)
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #565:
URL: https://github.com/apache/celix/pull/565#issuecomment-1592776655
## [Codecov](https://app.codecov.io/gh/apache/celix/pull/565?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#565](https://app.codecov.io/gh/apache/celix/pull/565?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (701e719) into [master](https://app.codecov.io/gh/apache/celix/commit/cd5a44cfc082d8972e9fc00786f4a103a4cfd790?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (cd5a44c) will **increase** coverage by `0.12%`.
> The diff coverage is `75.58%`.
> :exclamation: Current head 701e719 differs from pull request most recent head 44e19ae. Consider uploading reports for the commit 44e19ae to get more accurate results
```diff
@@ Coverage Diff @@
## master #565 +/- ##
==========================================
+ Coverage 77.83% 77.95% +0.12%
==========================================
Files 230 229 -1
Lines 35031 35002 -29
==========================================
+ Hits 27267 27287 +20
+ Misses 7764 7715 -49
```
| [Impacted Files](https://app.codecov.io/gh/apache/celix/pull/565?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...xx\_remote\_services/admin/src/RemoteServiceAdmin.cc](https://app.codecov.io/gh/apache/celix/pull/565?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-YnVuZGxlcy9jeHhfcmVtb3RlX3NlcnZpY2VzL2FkbWluL3NyYy9SZW1vdGVTZXJ2aWNlQWRtaW4uY2M=) | `78.70% <ø> (ø)` | |
| [...e\_service\_admin\_dfi/src/remote\_service\_admin\_dfi.c](https://app.codecov.io/gh/apache/celix/pull/565?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-YnVuZGxlcy9yZW1vdGVfc2VydmljZXMvcmVtb3RlX3NlcnZpY2VfYWRtaW5fZGZpL3NyYy9yZW1vdGVfc2VydmljZV9hZG1pbl9kZmkuYw==) | `84.46% <ø> (-0.37%)` | :arrow_down: |
| [...dles/remote\_services/rsa\_dfi\_utils/src/dfi\_utils.c](https://app.codecov.io/gh/apache/celix/pull/565?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-YnVuZGxlcy9yZW1vdGVfc2VydmljZXMvcnNhX2RmaV91dGlscy9zcmMvZGZpX3V0aWxzLmM=) | `84.14% <0.00%> (-2.11%)` | :arrow_down: |
| [libs/dfi/src/avrobin\_serializer.c](https://app.codecov.io/gh/apache/celix/pull/565?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-bGlicy9kZmkvc3JjL2F2cm9iaW5fc2VyaWFsaXplci5j) | `60.37% <0.00%> (-0.05%)` | :arrow_down: |
| [libs/dfi/src/dyn\_avpr\_function.c](https://app.codecov.io/gh/apache/celix/pull/565?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-bGlicy9kZmkvc3JjL2R5bl9hdnByX2Z1bmN0aW9uLmM=) | `64.88% <0.00%> (-0.44%)` | :arrow_down: |
| [libs/dfi/src/dyn\_avpr\_interface.c](https://app.codecov.io/gh/apache/celix/pull/565?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-bGlicy9kZmkvc3JjL2R5bl9hdnByX2ludGVyZmFjZS5j) | `70.92% <0.00%> (-0.21%)` | :arrow_down: |
| [libs/dfi/src/dyn\_common.c](https://app.codecov.io/gh/apache/celix/pull/565?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-bGlicy9kZmkvc3JjL2R5bl9jb21tb24uYw==) | `86.11% <ø> (-0.20%)` | :arrow_down: |
| [libs/dfi/src/dyn\_type.c](https://app.codecov.io/gh/apache/celix/pull/565?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-bGlicy9kZmkvc3JjL2R5bl90eXBlLmM=) | `90.83% <0.00%> (-0.28%)` | :arrow_down: |
| [libs/dfi/src/dyn\_type\_common.c](https://app.codecov.io/gh/apache/celix/pull/565?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-bGlicy9kZmkvc3JjL2R5bl90eXBlX2NvbW1vbi5j) | `92.85% <ø> (-0.48%)` | :arrow_down: |
| [libs/dfi/src/json\_serializer.c](https://app.codecov.io/gh/apache/celix/pull/565?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-bGlicy9kZmkvc3JjL2pzb25fc2VyaWFsaXplci5j) | `82.83% <0.00%> (-0.26%)` | :arrow_down: |
| ... and [24 more](https://app.codecov.io/gh/apache/celix/pull/565?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | |
... and [4 files with indirect coverage changes](https://app.codecov.io/gh/apache/celix/pull/565/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?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] Add logHelper function with tss errors (celix)
Posted by "pnoltes (via GitHub)" <gi...@apache.org>.
pnoltes commented on code in PR #565:
URL: https://github.com/apache/celix/pull/565#discussion_r1213506302
##########
libs/dfi/src/dyn_type.c:
##########
@@ -731,7 +732,7 @@ void dynType_deepFree(dyn_type *type, void *loc, bool alsoDeleteSelf) {
//nop
break;
default:
- LOG_ERROR("Unexpected switch case. cannot free dyn type %c\n", type->descriptor);
+ celix_err_pushf("Unexpected switch case. cannot free dyn type %c\n", type->descriptor);
Review Comment:
agree
##########
bundles/logging/log_helper/include/celix_log_helper.h:
##########
@@ -90,6 +90,14 @@ void celix_logHelper_logDetails(celix_log_helper_t* logHelper,
int line,
const char *format, ...) __attribute__((format(printf,6,7)));
+/**
+ * @brief Logs a message using the provided celix log level to the log_helper, printf style.
+ * It will also print celix thread-specific storage error messages(celix_err).
+ *
+ * Silently ignores log level CELIX_LOG_LEVEL_DISABLED.
+ */
+void celix_logHelper_logWithTssErrors(celix_log_helper_t* logHelper, celix_log_level_e level, const char *format, ...) __attribute__((format(printf,3,4)));
Review Comment:
Is it an idea to only add a `celix_logHelper_logTssErrors(celix_log_helper_t* logHelper, celix_log_level_e level)` function and a C++ `celix::LogHelper::logTssErrors(celix_log_level_e level)`?
This requires an extra call for the users (using log helper to log a message and then log celix err errors), but prevents that we (ideally) create an alternative for the `log`, `vlog`, `logDetails` and `vlogDetails` in C and C++.
--
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] Add logHelper function with tss errors (celix)
Posted by "xuzhenbao (via GitHub)" <gi...@apache.org>.
xuzhenbao merged PR #565:
URL: https://github.com/apache/celix/pull/565
--
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