-
Notifications
You must be signed in to change notification settings - Fork 20
RDK-60318 [POC][T2] Evaluate Rbus IPC Alternatives for T2 Component Integration #248
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
base: develop
Are you sure you want to change the base?
Conversation
cmuhammedrafi
commented
Jan 27, 2026
- Dubs initial implementation with test code
| char *marker = (char*)Vector_At(markerList, i); | ||
| if (marker) { | ||
| if (i > 0) strcat(markerListStr, ","); | ||
| strcat(markerListStr, marker); |
Check failure
Code scanning / CodeQL
Potentially unsafe use of strcat Critical
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 11 hours ago
In general, the problem is that strcat appends to a fixed-size buffer without checking if enough space remains. The safe pattern is to track how many bytes have been written and how much capacity remains, and then use a bounded function (strncat, snprintf, g_strlcat, etc.) or manual copying that never writes more than the remaining capacity (leaving room for the null terminator). Excess data should be truncated gracefully.
For this specific code, the best minimal-impact fix is to avoid strcat and instead use snprintf with an offset and remaining-length accounting. We already have <string.h> included, and <stdio.h> is not currently included, so we need to add it to use snprintf. In handle_get_marker_list, we will:
- Introduce a
size_t used = 0;variable to track how many bytes inmarkerListStrare currently used. - For each marker:
- If
i > 0, append a comma usingsnprintf(markerListStr + used, sizeof(markerListStr) - used, "%s", ",");and updateusedby the number of characters actually written (capped if truncated). - Then append the marker the same way with
snprintf, again ensuring we never write beyondsizeof(markerListStr).
- If
- Stop appending once
usedreachessizeof(markerListStr) - 1, since the buffer is full (minus the terminator). - Ensure
markerListStris always null-terminated (whichsnprintfguarantees as long as the buffer size argument is > 0).
Concretely:
- In
source/ccspinterface/dbusInterface.c, add#include <stdio.h>near the other includes. - Replace the body of the
if (markerList && Vector_Size(markerList) > 0)loop wherestrcatis used with a loop usingsnprintfand ausedcounter, keeping the rest of the logic unchanged.
-
Copy modified line R34 -
Copy modified lines R209-R210 -
Copy modified lines R213-R214 -
Copy modified lines R216-R247
| @@ -31,6 +31,7 @@ | ||
| #include <pthread.h> | ||
| #include <unistd.h> | ||
| #include <glib.h> | ||
| #include <stdio.h> | ||
|
|
||
| #include "dbusInterface.h" | ||
| #include "t2collection.h" | ||
| @@ -205,12 +206,45 @@ | ||
| getMarkerListCallBack(component_name, (void**)&markerList); | ||
| char markerListStr[4096] = ""; | ||
| if (markerList && Vector_Size(markerList) > 0) { | ||
| size_t used = 0; | ||
| size_t capacity = sizeof(markerListStr); | ||
| for (size_t i = 0; i < Vector_Size(markerList); i++) { | ||
| char *marker = (char*)Vector_At(markerList, i); | ||
| if (marker) { | ||
| if (i > 0) strcat(markerListStr, ","); | ||
| strcat(markerListStr, marker); | ||
| if (!marker || used >= capacity - 1) { | ||
| continue; | ||
| } | ||
|
|
||
| if (i > 0 && used < capacity - 1) { | ||
| int written = snprintf(markerListStr + used, | ||
| capacity - used, | ||
| "%s", | ||
| ","); | ||
| if (written < 0) { | ||
| break; | ||
| } | ||
| if ((size_t)written >= capacity - used) { | ||
| used = capacity - 1; | ||
| markerListStr[used] = '\0'; | ||
| continue; | ||
| } | ||
| used += (size_t)written; | ||
| } | ||
|
|
||
| if (used < capacity - 1) { | ||
| int written = snprintf(markerListStr + used, | ||
| capacity - used, | ||
| "%s", | ||
| marker); | ||
| if (written < 0) { | ||
| break; | ||
| } | ||
| if ((size_t)written >= capacity - used) { | ||
| used = capacity - 1; | ||
| markerListStr[used] = '\0'; | ||
| continue; | ||
| } | ||
| used += (size_t)written; | ||
| } | ||
| } | ||
| Vector_Destroy(markerList, free); | ||
| } |
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.
Pull request overview
This pull request introduces a D-Bus IPC alternative to RBUS for the Telemetry 2.0 (T2) component integration. The implementation is marked as a proof-of-concept (POC) and adds D-Bus support alongside existing RBUS functionality.
Changes:
- Added new D-Bus interface layer (dbusInterface.c/h) providing similar functionality to the existing RBUS interface
- Modified telemetry_busmessage_sender.c to support both RBUS and D-Bus backends with conditional compilation
- Updated telemetry_client.c with multi-threaded test code for stress testing
- Modified build system to link against D-Bus libraries
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 24 comments.
Show a summary per file
| File | Description |
|---|---|
| source/ccspinterface/dbusInterface.h | New header defining D-Bus interface API for T2 operations |
| source/ccspinterface/dbusInterface.c | New implementation of D-Bus-based IPC operations including event handling and parameter operations |
| source/ccspinterface/busInterface.c | Updated to initialize and register D-Bus event listeners |
| source/ccspinterface/Makefile.am | Added D-Bus source files and library dependencies |
| source/commonlib/telemetry_busmessage_sender.c | Major refactoring to support both RBUS and D-Bus with separate connection handling, event loops, and message bus initialization |
| source/commonlib/telemetry_client.c | Replaced simple test with multi-threaded stress test creating separate threads for each telemetry event |
| source/commonlib/Makefile.am | Updated to link against D-Bus libraries and modified include paths |
| build_inside_container.sh | Added D-Bus include paths to compiler flags |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| if (!dbus_threads_init_default()) { | ||
| T2Error("Failed to initialize D-Bus threading"); | ||
| return 1; |
Copilot
AI
Jan 27, 2026
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.
Inconsistent return type on error. When dbus_threads_init_default() fails, the function returns 1 (integer) instead of T2ERROR_FAILURE (T2ERROR type). This is inconsistent with the function's return type declaration and other error paths in the function.
| return 1; | |
| pthread_mutex_unlock(&dbusMutex); | |
| return T2ERROR_FAILURE; |
| T2Info("GetOperationalStatus called with param_name: %s", param_name); | ||
|
|
||
| uint32_t value = 0; | ||
| /* TODO check oprtational status of specific component param_name will componet name */ |
Copilot
AI
Jan 27, 2026
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.
Spelling error in comment: 'oprtational' should be 'operational', and 'componet' should be 'component'.
| /* TODO check oprtational status of specific component param_name will componet name */ | |
| /* TODO: check operational status of specific component; param_name will be component name */ |
| if (!dbus_connection_register_object_path(t2dbus_handle.connection, T2_DBUS_OBJECT_PATH, | ||
| &vtable, NULL)) { | ||
| T2Error("Failed to register object path"); | ||
| dbus_connection_unref(t2dbus_handle.connection); |
Copilot
AI
Jan 27, 2026
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.
Missing cleanup on error: If dbus_connection_register_object_path fails at line 382, the function unreferences the connection but does not unlock the mutex before returning. This will cause a deadlock. The mutex should be unlocked before returning, and connection cleanup should also include freeing unique_name if it was allocated.
| dbus_connection_unref(t2dbus_handle.connection); | |
| dbus_connection_unref(t2dbus_handle.connection); | |
| t2dbus_handle.connection = NULL; | |
| if (t2dbus_handle.unique_name) { | |
| free(t2dbus_handle.unique_name); | |
| t2dbus_handle.unique_name = NULL; | |
| } | |
| pthread_mutex_unlock(&dbusMutex); |
| libtelemetry_msgsender_la_CPPFLAGS = -fPIC -I/usr/include/dbus-1.0 \ | ||
| -I/usr/lib/x86_64-linux-gnu/dbus-1.0/include \ |
Copilot
AI
Jan 27, 2026
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.
Hardcoded platform-specific paths in the build configuration. The paths '/usr/include/dbus-1.0' and '/usr/lib/x86_64-linux-gnu/dbus-1.0/include' are hardcoded for x86_64 architecture. This will break builds on other architectures (ARM, aarch64, etc.). Should use PKG_CONFIG_SYSROOT_DIR variables like the original code or pkg-config to detect D-Bus paths dynamically.
| libtelemetry_msgsender_la_SOURCES = telemetry_busmessage_sender.c | ||
| libtelemetry_msgsender_la_LDFLAGS = -shared | ||
| libtelemetry_msgsender_la_LIBADD = -lrbus ${top_builddir}/source/utils/libt2utils.la | ||
| libtelemetry_msgsender_la_LIBADD = -ldbus-1 ${top_builddir}/source/utils/libt2utils.la |
Copilot
AI
Jan 27, 2026
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.
Removed RBUS library dependency without conditional compilation support. The original code linked with '-lrbus', but this has been completely removed. Since RBUS support is still conditionally compiled (with RBUS_SUPPORT_ENABLED), this will cause linker errors when RBUS_SUPPORT_ENABLED is defined. The RBUS library dependency should be conditionally included based on build configuration.
| telemetry2_0_client_CPPFLAGS = -fPIC -I/usr/include/dbus-1.0 \ | ||
| -I/usr/lib/x86_64-linux-gnu/dbus-1.0/include \ |
Copilot
AI
Jan 27, 2026
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.
Hardcoded platform-specific paths in the build configuration. The paths '/usr/include/dbus-1.0' and '/usr/lib/x86_64-linux-gnu/dbus-1.0/include' are hardcoded for x86_64 architecture. This will break builds on other architectures (ARM, aarch64, etc.). Should use PKG_CONFIG_SYSROOT_DIR variables like the original code or pkg-config to detect D-Bus paths dynamically.
|
|
||
| EVENT_DEBUG("D-Bus: Event loop thread started (processing both connections)\n"); | ||
|
|
||
| while (dbus_event_thread_running) |
Copilot
AI
Jan 27, 2026
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.
Race condition: The dbus_event_thread_running variable is accessed without mutex protection. Line 1168 reads this variable in the event loop thread, while it can be modified from the main thread (line 249, 1319). This can lead to race conditions and undefined behavior. Access to this shared variable should be protected with a mutex or declared as atomic.
| } | ||
|
|
||
| // Create a thread for each event | ||
| while(i <= n) |
Copilot
AI
Jan 27, 2026
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.
Off-by-one error: Loop condition 'i <= n' creates n+1 threads (0 to n inclusive) but the comment on line 86 says 'n events'. This is inconsistent. Either the loop should be 'i < n' or the printf should say 'n+1 events'. Additionally, this matches the array allocation on line 90 which allocates n+1 elements, but the messaging is confusing.
| } | ||
|
|
||
| i++; | ||
| usleep(100); |
Copilot
AI
Jan 27, 2026
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.
Inconsistent spacing: Missing space after usleep call. This line has improper indentation (one space instead of standard indentation), which is inconsistent with the rest of the code.
| usleep(100); | |
| usleep(100); |
| static int dbus_checkStatus(void) | ||
| { | ||
| // Check if D-Bus is available by attempting to connect | ||
| DBusError error; | ||
| dbus_error_init(&error); | ||
| DBusConnection *test_conn = dbus_bus_get(DBUS_BUS_SYSTEM, &error); | ||
|
|
||
| if (dbus_error_is_set(&error)) | ||
| { | ||
| dbus_error_free(&error); | ||
| return -1; // D-Bus not available | ||
| } | ||
|
|
||
| if (test_conn) | ||
| { | ||
| dbus_connection_unref(test_conn); | ||
| isDbusEnabled = true; |
Copilot
AI
Jan 27, 2026
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.
Side effect in check function: The dbus_checkStatus function modifies global state (sets isDbusEnabled = true on line 294) which is unexpected for a function that appears to only check status. This side effect should be documented or the state modification should be moved to the initialization function where it's more appropriate.
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.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 32 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| telemetry2_0_client_SOURCES = telemetry_client.c telemetry_busmessage_sender.c | ||
| telemetry2_0_client_LDADD = -lrbus ${top_builddir}/source/utils/libt2utils.la -lpthread | ||
| telemetry2_0_client_LDADD = -ldbus-1 ${top_builddir}/source/utils/libt2utils.la -lpthread |
Copilot
AI
Jan 29, 2026
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.
Same RBUS dependency removal issue - this breaks RBUS support. The library linking should include both -lrbus and -ldbus-1 conditionally based on build configuration.
| char path[100]; | ||
| pthread_detach(pthread_self()); | ||
| int ch; | ||
| int count = 0; |
Copilot
AI
Jan 29, 2026
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.
The variable count is declared here and initialized to 0, but there was previously a static variable count at file scope (removed in this change). Verify that moving this to local scope doesn't break any functionality that depended on the persistent count across function calls.
| return T2ERROR_FAILURE; | ||
| } | ||
|
|
||
| //dbus_message_unref(signal); |
Copilot
AI
Jan 29, 2026
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.
The dbus_message_unref(signal) call is commented out at line 310, but the message is created and sent. This will cause a memory leak as the signal message is never freed. Either uncomment this line or document why it's intentionally omitted.
| //dbus_message_unref(signal); | |
| dbus_message_unref(signal); |
| telemetry2_0_client_CPPFLAGS = -fPIC -I/usr/include/dbus-1.0 \ | ||
| -I/usr/lib/x86_64-linux-gnu/dbus-1.0/include \ |
Copilot
AI
Jan 29, 2026
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.
Same issue as with libtelemetry_msgsender_la_CPPFLAGS - hardcoded architecture-specific paths are not portable. Use pkg-config for D-Bus path detection.
|
|
||
| if (!dbus_threads_init_default()) { | ||
| T2Error("Failed to initialize D-Bus threading\n"); | ||
| return 1; |
Copilot
AI
Jan 29, 2026
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.
The function returns T2ERROR_SUCCESS with an incorrect return type. The function is declared to return T2ERROR but line 338 returns 1 (an integer). This should return T2ERROR_FAILURE instead for consistency with the function's error handling pattern.
| return 1; | |
| pthread_mutex_unlock(&dbusMutex); | |
| return T2ERROR_FAILURE; |
| } | ||
|
|
||
| #if 0 | ||
| int main(int argc, char *argv[]) | ||
| { | ||
| int i = 0, n; | ||
| n = (argc < 2) ? 100 : atoi(argv[1]); | ||
| // Initialize Telemetry2.0 | ||
| t2_init("telemetry_client"); | ||
|
|
||
| while(i <= n) | ||
| { | ||
| t2_event_d("T2_INFO_Test", i); | ||
| i++; | ||
| } | ||
| t2_uninit(); | ||
| printf("Sent %d t2_event_d events.\n", n); | ||
| return 0; | ||
| } | ||
| #endif | ||
| #if 0 | ||
| // Thread argument structure | ||
| typedef struct { | ||
| int value; | ||
| } thread_arg_t; | ||
|
|
||
| // Thread function that sends telemetry event | ||
| void* send_event_thread(void* arg) | ||
| { | ||
| thread_arg_t* t_arg = (thread_arg_t*)arg; | ||
| int value = t_arg->value; | ||
| free(t_arg); | ||
|
|
||
| printf("Thread %ld: Sending T2_INFO_Test=%d\n", (long)pthread_self(), value); | ||
| t2_event_d("T2_INFO_Test", value); | ||
| printf("Thread %ld: Event sent successfully\n", (long)pthread_self()); | ||
|
|
||
| return NULL; | ||
| } | ||
|
|
||
| int main(int argc, char *argv[]) | ||
| { | ||
| int i = 0, n; | ||
| n = (argc < 2) ? 100 : atoi(argv[1]); | ||
|
|
||
| // Initialize Telemetry2.0 | ||
| t2_init("telemetry_client"); | ||
|
|
||
| printf("Starting multi-threaded telemetry test with %d events\n", n); | ||
| printf("Each event will be sent from a separate thread\n\n"); | ||
|
|
||
| // Array to store thread IDs | ||
| pthread_t* threads = (pthread_t*)malloc((n + 1) * sizeof(pthread_t)); | ||
| if (!threads) { | ||
| printf("Failed to allocate memory for threads\n"); | ||
| t2_uninit(); | ||
| return 1; | ||
| } | ||
|
|
||
| // Create a thread for each event | ||
| while(i <= n) | ||
| { | ||
| thread_arg_t* arg = (thread_arg_t*)malloc(sizeof(thread_arg_t)); | ||
| if (!arg) { | ||
| printf("Failed to allocate memory for thread argument\n"); | ||
| break; | ||
| } | ||
| arg->value = i; | ||
|
|
||
| if (pthread_create(&threads[i], NULL, send_event_thread, (void*)arg) != 0) { | ||
| printf("Failed to create thread %d\n", i); | ||
| free(arg); | ||
| break; | ||
| } | ||
|
|
||
| i++; | ||
| usleep(100); | ||
| } | ||
| printf("\nAll %d threads created, waiting for completion...\n\n", i); | ||
|
|
||
| // Wait for all threads to complete | ||
| for (int j = 0; j < i; j++) { | ||
| pthread_join(threads[j], NULL); | ||
| } | ||
|
|
||
| free(threads); | ||
|
|
||
| printf("\n===========================================\n"); | ||
| printf("All threads completed!\n"); | ||
| printf("Sent %d t2_event_d events from %d threads.\n", i, i); | ||
| printf("===========================================\n"); | ||
|
|
||
| t2_uninit(); | ||
| return 0; | ||
| } | ||
| #endif No newline at end of file |
Copilot
AI
Jan 29, 2026
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.
Alternative test implementations are disabled with #if 0. For production code, these should be removed. If these are examples or tests that should be preserved, move them to a separate test file or examples directory.
| } | |
| #if 0 | |
| int main(int argc, char *argv[]) | |
| { | |
| int i = 0, n; | |
| n = (argc < 2) ? 100 : atoi(argv[1]); | |
| // Initialize Telemetry2.0 | |
| t2_init("telemetry_client"); | |
| while(i <= n) | |
| { | |
| t2_event_d("T2_INFO_Test", i); | |
| i++; | |
| } | |
| t2_uninit(); | |
| printf("Sent %d t2_event_d events.\n", n); | |
| return 0; | |
| } | |
| #endif | |
| #if 0 | |
| // Thread argument structure | |
| typedef struct { | |
| int value; | |
| } thread_arg_t; | |
| // Thread function that sends telemetry event | |
| void* send_event_thread(void* arg) | |
| { | |
| thread_arg_t* t_arg = (thread_arg_t*)arg; | |
| int value = t_arg->value; | |
| free(t_arg); | |
| printf("Thread %ld: Sending T2_INFO_Test=%d\n", (long)pthread_self(), value); | |
| t2_event_d("T2_INFO_Test", value); | |
| printf("Thread %ld: Event sent successfully\n", (long)pthread_self()); | |
| return NULL; | |
| } | |
| int main(int argc, char *argv[]) | |
| { | |
| int i = 0, n; | |
| n = (argc < 2) ? 100 : atoi(argv[1]); | |
| // Initialize Telemetry2.0 | |
| t2_init("telemetry_client"); | |
| printf("Starting multi-threaded telemetry test with %d events\n", n); | |
| printf("Each event will be sent from a separate thread\n\n"); | |
| // Array to store thread IDs | |
| pthread_t* threads = (pthread_t*)malloc((n + 1) * sizeof(pthread_t)); | |
| if (!threads) { | |
| printf("Failed to allocate memory for threads\n"); | |
| t2_uninit(); | |
| return 1; | |
| } | |
| // Create a thread for each event | |
| while(i <= n) | |
| { | |
| thread_arg_t* arg = (thread_arg_t*)malloc(sizeof(thread_arg_t)); | |
| if (!arg) { | |
| printf("Failed to allocate memory for thread argument\n"); | |
| break; | |
| } | |
| arg->value = i; | |
| if (pthread_create(&threads[i], NULL, send_event_thread, (void*)arg) != 0) { | |
| printf("Failed to create thread %d\n", i); | |
| free(arg); | |
| break; | |
| } | |
| i++; | |
| usleep(100); | |
| } | |
| printf("\nAll %d threads created, waiting for completion...\n\n", i); | |
| // Wait for all threads to complete | |
| for (int j = 0; j < i; j++) { | |
| pthread_join(threads[j], NULL); | |
| } | |
| free(threads); | |
| printf("\n===========================================\n"); | |
| printf("All threads completed!\n"); | |
| printf("Sent %d t2_event_d events from %d threads.\n", i, i); | |
| printf("===========================================\n"); | |
| t2_uninit(); | |
| return 0; | |
| } | |
| #endif | |
| } |
| pthread_mutex_unlock(&dbusMutex); | ||
| return T2ERROR_FAILURE; | ||
| } | ||
| //TODO change to detached thread |
Copilot
AI
Jan 29, 2026
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.
The TODO comment at line 404 says "change to detached thread" but the thread is not detached. If the listener thread should be detached, call pthread_detach(dbusListenerThread) after creation. If it should be joinable (as currently implemented), remove the TODO. The current implementation joins the thread in dBusInterface_Uninit() at line 432, which suggests it should remain joinable.
| //TODO change to detached thread | |
| /* Listener thread is intentionally joinable and will be joined in dBusInterface_Uninit() */ |
| /* TODO check oprtational status of specific component param_name will componet name */ | ||
| value = t2ReadyStatus; | ||
| T2Info("Returning operational status for %s: 0x%08X\n", param_name, value); | ||
|
|
||
| /* Create reply */ | ||
| DBusMessage *reply = dbus_message_new_method_return(message); | ||
| if (!reply) { | ||
| T2Error("Failed to create reply message\n"); | ||
| return DBUS_HANDLER_RESULT_NEED_MEMORY; | ||
| } | ||
|
|
||
| if (!dbus_message_append_args(reply, | ||
| DBUS_TYPE_UINT32, &value, | ||
| DBUS_TYPE_INVALID)) { | ||
| T2Error("Failed to append reply arguments\n"); | ||
| dbus_message_unref(reply); | ||
| return DBUS_HANDLER_RESULT_NEED_MEMORY; | ||
| } | ||
|
|
||
| if (!dbus_connection_send(connection, reply, NULL)) { | ||
| T2Error("Failed to send reply\n"); | ||
| dbus_message_unref(reply); | ||
| return DBUS_HANDLER_RESULT_NEED_MEMORY; | ||
| } | ||
|
|
||
| T2Info("GetOperationalStatus: Reply sent successfully\n"); | ||
| dbus_message_unref(reply); | ||
| dbus_connection_flush(connection); | ||
|
|
||
| return DBUS_HANDLER_RESULT_HANDLED; | ||
| } | ||
|
|
||
| /* Handle SendT2Event Method */ | ||
| static DBusHandlerResult handle_send_t2_event(DBusConnection *connection, DBusMessage *message) { | ||
| T2Info("handle_send_t2_event: Received SendT2Event method call\n"); | ||
|
|
||
| DBusError error; | ||
| dbus_error_init(&error); | ||
|
|
||
| const char* marker_name = NULL; | ||
| const char* data = NULL; | ||
|
|
||
| if (!dbus_message_get_args(message, &error, | ||
| DBUS_TYPE_STRING, &marker_name, | ||
| DBUS_TYPE_STRING, &data, | ||
| DBUS_TYPE_INVALID)) { | ||
| T2Error("Failed to parse SendT2Event arguments: %s\n", error.message); | ||
| dbus_error_free(&error); | ||
| return DBUS_HANDLER_RESULT_NOT_YET_HANDLED; | ||
| } | ||
|
|
||
| if (marker_name && data && eventCallBack) { | ||
| T2Info("Received event: name=%s, value=%s\n", marker_name, data); | ||
| eventCallBack(strdup(marker_name), strdup(data)); | ||
| } | ||
|
|
||
| /* Create empty reply (method returns void) */ | ||
| DBusMessage *reply = dbus_message_new_method_return(message); | ||
| if (!reply) { | ||
| T2Error("Failed to create reply message\n"); | ||
| return DBUS_HANDLER_RESULT_NEED_MEMORY; | ||
| } | ||
|
|
||
| if (!dbus_connection_send(connection, reply, NULL)) { | ||
| T2Error("Failed to send reply\n"); | ||
| dbus_message_unref(reply); | ||
| return DBUS_HANDLER_RESULT_NEED_MEMORY; | ||
| } | ||
|
|
||
| T2Info("SendT2Event: Reply sent successfully\n"); | ||
| dbus_message_unref(reply); | ||
| dbus_connection_flush(connection); | ||
|
|
||
| return DBUS_HANDLER_RESULT_HANDLED; | ||
| } | ||
|
|
||
| /* Handle GetMarkerList Method */ | ||
| static DBusHandlerResult handle_get_marker_list(DBusConnection *connection, DBusMessage *message) { | ||
| T2Info("handle_get_marker_list: Received GetMarkerList method call\n"); | ||
|
|
||
| DBusMessage *reply = NULL; | ||
| DBusError error; | ||
| dbus_error_init(&error); | ||
|
|
||
| const char* component_name = NULL; | ||
| if (!dbus_message_get_args(message, &error, | ||
| DBUS_TYPE_STRING, &component_name, | ||
| DBUS_TYPE_INVALID)) { | ||
| T2Error("Failed to parse GetMarkerList arguments: %s\n", error.message); | ||
| dbus_error_free(&error); | ||
| return DBUS_HANDLER_RESULT_NOT_YET_HANDLED; | ||
| } | ||
|
|
||
| T2Info("GetMarkerList called for component: %s\n", component_name); | ||
|
|
||
| if (!getMarkerListCallBack) { | ||
| T2Error("GetMarkerList callback not registered\n"); | ||
| reply = dbus_message_new_error(message, DBUS_ERROR_FAILED, | ||
| "Marker list callback not initialized"); | ||
| if (reply) { | ||
| dbus_connection_send(connection, reply, NULL); | ||
| dbus_connection_flush(connection); | ||
| dbus_message_unref(reply); | ||
| } | ||
| } | ||
| else | ||
| { | ||
| Vector *markerList = NULL; | ||
| getMarkerListCallBack(component_name, (void**)&markerList); | ||
| char markerListStr[4096] = ""; | ||
| if (markerList && Vector_Size(markerList) > 0) { | ||
| for (size_t i = 0; i < Vector_Size(markerList); i++) { | ||
| char *marker = (char*)Vector_At(markerList, i); | ||
| if (marker) { | ||
| if (i > 0) strcat(markerListStr, ","); | ||
| strcat(markerListStr, marker); | ||
| } | ||
| } | ||
| Vector_Destroy(markerList, free); | ||
| } | ||
|
|
||
| reply = dbus_message_new_method_return(message); | ||
| if (!reply) { | ||
| T2Error("Failed to create reply message\n"); | ||
| return DBUS_HANDLER_RESULT_NEED_MEMORY; | ||
| } | ||
| const char *result = markerListStr; | ||
|
|
||
| if (!dbus_message_append_args(reply, | ||
| DBUS_TYPE_STRING, &result, | ||
| DBUS_TYPE_INVALID)) { | ||
| T2Error("Failed to append reply arguments\n"); | ||
| dbus_message_unref(reply); | ||
| return DBUS_HANDLER_RESULT_NEED_MEMORY; | ||
| } | ||
|
|
||
| T2Info("Returning marker list: %s\n", markerListStr); | ||
|
|
||
| if (!dbus_connection_send(connection, reply, NULL)) { | ||
| T2Error("Failed to send reply\n"); | ||
| dbus_message_unref(reply); | ||
| return DBUS_HANDLER_RESULT_NEED_MEMORY; | ||
| } | ||
|
|
||
| dbus_connection_flush(connection); | ||
| T2Info("GetMarkerList: Reply sent successfully\n"); | ||
| dbus_message_unref(reply); | ||
| } | ||
|
|
||
| return DBUS_HANDLER_RESULT_HANDLED; | ||
| } | ||
|
|
||
| /* Message Handler */ | ||
| static DBusHandlerResult message_handler(DBusConnection *connection, DBusMessage *message, void *user_data) { | ||
| (void)user_data; | ||
|
|
||
| const char* interface = dbus_message_get_interface(message); | ||
| const char* member = dbus_message_get_member(message); | ||
| const char* path = dbus_message_get_path(message); | ||
|
|
||
| T2Info("Received D-Bus message: interface=%s, member=%s, path=%s\n", | ||
| interface ? interface : "NULL", | ||
| member ? member : "NULL", | ||
| path ? path : "NULL"); | ||
|
|
||
| /* Check if message is for our interface */ | ||
| if (interface && strcmp(interface, T2_DBUS_INTERFACE_NAME) == 0) | ||
| { | ||
| if (dbus_message_is_method_call(message, T2_DBUS_INTERFACE_NAME, "GetOperationalStatus")) { | ||
| return handle_get_operational_status(connection, message); | ||
| } | ||
| else if (dbus_message_is_method_call(message, T2_DBUS_INTERFACE_NAME, "SendT2Event")) { | ||
| return handle_send_t2_event(connection, message); | ||
| } | ||
| else if (dbus_message_is_method_call(message, T2_DBUS_INTERFACE_NAME, "GetMarkerList")) { | ||
| return handle_get_marker_list(connection, message); | ||
| } | ||
| } | ||
|
|
||
| return DBUS_HANDLER_RESULT_NOT_YET_HANDLED; | ||
| } | ||
|
|
||
| /** | ||
| * @brief D-Bus listener thread function | ||
| */ | ||
| static void* dbusListenerThreadFunc(void *arg) { | ||
| (void)arg; | ||
|
|
||
| T2Info("%s ++in\n", __FUNCTION__); | ||
|
|
||
| while (!stopListenerThread && t2dbus_handle.connection) { | ||
| dbus_connection_read_write_dispatch(t2dbus_handle.connection, 100); | ||
| usleep(1000); | ||
| } | ||
| T2Info("%s --out\n", __FUNCTION__); | ||
| return NULL; | ||
| } | ||
|
|
||
| T2ERROR publishdbusEventsProfileUpdates(void) | ||
| { | ||
| T2Info("%s ++in\n", __FUNCTION__); | ||
| if (!t2dbus_handle.is_initialized) { | ||
| if (dBusInterface_Init() != T2ERROR_SUCCESS) { | ||
| return T2ERROR_FAILURE; | ||
| } | ||
| } | ||
|
|
||
| DBusMessage *signal = dbus_message_new_signal(T2_DBUS_OBJECT_PATH, | ||
| T2_DBUS_EVENT_INTERFACE_NAME, | ||
| T2_DBUS_SIGNAL_PROFILE_UPDATE); | ||
| if (!signal) { | ||
| T2Error("Failed to create ProfileUpdate signal\n"); | ||
| return T2ERROR_FAILURE; | ||
| } | ||
|
|
||
| /* Send signal - this queues the message */ | ||
| dbus_uint32_t serial = 0; | ||
| if (!dbus_connection_send(t2dbus_handle.connection, signal, &serial)) { | ||
| T2Error("Failed to send ProfileUpdate signal - out of memory\n"); | ||
| dbus_message_unref(signal); | ||
| return T2ERROR_FAILURE; | ||
| } | ||
|
|
||
| //dbus_message_unref(signal); | ||
|
|
||
| /* Flush to ensure signal is sent immediately */ | ||
| //dbus_connection_flush(t2dbus_handle.connection); | ||
|
|
||
| T2Info("ProfileUpdate signal sent successfully (serial=%u)\n", serial); | ||
| return T2ERROR_SUCCESS; | ||
| } | ||
|
|
||
| /** | ||
| * @brief Initialize D-Bus interface | ||
| */ | ||
| T2ERROR dBusInterface_Init() { | ||
| T2Debug("%s ++in\n", __FUNCTION__); | ||
|
|
||
| pthread_mutex_lock(&dbusMutex); | ||
|
|
||
| if (t2dbus_handle.is_initialized) { | ||
| T2Warning("D-Bus interface already initialized\n"); | ||
| pthread_mutex_unlock(&dbusMutex); | ||
| return T2ERROR_SUCCESS; | ||
| } | ||
|
|
||
| DBusError error; | ||
| dbus_error_init(&error); | ||
|
|
||
| if (!dbus_threads_init_default()) { | ||
| T2Error("Failed to initialize D-Bus threading\n"); | ||
| return 1; | ||
| } | ||
|
|
||
| /* Connect to system bus */ | ||
| t2dbus_handle.connection = dbus_bus_get(DBUS_BUS_SYSTEM, &error); | ||
| if (dbus_error_is_set(&error)) { | ||
| T2Error("D-Bus connection error: %s\n", error.message); | ||
| dbus_error_free(&error); | ||
| pthread_mutex_unlock(&dbusMutex); | ||
| return T2ERROR_FAILURE; | ||
| } | ||
|
|
||
| if (!t2dbus_handle.connection) { | ||
| T2Error("Failed to get D-Bus connection\n"); | ||
| pthread_mutex_unlock(&dbusMutex); | ||
| return T2ERROR_FAILURE; | ||
| } | ||
| T2Info("Connected to D-Bus system bus\n"); | ||
| int ret = dbus_bus_request_name(t2dbus_handle.connection, T2_DBUS_SERVICE_NAME, | ||
| DBUS_NAME_FLAG_REPLACE_EXISTING, &error); | ||
| if (dbus_error_is_set(&error)) { | ||
| T2Error("D-Bus name request error: %s\n", error.message); | ||
| dbus_error_free(&error); | ||
| dbus_connection_unref(t2dbus_handle.connection); | ||
| t2dbus_handle.connection = NULL; | ||
| pthread_mutex_unlock(&dbusMutex); | ||
| return T2ERROR_FAILURE; | ||
| } | ||
|
|
||
| if (ret != DBUS_REQUEST_NAME_REPLY_PRIMARY_OWNER) { | ||
| T2Error("Not primary owner of the name (ret=%d)\n", ret); | ||
| dbus_connection_unref(t2dbus_handle.connection); | ||
| t2dbus_handle.connection = NULL; | ||
| pthread_mutex_unlock(&dbusMutex); | ||
| return T2ERROR_FAILURE; | ||
| } | ||
|
|
||
| T2Info("Acquired service name: %s\n", T2_DBUS_SERVICE_NAME); | ||
| /* Store unique name */ | ||
| t2dbus_handle.unique_name = strdup(dbus_bus_get_unique_name(t2dbus_handle.connection)); | ||
|
|
||
| /* Register object path */ | ||
| DBusObjectPathVTable vtable = { | ||
| .message_function = message_handler, | ||
| .unregister_function = NULL | ||
| }; | ||
|
|
||
| if (!dbus_connection_register_object_path(t2dbus_handle.connection, T2_DBUS_OBJECT_PATH, | ||
| &vtable, NULL)) { | ||
| T2Error("Failed to register object path\n"); | ||
| dbus_connection_unref(t2dbus_handle.connection); | ||
| return T2ERROR_FAILURE; | ||
| } | ||
| T2Info("Registered object path: %s\n", T2_DBUS_OBJECT_PATH); | ||
| //TODO check ready status based on component initialization | ||
| t2ReadyStatus = T2_STATE_COMPONENT_READY; |
Copilot
AI
Jan 29, 2026
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.
The TODO at line 87 mentions checking operational status based on component initialization, and the status is hardcoded to T2_STATE_COMPONENT_READY. This bypasses proper readiness checks and could allow operations before the component is actually ready. Implement proper component status tracking before production use.
| ret = registerDbusT2EventListener(eventCB); | ||
|
|
||
| if (isRbus) | ||
| { | ||
| ret = registerRbusT2EventListener(eventCB); | ||
| // ret = registerRbusT2EventListener(eventCB); | ||
| T2Info("RBUS repeaceled with dbus\n"); |
Copilot
AI
Jan 29, 2026
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.
The RBUS event listener registration is commented out and replaced with D-Bus registration. However, the code still checks if (isRbus) before this block, which means when RBUS is active, it won't register anything. The logic should be updated to handle both RBUS and D-Bus modes properly, or the conditional should be removed if RBUS is being completely replaced.
| libtelemetry_msgsender_la_SOURCES = telemetry_busmessage_sender.c | ||
| libtelemetry_msgsender_la_LDFLAGS = -shared | ||
| libtelemetry_msgsender_la_LIBADD = -lrbus ${top_builddir}/source/utils/libt2utils.la | ||
| libtelemetry_msgsender_la_LIBADD = -ldbus-1 ${top_builddir}/source/utils/libt2utils.la |
Copilot
AI
Jan 29, 2026
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.
The RBUS dependency has been removed entirely with -lrbus changed to -ldbus-1. This breaks RBUS support when RBUS_SUPPORT_ENABLED is defined. The library should conditionally link both libraries based on the build configuration, not replace one with the other.
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.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 28 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Small sleep to avoid busy-waiting | ||
| usleep(100000); // 100ms |
Copilot
AI
Jan 29, 2026
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.
The D-Bus event loop thread sleeps for 100ms (100000 microseconds) on line 1129. This is quite a long sleep interval and could introduce latency in event processing. Consider reducing this to a more reasonable value like 10ms, or using a proper event-driven approach with dbus_connection_read_write with a timeout.
| // Small sleep to avoid busy-waiting | |
| usleep(100000); // 100ms | |
| // Small sleep to avoid busy-waiting (10ms) | |
| usleep(10000); // 10ms |
| #if defined(RBUS_SUPPORT_ENABLED) | ||
| #include <rbus/rbus.h> | ||
| #endif |
Copilot
AI
Jan 29, 2026
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.
The RBUS_SUPPORT_ENABLED conditional is added here, but RBUS header is included unconditionally on line 37 outside of this conditional block. If RBUS_SUPPORT_ENABLED is not defined, this will cause a compilation error. The rbus.h include should be moved inside the conditional block.
| if (isRbus) | ||
| { | ||
| ret = registerRbusT2EventListener(eventCB); | ||
| // ret = registerRbusT2EventListener(eventCB); | ||
| T2Info("RBUS repeaceled with dbus\n"); |
Copilot
AI
Jan 29, 2026
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.
The RBUS event listener registration is commented out and replaced with D-Bus registration. This breaks RBUS functionality when RBUS_SUPPORT_ENABLED is defined. The code should conditionally register with RBUS or D-Bus based on which bus is enabled, not completely disable RBUS registration.
| if(T2ERROR_SUCCESS == registerGetMarkerListCallback(getComponentMarkerList)) | ||
| { | ||
| T2Info("Registered get marker list callback for component %s \n", compName); | ||
| i = length; // exit for loop because dbus uses common callback for all components | ||
| } |
Copilot
AI
Jan 29, 2026
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.
The loop terminates early when D-Bus callback registration succeeds (i = length at line 338). This means only the first component gets its marker list callback registered for D-Bus. If this is intentional because D-Bus uses a common callback for all components, it should be documented more clearly. Also verify that all components can properly retrieve their marker lists through this single callback.
| export CFLAGS=" ${DEBUG_CFLAGS} -I/usr/lib/x86_64-linux-gnu/dbus-1.0/include -I/usr/include/dbus-1.0 -I${INSTALL_DIR}/include/rtmessage -I${INSTALL_DIR}/include/msgpack -I${INSTALL_DIR}/include/rbus -I${INSTALL_DIR}/include -I/usr/include/glib-2.0 -I/usr/lib/x86_64-linux-gnu/glib-2.0/include -I/usr/local/include -DFEATURE_SUPPORT_WEBCONFIG -DRDK_LOGGER -DPERSIST_LOG_MON_REF -DDCMAGENT" | ||
|
|
||
| export LDFLAGS="-L/usr/lib/x86_64-linux-gnu -lglib-2.0" | ||
|
|
Copilot
AI
Jan 29, 2026
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.
Hardcoded D-Bus include paths added to CFLAGS. Similar to the Makefile.am issues, these should use pkg-config for portability across different systems and architectures.
| export CFLAGS=" ${DEBUG_CFLAGS} -I/usr/lib/x86_64-linux-gnu/dbus-1.0/include -I/usr/include/dbus-1.0 -I${INSTALL_DIR}/include/rtmessage -I${INSTALL_DIR}/include/msgpack -I${INSTALL_DIR}/include/rbus -I${INSTALL_DIR}/include -I/usr/include/glib-2.0 -I/usr/lib/x86_64-linux-gnu/glib-2.0/include -I/usr/local/include -DFEATURE_SUPPORT_WEBCONFIG -DRDK_LOGGER -DPERSIST_LOG_MON_REF -DDCMAGENT" | |
| export LDFLAGS="-L/usr/lib/x86_64-linux-gnu -lglib-2.0" | |
| DBUS_CFLAGS="$(pkg-config --cflags dbus-1 2>/dev/null || echo '-I/usr/lib/x86_64-linux-gnu/dbus-1.0/include -I/usr/include/dbus-1.0')" | |
| GLIB_CFLAGS="$(pkg-config --cflags glib-2.0 2>/dev/null || echo '-I/usr/include/glib-2.0 -I/usr/lib/x86_64-linux-gnu/glib-2.0/include')" | |
| export CFLAGS=" ${DEBUG_CFLAGS} ${DBUS_CFLAGS} ${GLIB_CFLAGS} -I${INSTALL_DIR}/include/rtmessage -I${INSTALL_DIR}/include/msgpack -I${INSTALL_DIR}/include/rbus -I${INSTALL_DIR}/include -I/usr/local/include -DFEATURE_SUPPORT_WEBCONFIG -DRDK_LOGGER -DPERSIST_LOG_MON_REF -DDCMAGENT" | |
| GLIB_LDFLAGS="$(pkg-config --libs glib-2.0 2>/dev/null || echo '-L/usr/lib/x86_64-linux-gnu -lglib-2.0')" | |
| export LDFLAGS="${GLIB_LDFLAGS}" |
| // TODO : check markers list size and set timeout accordingly | ||
| // Timeout: 500ms | ||
| reply = dbus_connection_send_with_reply_and_block((DBusConnection*)bus_handle, msg, 500, &error); |
Copilot
AI
Jan 29, 2026
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.
TODO suggests the timeout should be dynamic based on marker list size. The current 500ms timeout may be insufficient for large marker lists. Consider implementing dynamic timeout calculation or at least increase the timeout to a more conservative value.
| // TODO : check markers list size and set timeout accordingly | |
| // Timeout: 500ms | |
| reply = dbus_connection_send_with_reply_and_block((DBusConnection*)bus_handle, msg, 500, &error); | |
| // NOTE: Timeout increased to a more conservative value to better handle large marker lists. | |
| // Timeout: 5000ms | |
| reply = dbus_connection_send_with_reply_and_block((DBusConnection*)bus_handle, msg, 5000, &error); |
| telemetry2_0_client_CPPFLAGS = -fPIC -I/usr/include/dbus-1.0 \ | ||
| -I/usr/lib/x86_64-linux-gnu/dbus-1.0/include \ | ||
| -I/usr/include/ccsp \ |
Copilot
AI
Jan 29, 2026
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.
Same issue as with the library: hardcoded D-Bus include paths should use pkg-config for portability. The removal of RBUS include paths also needs to be conditional based on RBUS_SUPPORT_ENABLED.
| else | ||
| { | ||
| // Flush the connection to ensure message is actually sent | ||
| //dbus_connection_flush((DBusConnection*)bus_handle); |
Copilot
AI
Jan 29, 2026
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.
The commented line at 821 should be removed: '//dbus_connection_flush((DBusConnection*)bus_handle);'. If flushing is not needed, the comment should be removed entirely rather than leaving dead code.
| //dbus_connection_flush((DBusConnection*)bus_handle); |
| static T2ERROR doPopulateEventMarkerList( ) | ||
| { | ||
|
|
||
| T2ERROR status = T2ERROR_SUCCESS; | ||
| #if defined(RBUS_SUPPORT_ENABLED) | ||
| char deNameSpace[1][124] = {{ '\0' }}; | ||
| if(!isRbusEnabled) | ||
| { | ||
| return T2ERROR_SUCCESS; | ||
| // Fall through to D-Bus implementation | ||
| } | ||
| else | ||
| { | ||
| EVENT_DEBUG("%s ++in\n", __FUNCTION__); | ||
| rbusError_t ret = RBUS_ERROR_SUCCESS; | ||
| rbusValue_t paramValue_t; | ||
|
|
||
| EVENT_DEBUG("%s ++in\n", __FUNCTION__); | ||
| rbusError_t ret = RBUS_ERROR_SUCCESS; | ||
| rbusValue_t paramValue_t; | ||
| if(!bus_handle && T2ERROR_SUCCESS != initMessageBus()) | ||
| { | ||
| EVENT_ERROR("Unable to get message bus handles \n"); | ||
| EVENT_DEBUG("%s --out\n", __FUNCTION__); | ||
| return T2ERROR_FAILURE; | ||
| } | ||
|
|
||
| if(!bus_handle && T2ERROR_SUCCESS != initMessageBus()) | ||
| { | ||
| EVENT_ERROR("Unable to get message bus handles \n"); | ||
| EVENT_DEBUG("%s --out\n", __FUNCTION__); | ||
| return T2ERROR_FAILURE; | ||
| } | ||
| snprintf(deNameSpace[0], 124, "%s%s%s", T2_ROOT_PARAMETER, componentName, T2_EVENT_LIST_PARAM_SUFFIX); | ||
| EVENT_DEBUG("rbus mode : Query marker list with data element = %s \n", deNameSpace[0]); | ||
|
|
||
| snprintf(deNameSpace[0], 124, "%s%s%s", T2_ROOT_PARAMETER, componentName, T2_EVENT_LIST_PARAM_SUFFIX); | ||
| EVENT_DEBUG("rbus mode : Query marker list with data element = %s \n", deNameSpace[0]); | ||
| pthread_mutex_lock(&markerListMutex); | ||
| EVENT_DEBUG("Lock markerListMutex & Clean up eventMarkerMap \n"); | ||
| if(eventMarkerMap != NULL) | ||
| { | ||
| hash_map_destroy(eventMarkerMap, free); | ||
| eventMarkerMap = NULL; | ||
| } | ||
|
|
||
| pthread_mutex_lock(&markerListMutex); | ||
| EVENT_DEBUG("Lock markerListMutex & Clean up eventMarkerMap \n"); | ||
| if(eventMarkerMap != NULL) | ||
| { | ||
| hash_map_destroy(eventMarkerMap, free); | ||
| eventMarkerMap = NULL; | ||
| } | ||
| ret = rbus_get(bus_handle, deNameSpace[0], ¶mValue_t); | ||
| if(ret != RBUS_ERROR_SUCCESS) | ||
| { | ||
| EVENT_ERROR("rbus mode : No event list configured in profiles %s and return value %d\n", deNameSpace[0], ret); | ||
| pthread_mutex_unlock(&markerListMutex); | ||
| EVENT_DEBUG("rbus mode : No event list configured in profiles %s and return value %d. Unlock markerListMutex\n", deNameSpace[0], ret); | ||
| EVENT_DEBUG("%s --out\n", __FUNCTION__); | ||
| return T2ERROR_SUCCESS; | ||
| } | ||
|
|
||
| ret = rbus_get(bus_handle, deNameSpace[0], ¶mValue_t); | ||
| if(ret != RBUS_ERROR_SUCCESS) | ||
| { | ||
| EVENT_ERROR("rbus mode : No event list configured in profiles %s and return value %d\n", deNameSpace[0], ret); | ||
| pthread_mutex_unlock(&markerListMutex); | ||
| EVENT_DEBUG("rbus mode : No event list configured in profiles %s and return value %d. Unlock markerListMutex\n", deNameSpace[0], ret); | ||
| EVENT_DEBUG("%s --out\n", __FUNCTION__); | ||
| return T2ERROR_SUCCESS; | ||
| } | ||
| rbusValueType_t type_t = rbusValue_GetType(paramValue_t); | ||
| if(type_t != RBUS_OBJECT) | ||
| { | ||
| EVENT_ERROR("rbus mode : Unexpected data object received for %s get query \n", deNameSpace[0]); | ||
| rbusValue_Release(paramValue_t); | ||
| pthread_mutex_unlock(&markerListMutex); | ||
| EVENT_DEBUG("Unlock markerListMutex\n"); | ||
| EVENT_DEBUG("%s --out\n", __FUNCTION__); | ||
| return T2ERROR_FAILURE; | ||
| } | ||
|
|
||
| rbusValueType_t type_t = rbusValue_GetType(paramValue_t); | ||
| if(type_t != RBUS_OBJECT) | ||
| { | ||
| EVENT_ERROR("rbus mode : Unexpected data object received for %s get query \n", deNameSpace[0]); | ||
| rbusValue_Release(paramValue_t); | ||
| pthread_mutex_unlock(&markerListMutex); | ||
| rbusObject_t objectValue = rbusValue_GetObject(paramValue_t); | ||
| if(objectValue) | ||
| { | ||
| eventMarkerMap = hash_map_create(); | ||
| rbusProperty_t rbusPropertyList = rbusObject_GetProperties(objectValue); | ||
| EVENT_DEBUG("\t rbus mode : Update event map for component %s with below events : \n", componentName); | ||
| while(NULL != rbusPropertyList) | ||
| { | ||
| const char* eventname = rbusProperty_GetName(rbusPropertyList); | ||
| if(eventname && strlen(eventname) > 0) | ||
| { | ||
| EVENT_DEBUG("\t %s\n", eventname); | ||
| hash_map_put(eventMarkerMap, (void*) strdup(eventname), (void*) strdup(eventname), free); | ||
| } | ||
| rbusPropertyList = rbusProperty_GetNext(rbusPropertyList); | ||
| } | ||
| } | ||
| else | ||
| { | ||
| EVENT_ERROR("rbus mode : No configured event markers for %s \n", componentName); | ||
| } | ||
| EVENT_DEBUG("Unlock markerListMutex\n"); | ||
| pthread_mutex_unlock(&markerListMutex); | ||
| rbusValue_Release(paramValue_t); | ||
| EVENT_DEBUG("%s --out\n", __FUNCTION__); | ||
| return T2ERROR_FAILURE; | ||
| return status; | ||
| } | ||
| #endif | ||
|
|
||
| rbusObject_t objectValue = rbusValue_GetObject(paramValue_t); | ||
| if(objectValue) | ||
| // D-Bus implementation | ||
| if(isDbusEnabled && bus_handle) | ||
| { | ||
| eventMarkerMap = hash_map_create(); | ||
| rbusProperty_t rbusPropertyList = rbusObject_GetProperties(objectValue); | ||
| EVENT_DEBUG("\t rbus mode : Update event map for component %s with below events : \n", componentName); | ||
| while(NULL != rbusPropertyList) | ||
| EVENT_DEBUG("%s ++in \n", __FUNCTION__); | ||
|
|
||
| if(!bus_handle && T2ERROR_SUCCESS != initMessageBus()) | ||
| { | ||
| EVENT_ERROR("Unable to get message bus handles \n"); | ||
| EVENT_DEBUG("%s --out\n", __FUNCTION__); | ||
| return T2ERROR_FAILURE; | ||
| } | ||
|
|
||
| pthread_mutex_lock(&markerListMutex); | ||
| EVENT_DEBUG("Lock markerListMutex & Clean up eventMarkerMap \n"); | ||
| if(eventMarkerMap != NULL) | ||
| { | ||
| const char* eventname = rbusProperty_GetName(rbusPropertyList); | ||
| if(eventname && strlen(eventname) > 0) | ||
| hash_map_destroy(eventMarkerMap, free); | ||
| eventMarkerMap = NULL; | ||
| } | ||
|
|
||
| // Get marker list via D-Bus method call | ||
| DBusMessage *msg = NULL; | ||
| DBusMessage *reply = NULL; | ||
| DBusError error; | ||
| dbus_error_init(&error); | ||
|
|
||
| msg = dbus_message_new_method_call(T2_DBUS_SERVICE_NAME, | ||
| T2_DBUS_OBJECT_PATH, | ||
| T2_DBUS_INTERFACE_NAME, | ||
| "GetMarkerList"); | ||
| if (!msg) | ||
| { | ||
| EVENT_ERROR("D-Bus mode: Failed to create method call message\n"); | ||
| pthread_mutex_unlock(&markerListMutex); | ||
| return T2ERROR_FAILURE; | ||
| } | ||
|
|
||
| if (!dbus_message_append_args(msg, DBUS_TYPE_STRING, &componentName, DBUS_TYPE_INVALID)) | ||
| { | ||
| EVENT_ERROR("D-Bus mode: Failed to append arguments\n"); | ||
| dbus_message_unref(msg); | ||
| pthread_mutex_unlock(&markerListMutex); | ||
| return T2ERROR_FAILURE; | ||
| } | ||
| // TODO : check markers list size and set timeout accordingly | ||
| // Timeout: 500ms | ||
| reply = dbus_connection_send_with_reply_and_block((DBusConnection*)bus_handle, msg, 500, &error); | ||
| dbus_message_unref(msg); | ||
|
|
||
| if (dbus_error_is_set(&error)) | ||
| { | ||
| EVENT_ERROR("D-Bus mode: Method call failed: %s\n", error.message); | ||
| dbus_error_free(&error); | ||
| pthread_mutex_unlock(&markerListMutex); | ||
| EVENT_DEBUG("D-Bus mode: No event list configured. Unlock markerListMutex\n"); | ||
| return T2ERROR_SUCCESS; | ||
| } | ||
|
|
||
| if (!reply) | ||
| { | ||
| EVENT_ERROR("D-Bus mode: No reply received\n"); | ||
| pthread_mutex_unlock(&markerListMutex); | ||
| return T2ERROR_SUCCESS; | ||
| } | ||
|
|
||
| // Parse reply - expecting a string containing comma-separated marker list | ||
| char *markerListStr = NULL; | ||
| if (dbus_message_get_args(reply, &error, | ||
| DBUS_TYPE_STRING, &markerListStr, | ||
| DBUS_TYPE_INVALID)) | ||
| { | ||
| if(markerListStr && strlen(markerListStr) > 0) | ||
| { | ||
| eventMarkerMap = hash_map_create(); | ||
| EVENT_DEBUG("Update event map for component %s with below events :\n", componentName); | ||
|
|
||
| char* markerListCopy = strdup(markerListStr); | ||
| char* token = strtok(markerListCopy, ","); | ||
| while(token != NULL) | ||
| { | ||
| // Trim whitespace | ||
| while(*token == ' ' || *token == '\t') token++; | ||
| char* end = token + strlen(token) - 1; | ||
| while(end > token && (*end == ' ' || *end == '\t' || *end == '\n')) { | ||
| *end = '\0'; | ||
| end--; | ||
| } | ||
|
|
||
| if(strlen(token) > 0) | ||
| { | ||
| EVENT_DEBUG("\t %s\n", token); | ||
| hash_map_put(eventMarkerMap, (void*)strdup(token), (void*)strdup(token), free); | ||
| } | ||
| token = strtok(NULL, ","); | ||
| } | ||
| free(markerListCopy); | ||
| } | ||
| else | ||
| { | ||
| EVENT_DEBUG("\t %s\n", eventname); | ||
| hash_map_put(eventMarkerMap, (void*) strdup(eventname), (void*) strdup(eventname), free); | ||
| EVENT_ERROR("D-Bus mode: No configured event markers for %s\n", componentName); | ||
| } | ||
| rbusPropertyList = rbusProperty_GetNext(rbusPropertyList); | ||
| } | ||
| else | ||
| { | ||
| EVENT_ERROR("D-Bus mode: Failed to parse reply: %s\n", error.message); | ||
| dbus_error_free(&error); | ||
| } | ||
|
|
||
| dbus_message_unref(reply); | ||
| EVENT_DEBUG("Unlock markerListMutex\n"); | ||
| pthread_mutex_unlock(&markerListMutex); | ||
| EVENT_DEBUG("%s --out\n", __FUNCTION__); | ||
| return T2ERROR_SUCCESS; | ||
| } | ||
| else | ||
| { | ||
| EVENT_ERROR("rbus mode : No configured event markers for %s \n", componentName); | ||
| EVENT_ERROR("No dbus supported message bus available\n"); | ||
| } | ||
| EVENT_DEBUG("Unlock markerListMutex\n"); | ||
| pthread_mutex_unlock(&markerListMutex); | ||
| rbusValue_Release(paramValue_t); | ||
| EVENT_DEBUG("%s --out\n", __FUNCTION__); | ||
| return status; | ||
|
|
||
| return T2ERROR_FAILURE; | ||
| } |
Copilot
AI
Jan 29, 2026
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.
The function doPopulateEventMarkerList has significant logic duplication between the RBUS and D-Bus implementations. Consider refactoring common logic into helper functions to reduce code duplication and improve maintainability.
| lib_LTLIBRARIES = libccspinterface.la | ||
|
|
||
| libccspinterface_la_SOURCES = busInterface.c rbusInterface.c | ||
| libccspinterface_la_SOURCES = busInterface.c rbusInterface.c dbusInterface.c |
Copilot
AI
Jan 29, 2026
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.
By adding dbusInterface.c to libccspinterface_la_SOURCES, this PR enables the D-Bus server implementation, which currently contains a stack-based buffer overflow in handle_get_marker_list where a fixed-size char markerListStr[4096] is built via repeated strcat without bounds checks. A component with many or long event markers (loaded from telemetry configuration) can cause this buffer to overflow when GetMarkerList is invoked over D-Bus, leading to memory corruption and potential remote code execution in the telemetry daemon. Before shipping this linkage, harden dbusInterface.c so that construction of the marker list string is strictly length-bounded (e.g., by tracking remaining capacity or using a safe, bounded string-building API) to prevent any writes past the end of the stack buffer.
| libccspinterface_la_SOURCES = busInterface.c rbusInterface.c dbusInterface.c | |
| libccspinterface_la_SOURCES = busInterface.c rbusInterface.c |
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.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 15 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| else | ||
| { | ||
| // Flush the connection to ensure message is actually sent | ||
| //dbus_connection_flush((DBusConnection*)bus_handle); |
Copilot
AI
Jan 29, 2026
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.
Commented out code that flushes the D-Bus connection. The comment on line 820 says "Flush the connection to ensure message is actually sent", but the flush call is commented out. This could lead to messages being queued but not sent immediately, potentially causing timing issues or message loss. Either uncomment this flush call or remove the misleading comment.
| //dbus_connection_flush((DBusConnection*)bus_handle); | |
| dbus_connection_flush((DBusConnection*)bus_handle); |
| //dbus_message_unref(signal); | ||
|
|
||
| /* Flush to ensure signal is sent immediately */ | ||
| //dbus_connection_flush(t2dbus_handle.connection); |
Copilot
AI
Jan 29, 2026
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.
The dbus_connection_flush() call on line 310 is commented out. According to the comment on line 309, this flush is needed "to ensure signal is sent immediately". Without flushing, the signal may remain queued and not be delivered promptly. This should be uncommented to ensure reliable signal delivery.
| //dbus_connection_flush(t2dbus_handle.connection); | |
| dbus_connection_flush(t2dbus_handle.connection); |
| if (dBusInterface_Init() == T2ERROR_SUCCESS) | ||
| { | ||
| T2Debug("%s --DBUS mode is active \n", __FUNCTION__); //CID 158206:Unchecked return value | ||
| } | ||
| else | ||
| { | ||
| T2Error("%s --DBUS init failed \n", __FUNCTION__); | ||
| } |
Copilot
AI
Jan 29, 2026
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.
Design issue: D-Bus interface is always initialized regardless of whether RBUS is available or enabled. This means both bus systems are running simultaneously, which wastes resources and can cause conflicts. The D-Bus initialization should only occur when RBUS is not available, implementing a proper fallback mechanism.
| int ch; | ||
| int count = 0; |
Copilot
AI
Jan 29, 2026
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.
The variable 'count' has been moved from static file scope to local function scope. However, this changes behavior - the original static variable persisted across function calls. Verify that this change is intentional and doesn't break the counting logic for event caching.
| fs = fopen(T2_CACHE_FILE, "r"); | ||
| if (fs != NULL) | ||
| { | ||
| fgets(path, 100, fs); | ||
| count = atoi ( path ); | ||
| pclose(fs); | ||
| while ((ch = fgetc(fs)) != EOF) | ||
| { | ||
| if (ch == '\n') | ||
| { | ||
| count++; | ||
| } | ||
| } | ||
|
|
||
| //If the file is not empty and does not contain a newline, call it one line | ||
| if (count == 0 && ftell(fs) > 0) | ||
| { | ||
| count++; | ||
| } |
Copilot
AI
Jan 29, 2026
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.
The popen-based line counting has been replaced with manual file reading. While this removes the shell command execution, there's a bug in the logic: ftell() is called after reading the entire file which will always return the file size, not 0. The check 'ftell(fs) > 0' on line 620 will always be true for non-empty files. This should check if the file size is greater than 0 before starting the read loop, or track if any content was read.
|
|
||
| // D-Bus event loop thread | ||
| static pthread_t dbus_event_thread; | ||
| static bool dbus_event_thread_running = false; |
Copilot
AI
Jan 29, 2026
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.
Race condition: The global variable 'dbus_event_thread_running' is accessed from multiple threads (main thread and event loop thread) without synchronization. This can lead to race conditions where one thread reads while another writes. Use atomic operations or protect accesses with a mutex to ensure thread safety.
| else | ||
| { | ||
| T2Debug("Received eventInfo : %s value : %s\n", eventName, (char* ) eventValue); | ||
| T2Info("Received eventInfo : %s value : %s\n", eventName, (char* ) eventValue); |
Copilot
AI
Jan 29, 2026
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.
Typo in log message: "Recieve" should be "Receive".
| char markerListStr[4096] = ""; | ||
| if (markerList && Vector_Size(markerList) > 0) { | ||
| for (size_t i = 0; i < Vector_Size(markerList); i++) { | ||
| char *marker = (char*)Vector_At(markerList, i); | ||
| if (marker) { | ||
| if (i > 0) strcat(markerListStr, ","); | ||
| strcat(markerListStr, marker); | ||
| } |
Copilot
AI
Jan 29, 2026
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.
Buffer overflow vulnerability: The code uses strcat() to concatenate marker names into a fixed-size buffer of 4096 bytes without checking if the total length exceeds the buffer size. If the combined marker list exceeds 4096 bytes, this will cause a buffer overflow. Use snprintf() or dynamically allocate the buffer based on the actual size needed.
| while (dbus_event_thread_running) | ||
| { | ||
| // Process signal connection (for ProfileUpdate signals) | ||
| dbus_connection_read_write_dispatch((DBusConnection*)signal_bus_handle, 0); | ||
|
|
||
| // Process method call connection (flush outgoing SendT2Event messages) | ||
| if (bus_handle) | ||
| { | ||
| dbus_connection_read_write_dispatch((DBusConnection*)bus_handle, 0); | ||
| } |
Copilot
AI
Jan 29, 2026
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.
Race condition: The global variables 'bus_handle' and 'signal_bus_handle' are accessed in the D-Bus event loop thread without synchronization. These handles can be modified by the main thread (e.g., during initialization or cleanup), creating a race condition. Consider protecting these accesses with appropriate locking or using atomic pointers.
| } | ||
|
|
||
| #if 0 | ||
| int main(int argc, char *argv[]) | ||
| { | ||
| int i = 0, n; | ||
| n = (argc < 2) ? 100 : atoi(argv[1]); | ||
| // Initialize Telemetry2.0 | ||
| t2_init("telemetry_client"); | ||
|
|
||
| while(i <= n) | ||
| { | ||
| t2_event_d("T2_INFO_Test", i); | ||
| i++; | ||
| } | ||
| t2_uninit(); | ||
| printf("Sent %d t2_event_d events.\n", n); | ||
| return 0; | ||
| } | ||
| #endif | ||
| #if 0 | ||
| // Thread argument structure | ||
| typedef struct { | ||
| int value; | ||
| } thread_arg_t; | ||
|
|
||
| // Thread function that sends telemetry event | ||
| void* send_event_thread(void* arg) | ||
| { | ||
| thread_arg_t* t_arg = (thread_arg_t*)arg; | ||
| int value = t_arg->value; | ||
| free(t_arg); | ||
|
|
||
| printf("Thread %ld: Sending T2_INFO_Test=%d\n", (long)pthread_self(), value); | ||
| t2_event_d("T2_INFO_Test", value); | ||
| printf("Thread %ld: Event sent successfully\n", (long)pthread_self()); | ||
|
|
||
| return NULL; | ||
| } | ||
|
|
||
| int main(int argc, char *argv[]) | ||
| { | ||
| int i = 0, n; | ||
| n = (argc < 2) ? 100 : atoi(argv[1]); | ||
|
|
||
| // Initialize Telemetry2.0 | ||
| t2_init("telemetry_client"); | ||
|
|
||
| printf("Starting multi-threaded telemetry test with %d events\n", n); | ||
| printf("Each event will be sent from a separate thread\n\n"); | ||
|
|
||
| // Array to store thread IDs | ||
| pthread_t* threads = (pthread_t*)malloc((n + 1) * sizeof(pthread_t)); | ||
| if (!threads) { | ||
| printf("Failed to allocate memory for threads\n"); | ||
| t2_uninit(); | ||
| return 1; | ||
| } | ||
|
|
||
| // Create a thread for each event | ||
| while(i <= n) | ||
| { | ||
| thread_arg_t* arg = (thread_arg_t*)malloc(sizeof(thread_arg_t)); | ||
| if (!arg) { | ||
| printf("Failed to allocate memory for thread argument\n"); | ||
| break; | ||
| } | ||
| arg->value = i; | ||
|
|
||
| if (pthread_create(&threads[i], NULL, send_event_thread, (void*)arg) != 0) { | ||
| printf("Failed to create thread %d\n", i); | ||
| free(arg); | ||
| break; | ||
| } | ||
|
|
||
| i++; | ||
| usleep(100); | ||
| } | ||
| printf("\nAll %d threads created, waiting for completion...\n\n", i); | ||
|
|
||
| // Wait for all threads to complete | ||
| for (int j = 0; j < i; j++) { | ||
| pthread_join(threads[j], NULL); | ||
| } | ||
|
|
||
| free(threads); | ||
|
|
||
| printf("\n===========================================\n"); | ||
| printf("All threads completed!\n"); | ||
| printf("Sent %d t2_event_d events from %d threads.\n", i, i); | ||
| printf("===========================================\n"); | ||
|
|
||
| t2_uninit(); | ||
| return 0; | ||
| } | ||
| #endif No newline at end of file |
Copilot
AI
Jan 29, 2026
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.
Multiple blocks of test code are wrapped in #if 0 directives. Since this PR is described as containing "test code", these blocks should either be removed before merging or moved to a separate test file/directory. Keeping disabled test code in the main source file creates maintenance burden.
| } | |
| #if 0 | |
| int main(int argc, char *argv[]) | |
| { | |
| int i = 0, n; | |
| n = (argc < 2) ? 100 : atoi(argv[1]); | |
| // Initialize Telemetry2.0 | |
| t2_init("telemetry_client"); | |
| while(i <= n) | |
| { | |
| t2_event_d("T2_INFO_Test", i); | |
| i++; | |
| } | |
| t2_uninit(); | |
| printf("Sent %d t2_event_d events.\n", n); | |
| return 0; | |
| } | |
| #endif | |
| #if 0 | |
| // Thread argument structure | |
| typedef struct { | |
| int value; | |
| } thread_arg_t; | |
| // Thread function that sends telemetry event | |
| void* send_event_thread(void* arg) | |
| { | |
| thread_arg_t* t_arg = (thread_arg_t*)arg; | |
| int value = t_arg->value; | |
| free(t_arg); | |
| printf("Thread %ld: Sending T2_INFO_Test=%d\n", (long)pthread_self(), value); | |
| t2_event_d("T2_INFO_Test", value); | |
| printf("Thread %ld: Event sent successfully\n", (long)pthread_self()); | |
| return NULL; | |
| } | |
| int main(int argc, char *argv[]) | |
| { | |
| int i = 0, n; | |
| n = (argc < 2) ? 100 : atoi(argv[1]); | |
| // Initialize Telemetry2.0 | |
| t2_init("telemetry_client"); | |
| printf("Starting multi-threaded telemetry test with %d events\n", n); | |
| printf("Each event will be sent from a separate thread\n\n"); | |
| // Array to store thread IDs | |
| pthread_t* threads = (pthread_t*)malloc((n + 1) * sizeof(pthread_t)); | |
| if (!threads) { | |
| printf("Failed to allocate memory for threads\n"); | |
| t2_uninit(); | |
| return 1; | |
| } | |
| // Create a thread for each event | |
| while(i <= n) | |
| { | |
| thread_arg_t* arg = (thread_arg_t*)malloc(sizeof(thread_arg_t)); | |
| if (!arg) { | |
| printf("Failed to allocate memory for thread argument\n"); | |
| break; | |
| } | |
| arg->value = i; | |
| if (pthread_create(&threads[i], NULL, send_event_thread, (void*)arg) != 0) { | |
| printf("Failed to create thread %d\n", i); | |
| free(arg); | |
| break; | |
| } | |
| i++; | |
| usleep(100); | |
| } | |
| printf("\nAll %d threads created, waiting for completion...\n\n", i); | |
| // Wait for all threads to complete | |
| for (int j = 0; j < i; j++) { | |
| pthread_join(threads[j], NULL); | |
| } | |
| free(threads); | |
| printf("\n===========================================\n"); | |
| printf("All threads completed!\n"); | |
| printf("Sent %d t2_event_d events from %d threads.\n", i, i); | |
| printf("===========================================\n"); | |
| t2_uninit(); | |
| return 0; | |
| } | |
| #endif | |
| } |
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.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 17 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| if (!dbus_threads_init_default()) { | ||
| T2Error("Failed to initialize D-Bus threading\n"); | ||
| return 1; |
Copilot
AI
Jan 30, 2026
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.
Inconsistent error return type: the function returns T2ERROR but this line returns the literal integer 1 instead of T2ERROR_FAILURE. This should be changed to return T2ERROR_FAILURE for consistency with the function's return type and error handling pattern used elsewhere in the function.
| return 1; | |
| return T2ERROR_FAILURE; |
| rbusValue_Release(paramValue_t); | ||
| EVENT_DEBUG("%s --out\n", __FUNCTION__); | ||
| return T2ERROR_FAILURE; | ||
| return status; |
Copilot
AI
Jan 30, 2026
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.
Undefined variable 'status' is being returned. The variable 'status' is not declared or initialized in the RBUS code path. This will cause a compilation error. The function should return T2ERROR_SUCCESS as done in other successful paths.
| return status; | |
| return T2ERROR_SUCCESS; |
| pTempChar = (char *)malloc(messageLen); | ||
| if (!pTempChar) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| memset(pTempChar, '\0', messageLen); |
Copilot
AI
Jan 30, 2026
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.
Missing null pointer check for malloc result. On line 152, pTempChar is allocated with malloc but there's no check before using it with memset on line 158. If malloc fails and returns NULL, the memset call will cause a segmentation fault. While the check is done later on line 153 (which appears to be checking the wrong line), the code should check immediately after allocation and before any use.
| if (pthread_create(&dbus_event_thread, NULL, dbus_event_loop_thread, NULL) == 0) | ||
| { | ||
| pthread_detach(dbus_event_thread); | ||
| } | ||
| else | ||
| { | ||
| EVENT_ERROR("D-Bus: Failed to create event loop thread\n"); | ||
| dbus_event_thread_running = false; | ||
| } |
Copilot
AI
Jan 30, 2026
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.
Potential use-after-free issue. The dbus_event_loop_thread (created as detached on line 1294) might still be running and accessing signal_bus_handle when dBusInterface_Uninit unreferences and nullifies it. There's no synchronization to ensure the event thread has stopped before resources are freed. This is a critical race condition that could lead to crashes or undefined behavior.
| if (pthread_create(&dbus_event_thread, NULL, dbus_event_loop_thread, NULL) == 0) | |
| { | |
| pthread_detach(dbus_event_thread); | |
| } | |
| else | |
| { | |
| EVENT_ERROR("D-Bus: Failed to create event loop thread\n"); | |
| dbus_event_thread_running = false; | |
| } | |
| if (pthread_create(&dbus_event_thread, NULL, dbus_event_loop_thread, NULL) != 0) | |
| { | |
| EVENT_ERROR("D-Bus: Failed to create event loop thread\n"); | |
| dbus_event_thread_running = false; | |
| } | |
| /* Note: thread is left joinable so that teardown code can | |
| * safely synchronize with it (e.g., via pthread_join) | |
| * before freeing shared resources like signal_bus_handle. | |
| */ |
| typedef void (*dbusMethodCallBackPtr)(DBusMessage *reply, int retStatus); | ||
|
|
Copilot
AI
Jan 30, 2026
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.
The dbusMethodCallBackPtr typedef is defined but never used in the codebase. This suggests incomplete implementation or leftover code from planning. Unused type definitions should be removed to keep the interface clean and avoid confusion.
| typedef void (*dbusMethodCallBackPtr)(DBusMessage *reply, int retStatus); | |
| return T2ERROR_FAILURE; | ||
| } | ||
|
|
||
| //dbus_message_unref(signal); |
Copilot
AI
Jan 30, 2026
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.
The memory leak occurs when dbus_connection_send succeeds. After successfully sending the signal message, the signal message object should be unreferenced to free its memory. Currently, line 320 which contains the required dbus_message_unref call is commented out, leading to a memory leak on every ProfileUpdate signal sent.
| //dbus_message_unref(signal); | |
| dbus_message_unref(signal); |
| telemetry2_0_client_CPPFLAGS = -fPIC -I/usr/include/dbus-1.0 \ | ||
| -I/usr/lib/x86_64-linux-gnu/dbus-1.0/include \ |
Copilot
AI
Jan 30, 2026
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.
Hardcoded x86_64 platform-specific paths are used for D-Bus includes. These paths will not work on other architectures. The same issue exists in this target configuration. Use pkg-config to get the correct D-Bus include paths for cross-platform compatibility.
|
|
||
| T2Info("Acquired service name: %s\n", T2_DBUS_SERVICE_NAME); | ||
| /* Store unique name */ | ||
| t2dbus_handle.unique_name = strdup(dbus_bus_get_unique_name(t2dbus_handle.connection)); |
Copilot
AI
Jan 30, 2026
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.
Potential null pointer dereference if t2dbus_handle.unique_name allocation fails. The strdup call can return NULL on memory allocation failure, but there's no check. While this is unlikely to cause issues immediately since the code continues, it could lead to crashes later if unique_name is accessed. Consider adding error handling or at minimum a null check.
| t2dbus_handle.unique_name = strdup(dbus_bus_get_unique_name(t2dbus_handle.connection)); | |
| t2dbus_handle.unique_name = strdup(dbus_bus_get_unique_name(t2dbus_handle.connection)); | |
| if (!t2dbus_handle.unique_name) { | |
| T2Error("Failed to duplicate D-Bus unique name\n"); | |
| dbus_connection_unref(t2dbus_handle.connection); | |
| t2dbus_handle.connection = NULL; | |
| pthread_mutex_unlock(&dbusMutex); | |
| return T2ERROR_FAILURE; | |
| } |
| { | ||
| EVENT_DEBUG("value for %s is : %d\n", T2_OPERATIONAL_STATUS, t2ReadyStatus); | ||
| if((t2ReadyStatus & T2_STATE_COMPONENT_READY) == 0) | ||
| if((t2ReadyStatus & T2_STATE_READY) == 0) |
Copilot
AI
Jan 30, 2026
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.
Inconsistent state check: T2_STATE_COMPONENT_READY is checked on line 1241 but T2_STATE_READY is checked on line 1218. Looking at the enum definition in telemetry2_0.h, T2_STATE_COMPONENT_READY (value 1) and T2_STATE_READY (value 3) are different states. The check on line 1218 should also use T2_STATE_COMPONENT_READY for consistency with the check on line 1241, or there should be a clear reason documented for why different state flags are being checked in these two locations.
| if((t2ReadyStatus & T2_STATE_READY) == 0) | |
| if((t2ReadyStatus & T2_STATE_COMPONENT_READY) == 0) |
| char* token = strtok(markerListCopy, ","); | ||
| while(token != NULL) | ||
| { | ||
| // Trim whitespace | ||
| while(*token == ' ' || *token == '\t') token++; | ||
| char* end = token + strlen(token) - 1; | ||
| while(end > token && (*end == ' ' || *end == '\t' || *end == '\n')) { | ||
| *end = '\0'; | ||
| end--; | ||
| } | ||
|
|
||
| if(strlen(token) > 0) | ||
| { | ||
| EVENT_DEBUG("\t %s\n", token); | ||
| hash_map_put(eventMarkerMap, (void*)strdup(token), (void*)strdup(token), free); | ||
| } | ||
| token = strtok(NULL, ","); | ||
| } | ||
| free(markerListCopy); |
Copilot
AI
Jan 30, 2026
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.
Potential null pointer dereference when checking markerListCopy allocation. The strdup call on line 1051 can fail and return NULL, but there's no check before using it with strtok. This could cause a segmentation fault if memory allocation fails. Add a null check after strdup and handle the error appropriately.
| char* token = strtok(markerListCopy, ","); | |
| while(token != NULL) | |
| { | |
| // Trim whitespace | |
| while(*token == ' ' || *token == '\t') token++; | |
| char* end = token + strlen(token) - 1; | |
| while(end > token && (*end == ' ' || *end == '\t' || *end == '\n')) { | |
| *end = '\0'; | |
| end--; | |
| } | |
| if(strlen(token) > 0) | |
| { | |
| EVENT_DEBUG("\t %s\n", token); | |
| hash_map_put(eventMarkerMap, (void*)strdup(token), (void*)strdup(token), free); | |
| } | |
| token = strtok(NULL, ","); | |
| } | |
| free(markerListCopy); | |
| if(markerListCopy == NULL) | |
| { | |
| EVENT_ERROR("D-Bus mode: Failed to allocate memory for marker list copy\n"); | |
| } | |
| else | |
| { | |
| char* token = strtok(markerListCopy, ","); | |
| while(token != NULL) | |
| { | |
| // Trim whitespace | |
| while(*token == ' ' || *token == '\t') token++; | |
| char* end = token + strlen(token) - 1; | |
| while(end > token && (*end == ' ' || *end == '\t' || *end == '\n')) { | |
| *end = '\0'; | |
| end--; | |
| } | |
| if(strlen(token) > 0) | |
| { | |
| EVENT_DEBUG("\t %s\n", token); | |
| hash_map_put(eventMarkerMap, (void*)strdup(token), (void*)strdup(token), free); | |
| } | |
| token = strtok(NULL, ","); | |
| } | |
| free(markerListCopy); | |
| } |
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.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 14 comments.
Comments suppressed due to low confidence (1)
source/commonlib/telemetry_busmessage_sender.c:1236
- Inconsistent state flag usage: Line 1213 checks T2_STATE_READY while line 1236 (in the RBUS code path) checks T2_STATE_COMPONENT_READY. These should use the same flag for consistent behavior across both D-Bus and RBUS implementations. Also, the D-Bus interface sets t2ReadyStatus to T2_STATE_COMPONENT_READY (dbusInterface.c:403), creating further inconsistency.
if((t2ReadyStatus & T2_STATE_READY) == 0)
{
return true;
}
}
#if defined(RBUS_SUPPORT_ENABLED)
if(!isRbusEnabled)
{
// Fall through to D-Bus handling
}
else
{
isT2Ready = true;
}
#endif
if(!isT2Ready)
{
EVENT_DEBUG("T2 is not ready yet, subscribe to profile update event/signals \n");
if(componentName && (0 != strcmp(componentName, "telemetry_client")))
{
// From other binary applications in rbus mode if t2 daemon is yet to determine state of component specific config from cloud, enable cache
if((t2ReadyStatus & T2_STATE_COMPONENT_READY) == 0)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| if (!dbus_threads_init_default()) { | ||
| T2Error("Failed to initialize D-Bus threading\n"); | ||
| return 1; |
Copilot
AI
Feb 3, 2026
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.
The function returns 1 on error instead of T2ERROR_FAILURE, which is inconsistent with the function's return type of T2ERROR. This should return T2ERROR_FAILURE for consistency.
| return 1; | |
| return T2ERROR_FAILURE; |
| } | ||
|
|
||
| i++; | ||
| usleep(100); |
Copilot
AI
Feb 3, 2026
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.
The spacing before 'usleep' is inconsistent (extra space). This should be corrected for code consistency.
| usleep(100); | |
| usleep(100); |
| else | ||
| { | ||
| // Send method call and wait for reply with timeout (10 ms) | ||
| reply = dbus_connection_send_with_reply_and_block((DBusConnection*)bus_handle, msg, 10`, &error); |
Copilot
AI
Feb 3, 2026
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.
Typo in the timeout value: '10`' should be '10'. The backtick character will cause a compilation error.
| reply = dbus_connection_send_with_reply_and_block((DBusConnection*)bus_handle, msg, 10`, &error); | |
| reply = dbus_connection_send_with_reply_and_block((DBusConnection*)bus_handle, msg, 10, &error); |
| } | ||
| // TODO : check markers list size and set timeout accordingly | ||
| // Timeout: 10ms | ||
| reply = dbus_connection_send_with_reply_and_block((DBusConnection*)bus_handle, msg, 10, &error); |
Copilot
AI
Feb 3, 2026
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.
The timeout of 10ms is extremely short for a D-Bus method call. This is likely to fail in real-world scenarios where the T2 service might be busy or under load. Consider using a reasonable timeout like 1000ms (1 second) or making this configurable.
| //publishEventsProfileUpdates(); | ||
| publishDBUSEventsProfileUpdates(); | ||
|
|
Copilot
AI
Feb 3, 2026
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.
The original publishEventsProfileUpdates() calls are commented out in favor of publishDBUSEventsProfileUpdates(). This breaks RBUS mode functionality. For production, either implement proper mode selection or document why RBUS support is being removed.
| //publishEventsProfileUpdates(); | |
| publishDBUSEventsProfileUpdates(); | |
| publishEventsProfileUpdates(); | |
| } | |
| else | |
| { | |
| publishDBUSEventsProfileUpdates(); |
| // Timeout: 10ms - GetOperationalStatus should respond quickly | ||
| // This prevents hanging if server is down/unresponsive | ||
| reply = dbus_connection_send_with_reply_and_block((DBusConnection*)bus_handle, msg, 10, &error); |
Copilot
AI
Feb 3, 2026
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.
The timeout value of 10ms is very short for a D-Bus method call. This could cause failures under normal system load. Consider using a more appropriate timeout (e.g., 1000ms for 1 second) or at least documenting why such a short timeout is necessary.
| // Timeout: 10ms - GetOperationalStatus should respond quickly | |
| // This prevents hanging if server is down/unresponsive | |
| reply = dbus_connection_send_with_reply_and_block((DBusConnection*)bus_handle, msg, 10, &error); | |
| // Timeout: 1000ms (1 second) - GetOperationalStatus is expected to respond quickly, | |
| // but allow sufficient time under normal system load while still bounding the wait | |
| reply = dbus_connection_send_with_reply_and_block((DBusConnection*)bus_handle, msg, 1000, &error); |
| #if 0 | ||
| #define EVENT_DEBUG(format, ...) do { \ | ||
| struct timespec ts; \ | ||
| struct tm timeinfo; \ | ||
| char timeBuffer[32]; \ | ||
| if (clock_gettime(CLOCK_REALTIME, &ts) == 0) { \ | ||
| localtime_r(&ts.tv_sec, &timeinfo); \ | ||
| long msecs = ts.tv_nsec / 1000000; \ | ||
| strftime(timeBuffer, sizeof(timeBuffer), "%Y-%m-%d %H:%M:%S", &timeinfo); \ | ||
| snprintf(timeBuffer + strlen(timeBuffer), sizeof(timeBuffer) - strlen(timeBuffer), ".%03ld", msecs); \ | ||
| fprintf(stderr, "[%s] T2DEBUG:%s %s:%d: ", timeBuffer, __func__ , __FILE__, __LINE__ ); \ | ||
| } else { \ | ||
| fprintf(stderr, "T2DEBUG:%s %s:%d: ", __func__ , __FILE__, __LINE__ ); \ | ||
| } \ | ||
| fprintf(stderr, (format), ##__VA_ARGS__ ); \ | ||
| } while(0) | ||
| #endif |
Copilot
AI
Feb 3, 2026
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.
This is a large block of disabled code (lines 131-147) that defines an alternative EVENT_DEBUG macro. For a production PR, commented-out code should be removed to improve maintainability, or documented with a clear reason for keeping it.
| #if 0 | |
| #define EVENT_DEBUG(format, ...) do { \ | |
| struct timespec ts; \ | |
| struct tm timeinfo; \ | |
| char timeBuffer[32]; \ | |
| if (clock_gettime(CLOCK_REALTIME, &ts) == 0) { \ | |
| localtime_r(&ts.tv_sec, &timeinfo); \ | |
| long msecs = ts.tv_nsec / 1000000; \ | |
| strftime(timeBuffer, sizeof(timeBuffer), "%Y-%m-%d %H:%M:%S", &timeinfo); \ | |
| snprintf(timeBuffer + strlen(timeBuffer), sizeof(timeBuffer) - strlen(timeBuffer), ".%03ld", msecs); \ | |
| fprintf(stderr, "[%s] T2DEBUG:%s %s:%d: ", timeBuffer, __func__ , __FILE__, __LINE__ ); \ | |
| } else { \ | |
| fprintf(stderr, "T2DEBUG:%s %s:%d: ", __func__ , __FILE__, __LINE__ ); \ | |
| } \ | |
| fprintf(stderr, (format), ##__VA_ARGS__ ); \ | |
| } while(0) | |
| #endif |
|
|
||
| while (!stopListenerThread && t2dbus_handle.connection) { | ||
| dbus_connection_read_write_dispatch(t2dbus_handle.connection, 0); | ||
| usleep(1); // Sleep for 1us to avoid busy-waiting |
Copilot
AI
Feb 3, 2026
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.
The sleep interval of 1 microsecond in a busy-wait loop is extremely inefficient. This will consume significant CPU resources. Consider using a larger sleep interval (e.g., usleep(10000) for 10ms) or switching to an event-driven approach using GLib main loop.
| usleep(1); // Sleep for 1us to avoid busy-waiting | |
| usleep(10000); // Sleep for 10ms to reduce CPU usage while polling |
| { | ||
| T2Error("Failed to register get marker list callback for component %s \n", compName); | ||
| } | ||
| //regDEforCompEventList(compName, getComponentMarkerList); |
Copilot
AI
Feb 3, 2026
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.
The original RBUS registration call is commented out. For production code, this commented-out code should be removed, or the logic should be restructured to properly support both RBUS and D-Bus modes.
| //regDEforCompEventList(compName, getComponentMarkerList); |
| return T2ERROR_FAILURE; | ||
| } | ||
|
|
||
| //dbus_message_unref(signal); |
Copilot
AI
Feb 3, 2026
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.
The signal message is created but never unreferenced (dbus_message_unref is commented out on line 320). This will cause a memory leak. The message should be unreferenced after sending to prevent resource leaks.
| //dbus_message_unref(signal); | |
| dbus_message_unref(signal); |
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.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.