RDKEMW-8587: consume the config variables using dlsym() in MW. #168
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,6 +1,6 @@ | ||||||||||||||||||||||
| /* | ||||||||||||||||||||||
| * If not stated otherwise in this file or this component's LICENSE file the | ||||||||||||||||||||||
| * following copyright and licenses apply: | ||||||||||||||||||||||
|
Check failure on line 3 in ds/audioOutputPortConfig.cpp
|
||||||||||||||||||||||
| * | ||||||||||||||||||||||
| * Copyright 2016 RDK Management | ||||||||||||||||||||||
| * | ||||||||||||||||||||||
|
|
@@ -34,6 +34,19 @@ | |||||||||||||||||||||
| #include "dsUtl.h" | ||||||||||||||||||||||
| #include "stdlib.h" | ||||||||||||||||||||||
| #include "dslogger.h" | ||||||||||||||||||||||
| #include <dlfcn.h> | ||||||||||||||||||||||
| #include "manager.hpp" | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| #define DEBUG 1 // Using for dumpconfig | ||||||||||||||||||||||
|
||||||||||||||||||||||
| #define DEBUG 1 // Using for dumpconfig | |
| #ifndef DEBUG | |
| #define DEBUG 0 | |
| #endif |
Outdated
Contributor
There was a problem hiding this comment.
Enable copilit as reviewr so fix all issue reported by copilot. Also review whole file and make sure null check added as poineted by Yuva
Author
There was a problem hiding this comment.
I have addressed all the review comments from you, Yuva, and Copilot, except for the new one related to the deumconfig() API. This function is being called only after performing the necessary validation in the caller function, and then the call is made accordingly.
Contributor
There was a problem hiding this comment.
instead of using INT_INFO and creating macro for "#define DEBUG 1 /" use INT_DEBUG and remove macro
Author
There was a problem hiding this comment.
I will do change to INT_DEBUG in final review before merge since debug logs are not coming.
Outdated
Contributor
There was a problem hiding this comment.
i see many comment from yova regardig null check so go thought all the changes and make sure pointer has null check
Contributor
There was a problem hiding this comment.
Please add null check for pKConfigs and pKConfigSize since it is pointer variable
Contributor
There was a problem hiding this comment.
Please add null check for pKPortSize since it is pointer variable
There was a problem hiding this comment.
The variable name searchVaribles is misspelled. It should be searchVariables.
Suggested change
| const char* searchVaribles[] = { | |
| const char* searchVariables[] = { |
Outdated
Contributor
There was a problem hiding this comment.
When u call searchConfig API first it should have input variable and second out variable. it means earchVaribles[2] should be first argumenet not second
Contributor
There was a problem hiding this comment.
Do we need fallbacks to default configs if pKConfigSize, pKPorts and pKPortSize are not present in the library
Author
There was a problem hiding this comment.
We can not mix the new and old configs.
Outdated
There was a problem hiding this comment.
Unnecessary cast to (char *) for parameters that are already const char*. The searchConfigs() function expects const char* for the second parameter, so these casts are redundant.
Suggested change
| ret = searchConfigs((void **)&configuration.pKPortSize, (char *)searchVaribles[3]); | |
| ret = searchConfigs((void **)&configuration.pKPortSize, searchVaribles[3]); |
Comment on lines
+232
to
+235
There was a problem hiding this comment.
Local stack variables configSize and portSize may go out of scope while their addresses are still stored in the configuration struct and used later. When the fallback path is taken (lines 226-231), these local variables' addresses are stored and later dereferenced at lines 247 and 268. This results in undefined behavior.
The variables should be declared as static or the configuration struct should store the values directly instead of pointers to local variables.
Suggested change
| configuration.pKConfigs = kConfigs; | |
| configSize = dsUTL_DIM(kConfigs); | |
| configuration.pKConfigSize = &configSize; | |
| configuration.pKPorts = kPorts; | |
| configuration.pKConfigs = kConfigs; | |
| static int configSize; | |
| configSize = dsUTL_DIM(kConfigs); | |
| configuration.pKConfigSize = &configSize; | |
| configuration.pKPorts = kPorts; | |
| static int portSize; |
Author
There was a problem hiding this comment.
Added the static in the start of funcation and using same here.
Outdated
Contributor
There was a problem hiding this comment.
Please add NULL check here as well.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,6 @@ | ||
| /* | ||
| * If not stated otherwise in this file or this component's LICENSE file the | ||
| * following copyright and licenses apply: | ||
|
Check failure on line 3 in ds/frontPanelConfig.cpp
|
||
| * | ||
| * Copyright 2016 RDK Management | ||
| * | ||
|
|
@@ -41,9 +41,18 @@ | |
| #include "frontPanelSettings.hpp" | ||
| #include "illegalArgumentException.hpp" | ||
| #include "dslogger.h" | ||
| #include "manager.hpp" | ||
|
|
||
| using namespace std; | ||
|
|
||
| typedef struct fpdConfigs | ||
| { | ||
| const dsFPDColorConfig_t *pKFPDIndicatorColors; | ||
| const dsFPDIndicatorConfig_t *pKIndicators; | ||
| int *pKFPDIndicatorColors_size; | ||
| int *pKIndicators_size; | ||
| }fpdConfigs_t; | ||
|
|
||
| namespace device { | ||
|
|
||
| /** | ||
|
|
@@ -337,6 +346,27 @@ | |
| return rTexts; | ||
| } | ||
|
|
||
| void dumpconfig(fpdConfigs_t *configuration) | ||
| { | ||
| // Dump the configuration details | ||
| printf("Dumping Front Panel Configuration Details:\n"); | ||
| printf("Indicator Colors:\n"); | ||
| for (size_t i = 0; i < *(configuration->pKFPDIndicatorColors_size); i++) { | ||
| printf(" Color ID: %d, color: %d\n", | ||
| configuration->pKFPDIndicatorColors[i].id, | ||
| configuration->pKFPDIndicatorColors[i].color); | ||
| } | ||
|
|
||
| printf("Indicators:\n"); | ||
| for (size_t i = 0; i < *(configuration->pKIndicators_size); i++) { | ||
| printf(" Indicator ID: %d, Max Brightness: %d, Max Cycle Rate: %d, Levels: %d, Color Mode: %d\n", | ||
| configuration->pKIndicators[i].id, | ||
| configuration->pKIndicators[i].maxBrightness, | ||
| configuration->pKIndicators[i].maxCycleRate, | ||
| configuration->pKIndicators[i].levels, | ||
| configuration->pKIndicators[i].colorMode); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * @fn FrontPanelConfig::load() | ||
|
|
@@ -352,18 +382,71 @@ | |
| * 1. Create Supported Colors. | ||
| * 2. Create Indicators. | ||
| */ | ||
| int indicatorSize, indicatorColorSize, invalid_size = -1; | ||
| fpdConfigs_t configuration = {0}; | ||
|
|
||
| const char* searchVaribles[] = { | ||
|
There was a problem hiding this comment. The variable name |
||
| "kFPDIndicatorColors", | ||
| "kFPDIndicatorColors_size", | ||
| "kIndicators", | ||
| "kIndicators_size" | ||
| }; | ||
| bool ret = false; | ||
|
|
||
| INT_INFO("%d:%s: Calling searchConfigs( %s)\n", __LINE__, __func__, searchVaribles[0]); | ||
| ret = searchConfigs((void **)&configuration.pKFPDIndicatorColors, searchVaribles[0]); | ||
| if(ret == true) | ||
| { | ||
| INT_INFO("%d:%s: Calling searchConfigs( %s)\n", __LINE__, __func__, searchVaribles[1]); | ||
| ret = searchConfigs((void **)&configuration.pKFPDIndicatorColors_size, (char *)searchVaribles[1]); | ||
|
||
| if(ret == false) | ||
| { | ||
| INT_ERROR("%s is not defined\n", searchVaribles[1]); | ||
| configuration.pKFPDIndicatorColors_size = &invalid_size; | ||
| } | ||
| INT_INFO("%d:%s: Calling searchConfigs( %s)\n", __LINE__, __func__, searchVaribles[2]); | ||
| ret = searchConfigs((void **)&configuration.pKIndicators, searchVaribles[2]); | ||
| if(ret == false) | ||
| { | ||
| INT_ERROR("%s is not defined\n", searchVaribles[2]); | ||
| } | ||
| INT_INFO("%d:%s: Calling searchConfigs( %s)\n", __LINE__, __func__, searchVaribles[3]); | ||
| ret = searchConfigs((void **)&configuration.pKIndicators_size, (char *)searchVaribles[3]); | ||
| if(ret == false) | ||
| { | ||
| INT_ERROR("%s is not defined\n", searchVaribles[3]); | ||
| configuration.pKIndicators_size = &invalid_size; | ||
| } | ||
| } | ||
| else | ||
| { | ||
| INT_ERROR("Read Old Configs\n"); | ||
| configuration.pKFPDIndicatorColors = kIndicatorColors; | ||
| indicatorColorSize = dsUTL_DIM(kIndicatorColors); | ||
| configuration.pKFPDIndicatorColors_size = &indicatorColorSize; | ||
| configuration.pKIndicators = kIndicators; | ||
| indicatorSize = dsUTL_DIM(kIndicators); | ||
| configuration.pKIndicators_size = &indicatorSize; | ||
|
|
||
| INT_INFO("configuration.pKFPDIndicatorColors =%p, *(configuration.pKFPDIndicatorColors_size) = %d\n", configuration.pKFPDIndicatorColors, *(configuration.pKFPDIndicatorColors_size)); | ||
| INT_INFO("configuration.pKIndicators =%p, *(configuration.pKIndicators_size) = %d\n", configuration.pKIndicators, *(configuration.pKIndicators_size)); | ||
| } | ||
| #if DEBUG | ||
| //dumpconfig(&configuration); | ||
| #endif | ||
|
|
||
| { | ||
| for (size_t i = 0; i < *(configuration.pKFPDIndicatorColors_size); i++) { | ||
|
||
| _colors.push_back(FrontPanelIndicator::Color(configuration.pKFPDIndicatorColors[i].id)); | ||
| } | ||
|
|
||
| for (size_t i = 0; i < *(configuration.pKIndicators_size); i++) { | ||
| /* All indicators support a same set of colors */ | ||
| _indicators.push_back(FrontPanelIndicator(configuration.pKIndicators[i].id, | ||
| configuration.pKIndicators[i].maxBrightness, | ||
| configuration.pKIndicators[i].maxCycleRate, | ||
| configuration.pKIndicators[i].levels, | ||
| configuration.pKIndicators[i].colorMode)); | ||
| } | ||
|
|
||
| } | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -43,6 +43,12 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| #include "exception.hpp" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| #include <pthread.h> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| #include <unistd.h> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| #include <dlfcn.h> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| #include "dsHALConfig.h" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| static pthread_mutex_t dsLock = PTHREAD_MUTEX_INITIALIZER; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * @file manager.cpp | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -154,6 +160,36 @@ void Manager::load() | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| printf("%d:%s load completed\n", __LINE__, __FUNCTION__); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| bool searchConfigs(void **pConfigVar, const char *searchConfigStr) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| INT_INFO("%d:%s: Entering function\n", __LINE__, __func__); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| INT_INFO("%d:%s: searchConfigStr = %s\n", __LINE__, __func__, searchConfigStr); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| static int invalidsize = -1; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| static int invalidsize = -1; |
Comment on lines
296
to
334
There was a problem hiding this comment.
The dlopen() handle is not stored and dlclose() is called immediately after dlsym(). This means the loaded library is unloaded while the returned pointers are still being used throughout the application lifecycle. When dlclose() is called, the memory pointed to by the symbols may become invalid, leading to potential crashes or undefined behavior.
Consider either:
- Keeping the library handle open for the lifetime of the application (store it globally and never close it)
- Using
RTLD_NODELETEflag withdlopen()to prevent unloading
Suggested change
| pthread_mutex_lock(&dsLock); | |
| void *dllib = dlopen(RDK_DSHAL_NAME, RTLD_LAZY); | |
| if (dllib) { | |
| *pConfigVar = (void *) dlsym(dllib, searchConfigStr); | |
| if (*pConfigVar != NULL) { | |
| INT_INFO("%s is defined and loaded pConfigVar= %p\r\n", searchConfigStr, *pConfigVar); | |
| } | |
| else { | |
| INT_ERROR("%d:%s: %s is not defined\n", __LINE__, __func__, searchConfigStr); | |
| } | |
| dlclose(dllib); | |
| } | |
| else { | |
| INT_ERROR("%d:%s: Open %s failed\n", __LINE__, __func__, RDK_DSHAL_NAME); | |
| } | |
| static void *dllib = nullptr; | |
| pthread_mutex_lock(&dsLock); | |
| if (!dllib) { | |
| dllib = dlopen(RDK_DSHAL_NAME, RTLD_LAZY); | |
| if (!dllib) { | |
| INT_ERROR("%d:%s: Open %s failed\n", __LINE__, __func__, RDK_DSHAL_NAME); | |
| pthread_mutex_unlock(&dsLock); | |
| INT_INFO("%d:%s: Exit function\n", __LINE__, __func__); | |
| return false; | |
| } | |
| } | |
| *pConfigVar = (void *) dlsym(dllib, searchConfigStr); | |
| if (*pConfigVar != NULL) { | |
| INT_INFO("%s is defined and loaded pConfigVar= %p\r\n", searchConfigStr, *pConfigVar); | |
| } | |
| else { | |
| INT_ERROR("%d:%s: %s is not defined\n", __LINE__, __func__, searchConfigStr); | |
| } | |
| // Do NOT dlclose(dllib); keep the handle open for the lifetime of the process |
Comment on lines
296
to
334
There was a problem hiding this comment.
Calling dlopen() multiple times for every configuration variable lookup is inefficient. The searchConfigs() function is called 6 times in videoOutputPortConfig.cpp, 6 times in frontPanelConfig.cpp, 4 times in audioOutputPortConfig.cpp, and 2 times in videoDeviceConfig.cpp. Each call reopens the same library.
Consider opening the library once (e.g., during initialization) and reusing the handle for all dlsym() calls.
Suggested change
| pthread_mutex_lock(&dsLock); | |
| void *dllib = dlopen(RDK_DSHAL_NAME, RTLD_LAZY); | |
| if (dllib) { | |
| *pConfigVar = (void *) dlsym(dllib, searchConfigStr); | |
| if (*pConfigVar != NULL) { | |
| INT_INFO("%s is defined and loaded pConfigVar= %p\r\n", searchConfigStr, *pConfigVar); | |
| } | |
| else { | |
| INT_ERROR("%d:%s: %s is not defined\n", __LINE__, __func__, searchConfigStr); | |
| } | |
| dlclose(dllib); | |
| } | |
| else { | |
| INT_ERROR("%d:%s: Open %s failed\n", __LINE__, __func__, RDK_DSHAL_NAME); | |
| } | |
| static void *dllib = nullptr; | |
| pthread_mutex_lock(&dsLock); | |
| if (!dllib) { | |
| dllib = dlopen(RDK_DSHAL_NAME, RTLD_LAZY); | |
| if (!dllib) { | |
| INT_ERROR("%d:%s: Open %s failed\n", __LINE__, __func__, RDK_DSHAL_NAME); | |
| pthread_mutex_unlock(&dsLock); | |
| INT_INFO("%d:%s: Exit function\n", __LINE__, __func__); | |
| return false; | |
| } | |
| } | |
| *pConfigVar = (void *) dlsym(dllib, searchConfigStr); | |
| if (*pConfigVar != NULL) { | |
| INT_INFO("%s is defined and loaded pConfigVar= %p\r\n", searchConfigStr, *pConfigVar); | |
| } | |
| else { | |
| INT_ERROR("%d:%s: %s is not defined\n", __LINE__, __func__, searchConfigStr); | |
| } |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
instead of this macro replace print in all dump function from INT_INFO INT_DEBUG