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-unsubscr...@celix.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
