Feature/rdkemw 8587 dlsym - Test

[yuvaramachandran-gurusamy]

No description provided.

[Copilot]

Pull request overview

Copilot reviewed 31 out of 31 changed files in this pull request and generated 16 comments.

Comments suppressed due to low confidence (1)

ds/frontPanelConfig.cpp:108

• The frontPanelConfig.load() method was removed from the getInstance() method but is now expected to be called separately via loadDeviceCapabilities(). However, there's no guard to prevent operations before load() is called. If code tries to use front panel functionality before loadDeviceCapabilities() is invoked, it will fail because _colors, _indicators, and _textDisplays vectors will be empty. Consider adding a check in getter methods or documenting this initialization requirement.

FrontPanelConfig & FrontPanelConfig::getInstance()
{
    static FrontPanelConfig _singleton;
    if (!_singleton.m_isFPInitialized)
    {
        dsError_t errorCode = dsERR_NONE;
        unsigned int retryCount = 1;
        do
        {
            errorCode = dsFPInit();
            if (dsERR_NONE == errorCode)
            {
                _singleton.m_isFPInitialized = true;
                INT_INFO("dsFPInit success\n");
            }
            else{
                INT_ERROR("dsFPInit failed with error[%d]. Retrying... (%d/20)\n", errorCode, retryCount);
                usleep(50000); // Sleep for 50ms before retrying
            }
        }
        while ((!_singleton.m_isFPInitialized) && (retryCount++ < 20));
    }
	return _singleton;

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Typo in variable name 'kVidoeResolutionsSettings' - should be 'kVideoResolutionsSettings' (missing 'd' in 'Video'). This typo appears multiple times in the file and affects code readability.

[Copilot]

Shallow copy of structures with pointer members. The memcpy operation performs a shallow copy of dsVideoPortTypeConfig_t structures. Based on the stub definition at stubs/dsVideoPortSettings.h:93-103, these structures contain pointer members like 'supportedResolutions' (line 101). When copying from dynamic configs loaded via dlsym, the pointers will reference memory in the dynamically loaded library. If the library is unloaded (as it is at line 168 of dsConfigs.c), these pointers become dangling, leading to undefined behavior when accessed later. Consider either keeping the library loaded or performing deep copies of the data.

[Copilot]

Shallow copy of structures with pointer members. The memcpy operation performs a shallow copy of dsAudioTypeConfig_t structures which likely contain pointer members (encodings, compressions, stereoModes arrays based on usage at audioOutputPortConfig.cpp:229-241). When copying from dynamic configs loaded via dlsym, the pointers reference memory in the dynamically loaded library. If the library is unloaded (as it is at line 168 of dsConfigs.c), these pointers become dangling, leading to undefined behavior. Consider either keeping the library loaded or performing deep copies.

[Copilot]

Mixing C and C++ null pointer constants. In C code (line 52), 'nullptr' is used which is a C++ keyword. In C, use 'NULL' instead. This will cause a compilation error in C code.

Suggested change

	if (nullptr == config) {
	if (NULL == config) {

[Copilot]

Duplicate function implementation. loadDeviceCapabilities is implemented in both manager.cpp (lines 101-184) and dsConfigs.c (lines 84-172). These implementations are nearly identical and load the same device configurations. This code duplication violates DRY principle and creates maintenance burden. Consider consolidating into a single shared implementation or clearly documenting why both are needed.

[Copilot]

The logging function signature changed to include a 'func' parameter, but the INT_INFO macro at line 114 and 134 doesn't include the '\n' character which was present in the previous logging. This inconsistency could affect log formatting, though it appears the new logging function adds its own newline at line 73 of dslogger.cpp.

[Copilot]

Potential buffer overflow risk. The log formatting uses kFormatMessageSize (896 bytes) for the user message, but then formats an additional header that could be up to ~128 bytes. If the user message is exactly 896 bytes, the total formatted output at line 66-72 could exceed MAX_LOG_BUFF (1024 bytes), causing buffer overflow. Consider reducing kFormatMessageSize further or using dynamic allocation.

[Copilot]

The function return type is declared as 'int' but the function always returns 0 or -1. However, the caller in dsConfigs.c (line 111) checks the result but doesn't actually use the return value properly - it checks 'isSymbolsLoaded' which is the result of this function, then passes it as a boolean to determine whether to use dynamic or static config. The function should return a boolean type (or the caller should be updated to handle -1 as false).

[Copilot]

The function signature declares return type as 'int', but at line 53 in the header file it's declared as 'void'. This is a function signature mismatch between declaration and definition that will cause compilation errors.

[Copilot]

Duplicate log message at lines 252 and 256. The same information is being logged twice consecutively, which is redundant and clutters the logs.

[Copilot]

Pull request overview

Copilot reviewed 41 out of 41 changed files in this pull request and generated 9 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

initializeFunctionWithRetry is forward-declared as static but later defined without static, which is a linkage mismatch and typically fails to compile. Also this file uses std::function but doesn't include <functional>. Make the declaration/definition consistent (either both static or both non-static) and add the missing header include.

[Copilot]

The port/resolution array dumps added here run unconditionally at INT_INFO level and iterate over every entry. These functions can be called frequently, so this can create significant log spam and runtime overhead in production. Please gate these dumps behind an existing runtime flag (e.g., /opt/dsMgrDumpDeviceConfigs) or only enable them under INT_DEBUG/higher log level.

[Copilot]

_supportedResolutions was moved into the public API surface of VideoOutputPortConfig. This exposes internal mutable state and allows callers to bypass locking (gSupportedResolutionsMutex) which can lead to data races or inconsistent cache state. Prefer keeping this member private and providing a const accessor (or a method that fills a caller-provided list) that preserves the class’ synchronization guarantees.

[Copilot]

Host::getAudioPortHandle() returns NULL even though the return type is intptr_t. This can be misleading and makes it harder to distinguish “no handle” from a valid numeric handle; it also gets passed into dsGetAudioFormat() immediately after. Return a well-defined invalid handle value (e.g., 0 or -1, matching HAL expectations) and consider logging via INT_* macros instead of cout for consistency with the rest of the module.

[Copilot]

This file is compiled as C (.c) but uses C++ nullptr (line 66) and calls access()/F_OK without including the proper header. This will fail to compile on C toolchains (and C99+ treats implicit declarations as errors). Replace nullptr with NULL and add the required includes (e.g., <unistd.h> for access/F_OK; also ensure bool is available via <stdbool.h> or a project header).

[Copilot]

#define DEBUG 1 is introduced but never used in this translation unit. Consider removing it to avoid dead code/macros and accidental interactions with other headers/macros during compilation.

Suggested change

#define DEBUG 1 // Using for dumpconfig

[Copilot]

access()/F_OK is used here but this C file doesn't include the header that declares it (typically <unistd.h>). On C99+ compilers this can be a hard error due to implicit function declaration. Please include the correct header (and ensure bool is defined via <stdbool.h> or a project header since this file uses bool later).

[Copilot]

getPixelResolution() unconditionally sets Resn = &kVidoeResolutionsSettings[0] even when dsGetVideoPortResolutions() returns iCount == 0 or kVidoeResolutionsSettings == NULL, which will dereference invalid memory. Add a guard for iCount > 0 && kVidoeResolutionsSettings != NULL and return a safe default (or an error) when the resolutions list isn't available.

[Copilot]

In _GetDisplayPortType(), the log prints port->defaultResolution with %s without a NULL check. If defaultResolution is NULL for any port, this can crash during logging. Use a ternary fallback (e.g., "NULL") like other call sites.

[Copilot]

Pull request overview

Copilot reviewed 41 out of 41 changed files in this pull request and generated 9 comments.

Comments suppressed due to low confidence (1)

rpc/srv/dsDisplay.c:586

• After malloc(sizeof(dsDisplayEDID_t)), the code checks if (edid == NULL) but the comment says it's handling malloc failure. This should check edidData == NULL (and also handle the case where edidData is NULL before memset/memcpy_s). As written, an allocation failure will lead to a NULL dereference.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

dsVideoPortConfigFree() frees only the top-level arrays, but allocateAndCopyVideoPortConfigs() and allocateAndCopyVideoPortPorts() deep-copy strings (name, defaultResolution). Those heap allocations will leak unless freed per-entry before freeing the arrays.

[Copilot]

INT_TRACE expands to LOG_TRACE(...), but LOG_TRACE is not defined anywhere in this header (only DEBUG/ERROR/INFO/WARNING exist). This leaves a broken macro that will fail to compile if INT_TRACE is ever used. Define LOG_TRACE (mapping to an appropriate RDK_LOG level) or remove INT_TRACE.

[Copilot]

audioDumpconfig() uses nullptr, but this file is C (.c) and will not compile. Use NULL (and consider adding a NULL check for edidData-style allocations consistently).

[Copilot]

allocateAndCopyAudioConfigs() does a shallow memcpy of dsAudioTypeConfig_t. That struct contains pointer fields (e.g., name, encodings, compressions, stereoModes), so after dsConfigs.c dlclose()s the HAL, these pointers can dangle and cause use-after-free when audio config is accessed. Deep-copy the pointed-to data (and free it in dsAudioConfigFree()), or keep the HAL library loaded for the lifetime of the cached config.

[Copilot]

In loadDeviceCapabilities(), audioRet/videoRet/videoPortRet are initialized to dsERR_GENERAL and later the function returns failure if any remain dsERR_GENERAL, even when that capability wasn't requested in capabilityType. This makes the capabilityType parameter misleading and will break partial loads. Initialize the "*_Ret" variables to dsERR_NONE and only validate the ones corresponding to requested capability bits (or gate the final check by capabilityType).

[Copilot]

When logCb is registered, ds_log() now forwards only the raw formatted message (without level/file/line/function context). Since the callback signature is just (priority, const char*), this is a behavior change that drops important context compared to the stderr path. Consider prefixing the message with the same metadata before invoking logCb (or keep the previous behavior).

Suggested change

	logCb(priority, formatted);
	char fullMsg[MAX_LOG_BUFF] = {'\0'};
	snprintf(fullMsg,
	sizeof(fullMsg),
	"[DS][%d] %s [%s:%d] %s: %s \n",
	(int)syscall(SYS_gettid),
	levelMap[static_cast<int>(priority)],
	fileName,
	lineNum,
	func,
	formatted);
	logCb(priority, fullMsg);

[Copilot]

This function returns NULL on error, but its return type is intptr_t and this module already defines NULL_HANDLE as 0. Returning NULL_HANDLE (or a documented sentinel) would be clearer and avoids pointer-typed semantics in integer APIs.

[Copilot]

In the error cleanup path, this code frees videoPortResolutionsSettings[i].name, but dsVideoPortResolution_t::name is a fixed-size array (copied via memcpy), not a heap pointer. Freeing it is invalid and can crash. Remove these per-entry frees and only free the resolutions array itself.

[Copilot]

Same issue as above: the cleanup path frees videoPortResolutionsSettings[i].name, which is not heap-allocated. This is an invalid free.

[Copilot]

Pull request overview

Copilot reviewed 41 out of 41 changed files in this pull request and generated 7 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

This file uses access()/F_OK but does not include <unistd.h>, which can lead to implicit-declaration warnings/errors depending on compiler flags. Add the proper include so the declaration and F_OK are available.

[Copilot]

This file uses access()/F_OK but does not include <unistd.h>, which can lead to implicit-declaration warnings/errors depending on compiler flags. Add the proper include so the declaration and F_OK are available.

[Copilot]

Making _supportedResolutions and _originalSupportedResolutions public exposes mutable internal state to the rest of the codebase, which makes it easy to accidentally corrupt the cache and creates tight coupling. Prefer keeping these members private and providing a const accessor (e.g., returning a const std::vector<VideoResolution>& or a read-only List).

[Copilot]

initializeFunctionWithRetry is forward-declared as static but defined without static later in the file. This changes linkage and typically results in a compile error. Make the declaration/definition consistent (either both static or both non-static).

Suggested change

	static dsError_t initializeFunctionWithRetry(const char* functionName, std::function<dsError_t()> initFunc);
	dsError_t initializeFunctionWithRetry(const char* functionName, std::function<dsError_t()> initFunc);

[Copilot]

This file uses access()/F_OK but does not include <unistd.h>, which can lead to implicit-declaration warnings/errors depending on compiler flags. Add the proper include so the declaration and F_OK are available.

[Copilot]

DS_LOG_LEVEL is defined to the enum identifier ERROR_LEVEL, but #if DS_LOG_LEVEL >= DEBUG_LEVEL is evaluated by the preprocessor. Since enum identifiers aren't macros, they evaluate as 0, which will make this condition behave incorrectly (typically enabling debug logs unconditionally). Define log levels as preprocessor constants (or make DS_LOG_LEVEL a numeric literal) so the #if works as intended.

[Copilot]

allocateAndCopyAudioPorts() does a shallow memcpy of dsAudioPortConfig_t. The port config struct includes pointer members (e.g., connected VOPs in the stub configs), so after dlclose() those pointers can become dangling. Deep-copy any pointer members here as well, and ensure dsAudioConfigFree() frees them.

[Copilot]

Pull request overview

Copilot reviewed 41 out of 41 changed files in this pull request and generated 4 comments.

Comments suppressed due to low confidence (1)

rpc/srv/dsDisplay.c:586

• edidData is allocated with malloc(), but the subsequent NULL check is against edid rather than edidData. If the allocation fails, memset(edidData, ...) will dereference NULL and crash. Please check edidData == NULL (and separately validate edid) before using it.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

dsGetDefaultPortHandle() returns NULL on failure, but the return type is intptr_t (integer). In C this is typically a pointer constant and can trigger -Wint-conversion warnings; also callers likely expect 0/NULL_HANDLE for an invalid handle. Return 0 (or the project’s invalid-handle constant) instead.

[Copilot]

isComponentPortPresent() is not referenced anywhere in the codebase (as noted by the comment) and is currently a non-static symbol in a C translation unit. If it’s meant to be an internal helper, make it static (or remove it) to avoid exporting an unused symbol from the server binary/library.

[Copilot]

connectedVOPs is deep-copied as if it contains a single dsVideoPortPortId_t, but in the existing stub config it is an array indexed by dsVIDEOPORT_TYPE_MAX (i.e., multiple entries). Copying/allocating only one element will truncate the data and can lead to out-of-bounds reads if callers iterate over the expected array length. Allocate/copy the full expected array size (or add an explicit size field to dsAudioPortConfig_t and copy that many elements).

[Copilot]

If _dsGetVideoPortResolutions() fails, iCount remains 0 and pVideoResolutionsSettings stays NULL, so the loop never runs and edid->numOfSupportedResolution is left at 0 even if the EDID contains supported modes. Consider returning early without clearing the EDID list, or falling back to the static resolution table so behavior matches the previous implementation.

[Copilot]

Pull request overview

Copilot reviewed 42 out of 42 changed files in this pull request and generated 5 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

dsConfigs.c uses dlopen/dlsym/dlclose, but libdshalsrv_la doesn't appear to link against libdl. On many toolchains this will fail at link time with unresolved dlopen symbols. Consider adding -ldl via libdshalsrv_la_LIBADD (or an appropriate *_LDFLAGS) so the new config loader links reliably.

[Copilot]

initializeFunctionWithRetry’s docstring mentions returning early based on dsERR_INVALID_STATE / a checkInvalidState option, but the implementation retries unconditionally for any non-dsERR_NONE result and has no checkInvalidState parameter. Please align the comment with the actual behavior (or implement the described behavior if that was the intent).

[Copilot]

The Manager::Initialize() API comment immediately above this function still references AudioOutputPortConfig::getInstance().load() / VideoOutputPortConfig::getInstance().load() / VideoDeviceConfig::getInstance().load() with no parameters, but these load methods now take dynamic config pointers and are invoked via loadDeviceCapabilities(...). Updating that comment would prevent callers from following an outdated initialization sequence.

[Copilot]

This library now calls dlopen/dlsym (see ds/manager.cpp), but the link command here only adds -lIARMBus -ldshalcli. On many systems you must also link -ldl to resolve these symbols. Consider adding -ldl (or ensuring $(DSHAL_LDFLAGS) already includes it) to avoid link failures.

Suggested change

	$(CXX) $(OBJS) $(CFLAGS) -L$(INSTALL)/lib -lIARMBus -ldshalcli -shared -o $(LIBNAMECLI)
	$(CXX) $(OBJS) $(CFLAGS) $(DSHAL_LDFLAGS) -L$(INSTALL)/lib -lIARMBus -ldshalcli -shared -o $(LIBNAMECLI)

[Copilot]

Minor: the new printf uses \n while surrounding output in this file uses \r\n. For consistent console output formatting, consider using \r\n here too.