Conversation
* RDKEMW-4848: pass the proper handle to dsEnableHDCP
There was a problem hiding this comment.
Pull request overview
This PR refactors the dynamic library loading mechanism for device settings configuration to use a shared library handle instead of opening/closing the library multiple times. The changes introduce a centralized getDLInstance() and releaseDLInstance() pattern, and update all config loading methods to accept a handle parameter.
Key Changes
- Centralized dynamic library handle management with thread-safe singleton pattern
- Updated all config load methods to accept a
void* pDLHandleparameter - Added support for FrontPanelConfig in the initialization flow
- Removed the
parse_opt_flag()function and associated delay logic
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| ds/manager.cpp | Added getDLInstance/releaseDLInstance functions, refactored searchConfigs to use provided handle, updated Initialize/load methods to pass handle to configs, removed parse_opt_flag function and delay logic |
| ds/include/manager.hpp | Updated searchConfigs declaration to include pDLHandle parameter, removed parse_opt_flag declaration |
| ds/videoOutputPortConfig.hpp | Updated load method signature to accept pDLHandle parameter |
| ds/videoOutputPortConfig.cpp | Refactored load to accept handle parameter, moved symbol search variables into conditional block, added dynamic vs old config loading logic |
| ds/videoDeviceConfig.hpp | Updated load method signature to accept pDLHandle parameter |
| ds/videoDeviceConfig.cpp | Refactored load to accept handle parameter, added dynamic vs old config loading logic |
| ds/audioOutputPortConfig.hpp | Updated load method signature to accept pDLHandle parameter |
| ds/audioOutputPortConfig.cpp | Refactored load to accept handle parameter, added dynamic vs old config loading logic |
| ds/include/frontPanelConfig.hpp | Moved load method from private to public, added m_isFPConfigLoaded member, updated load signature to accept pDLHandle |
| ds/frontPanelConfig.cpp | Removed load call from getInstance, added load validation checks with m_isFPConfigLoaded, refactored load to accept handle parameter and support dynamic/old config loading |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ds/frontPanelConfig.cpp
Outdated
| configuration.pKIndicators[i].colorMode)); | ||
| } | ||
| } | ||
| if (pDLHandle) |
There was a problem hiding this comment.
In the FrontPanelConfig::load method, the check for pDLHandle uses a different null comparison pattern (pDLHandle without nullptr comparison) compared to other config files which consistently use "nullptr != pDLHandle". Consider using "nullptr != pDLHandle" for consistency with the rest of the codebase.
| if (pDLHandle) | |
| if (nullptr != pDLHandle) |
| } | ||
| m_isFPConfigLoaded = true; | ||
| INT_INFO("Exit function\n"); | ||
| return; |
There was a problem hiding this comment.
The explicit return statement at the end of a void function is unnecessary. Since this function returns void, the return statement at line 554 can be removed as the function will automatically return at its end.
| return; |
ds/videoDeviceConfig.cpp
Outdated
|
|
||
| if ( false == isDynamicConfigLoad) | ||
| { | ||
| INT_INFO("Read Old Configs\n"); |
There was a problem hiding this comment.
The log message format for old config loading is inconsistent. In videoDeviceConfig.cpp line 164 it says "Read Old Configs" while other files (videoOutputPortConfig.cpp line 435, audioOutputPortConfig.cpp line 235, frontPanelConfig.cpp line 482) consistently use "Using OLD config loading". Consider updating this message to match the consistent pattern used elsewhere.
| INT_INFO("Read Old Configs\n"); | |
| INT_INFO("Using OLD config loading\n"); |
ds/manager.cpp
Outdated
| void* getDLInstance() | ||
| { | ||
| std::lock_guard<std::mutex> lock(gDLMutex); | ||
| dlerror(); // clear old error | ||
| if (nullptr == gDLHandle){ | ||
| gDLHandle = dlopen(RDK_DSHAL_NAME, RTLD_LAZY); | ||
| } | ||
| INT_INFO("%d:%s: DL Instance '%s'\n", __LINE__, __func__, (nullptr == gDLHandle ? "NULL" : "Valid")); | ||
| return gDLHandle; | ||
| } |
There was a problem hiding this comment.
The getDLInstance function does not log or handle the error when dlopen fails. The old implementation would log the error message from dlerror(), but this new implementation only logs whether the handle is NULL or Valid. Consider logging the actual error message from dlerror() when dlopen fails to aid in debugging issues with loading the dynamic library.
ds/manager.cpp
Outdated
| void* pDLHandle = getDLInstance(); | ||
| device::AudioOutputPortConfig::getInstance().load(pDLHandle); | ||
| device::VideoOutputPortConfig::getInstance().load(pDLHandle); | ||
| device::VideoDeviceConfig::getInstance().load(pDLHandle); |
There was a problem hiding this comment.
The Manager::load() method calls load for AudioOutputPortConfig, VideoOutputPortConfig, and VideoDeviceConfig, but does not call FrontPanelConfig::getInstance().load(pDLHandle). This is inconsistent with Manager::Initialize() which calls load on all four config classes including FrontPanelConfig. This inconsistency could lead to FrontPanelConfig not being properly loaded when Manager::load() is called.
| device::VideoDeviceConfig::getInstance().load(pDLHandle); | |
| device::VideoDeviceConfig::getInstance().load(pDLHandle); | |
| device::FrontPanelConfig::getInstance().load(pDLHandle); |
Reason for change: consume the config variables using dlsym() in MW. Test Procedure: refer RDKEMW-8587 Risks: High Signed-off-by:gsanto722 <grandhi_santoshkumar@comcast.com>
Signed-off-by: grandhi_santoshkumar <grandhi_santoshkumar@comcast.com>
Reason for change: consume the config variables using dlsym() in MW. Test Procedure: refer RDKEMW-8587 Risks: High Signed-off-by:gsanto722 <grandhi_santoshkumar@comcast.com>
Reason for change: consume the config variables using dlsym() in MW. Test Procedure: refer RDKEMW-8587 Risks: High Signed-off-by:gsanto722 <grandhi_santoshkumar@comcast.com>
Reason for change: consume the config variables using dlsym() in MW. Test Procedure: refer RDKEMW-8587 Risks: High Signed-off-by:gsanto722 <grandhi_santoshkumar@comcast.com>
Reason for change: consume the config variables using dlsym() in MW. Test Procedure: refer RDKEMW-8587 Risks: High Signed-off-by:gsanto722 <grandhi_santoshkumar@comcast.com>
Reason for change: consume the config variables using dlsym() in MW. Test Procedure: refer RDKEMW-8587 Risks: High Signed-off-by: grandhi_santoshkumar <grandhi_santoshkumar@comcast.com>
Reason for change: consume the config variables using dlsym() in MW. Test Procedure: refer RDKEMW-8587 Risks: High Signed-off-by:gsanto722 <grandhi_santoshkumar@comcast.com>
Reason for change: consume the config variables using dlsym() in MW. Test Procedure: refer RDKEMW-8587 Risks: High Signed-off-by:gsanto722 <grandhi_santoshkumar@comcast.com>
Reason for change: consume the config variables using dlsym() in MW. Test Procedure: refer RDKEMW-8587 Risks: High Signed-off-by:gsanto722 <grandhi_santoshkumar@comcast.com>
Reason for change: consume the config variables using dlsym() in MW. Test Procedure: refer RDKEMW-8587 Risks: High Signed-off-by:gsanto722 <grandhi_santoshkumar@comcast.com>
Reason for change: consume the config variables using dlsym() in MW. Test Procedure: refer RDKEMW-8587 Risks: High Signed-off-by:gsanto722 <grandhi_santoshkumar@comcast.com>
Signed-off-by: grandhi_santoshkumar <grandhi_santoshkumar@comcast.com>
Signed-off-by: grandhi_santoshkumar <grandhi_santoshkumar@comcast.com>
Signed-off-by: grandhi_santoshkumar <grandhi_santoshkumar@comcast.com>
Signed-off-by: yuvaramachandran_gurusamy <yuvaramachandran_gurusamy@comcast.com>
Signed-off-by: yuvaramachandran_gurusamy <yuvaramachandran_gurusamy@comcast.com>
Signed-off-by: yuvaramachandran_gurusamy <yuvaramachandran_gurusamy@comcast.com>
Signed-off-by: yuvaramachandran_gurusamy <yuvaramachandran_gurusamy@comcast.com>
…logging format Signed-off-by: yuvaramachandran_gurusamy <yuvaramachandran_gurusamy@comcast.com>
Signed-off-by: yuvaramachandran_gurusamy <yuvaramachandran_gurusamy@comcast.com>
Reason for change: Address review comments. Test Procedure: refer RDKEMW-8587 Risks: High Signed-off-by:gsanto722 <grandhi_santoshkumar@comcast.com>
Reason for change: Addressed review comments. Test Procedure: refer RDKEMW-8587 Risks: High Signed-off-by:gsanto722 <grandhi_santoshkumar@comcast.com>
ef2294f to
b8cdf99
Compare
No description provided.