RDKB-63166 : Update device.properties file during runtime #178
Conversation
Reason for change : Modify device.properties file based on partner ID Test Procedure : Check that /etc/device.properties contains "IS_BCI=yes" Risks : Low Priority: Medium Signed-off-by: abhishek_kumaracee2@comcast.com
There was a problem hiding this comment.
Pull request overview
This PR modifies the device.properties file at runtime to add the "IS_BCI=yes" property for Comcast Business partner deployments. The change is triggered when the partner ID matches "comcast-business".
Changes:
- Added a new function
addPartnerDefaultsToDevPropFile()to append partner-specific properties to /etc/device.properties - Integrated the function call into the main workflow after sysevent processing completes
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| static int addPartnerDefaultsToDevPropFile() | ||
| { | ||
| FILE *fp = NULL; | ||
| const char *is_bci = "IS_BCI=yes\n"; | ||
| //open to check for existence | ||
| fp = fopen(DEV_PROP_FILE, "r"); | ||
| if (fp == NULL) | ||
| { | ||
| APPLY_PRINT("[Utopia - %s]File %s does not exist or cannot be accessed",__FUNCTION__,DEV_PROP_FILE); | ||
| return 1; | ||
| } | ||
| fclose(fp); // we don't need it anymore | ||
|
|
||
| //open for writing | ||
| fp = fopen(DEV_PROP_FILE, "a"); | ||
| if (fp == NULL) | ||
| { | ||
| APPLY_PRINT("[Utopia - %s]File %s couldnot open for writing",__FUNCTION__,DEV_PROP_FILE); | ||
| return 1; | ||
| } | ||
|
|
||
| if (fputs(is_bci, fp) == EOF) | ||
| { | ||
| APPLY_PRINT("[Utopia - %s]File %s write failed",__FUNCTION__,DEV_PROP_FILE); | ||
| fclose(fp); | ||
| return 1; | ||
| } | ||
|
|
||
| APPLY_PRINT("[Utopia - %s]File %s write successful",__FUNCTION__,DEV_PROP_FILE); | ||
|
|
||
| fclose(fp); | ||
|
|
||
| return 0; | ||
| } |
There was a problem hiding this comment.
The function unconditionally appends "IS_BCI=yes" to the file every time it's called, without checking if the entry already exists. This will result in duplicate entries if the program runs multiple times or if the entry is already present. Before appending, check if "IS_BCI" already exists in the file to prevent duplicates.
|
|
||
| if (0 == strcasecmp (PartnerID, "comcast-business")) | ||
| { | ||
| addPartnerDefaultsToDevPropFile(); |
There was a problem hiding this comment.
The return value of addPartnerDefaultsToDevPropFile() is not checked. If the function fails to write to the file (returns 1), the error is silently ignored. Consider checking the return value and logging an appropriate error message, similar to how other function calls in this section handle errors (see line 3563-3565).
| fp = fopen(DEV_PROP_FILE, "r"); | ||
| if (fp == NULL) | ||
| { | ||
| APPLY_PRINT("[Utopia - %s]File %s does not exist or cannot be accessed",__FUNCTION__,DEV_PROP_FILE); | ||
| return 1; | ||
| } | ||
| fclose(fp); // we don't need it anymore | ||
|
|
||
| //open for writing | ||
| fp = fopen(DEV_PROP_FILE, "a"); |
There was a problem hiding this comment.
Opening the file twice (first for read, then for append) creates a race condition window where the file could be deleted, modified, or have its permissions changed between the two operations. Consider combining the operations or using access() to check file existence if the only purpose of the first open is verification.
| fp = fopen(DEV_PROP_FILE, "a"); | ||
| if (fp == NULL) | ||
| { | ||
| APPLY_PRINT("[Utopia - %s]File %s couldnot open for writing",__FUNCTION__,DEV_PROP_FILE); |
There was a problem hiding this comment.
Spelling error in error message. "couldnot" should be two separate words: "could not".
| APPLY_PRINT("[Utopia - %s]File %s couldnot open for writing",__FUNCTION__,DEV_PROP_FILE); | |
| APPLY_PRINT("[Utopia - %s]File %s could not open for writing",__FUNCTION__,DEV_PROP_FILE); |
| #endif | ||
|
|
||
| #define DEV_PROP_FILE "/etc/device.properties" | ||
| static int addPartnerDefaultsToDevPropFile() |
There was a problem hiding this comment.
The function lacks documentation. Add a comment block before the function explaining its purpose, parameters (if any were to be added), return values, and any side effects (modifying /etc/device.properties file). This is especially important since this function modifies a system configuration file.
| } | ||
| #endif | ||
|
|
||
| #define DEV_PROP_FILE "/etc/device.properties" |
There was a problem hiding this comment.
The macro DEV_PROP_FILE is defined locally within the function scope when it should be defined at file scope with other macros (near lines 64-66). This placement is inconsistent with the established pattern in the codebase where file path macros are defined at the top of the file.
| FILE *fp = NULL; | ||
| const char *is_bci = "IS_BCI=yes\n"; | ||
| //open to check for existence | ||
| fp = fopen(DEV_PROP_FILE, "r"); |
There was a problem hiding this comment.
can we modify with access call?
| } | ||
|
|
||
| sysevent_close(global_fd, global_id); | ||
|
|
There was a problem hiding this comment.
can you please bring this under ONESTACK_PRODUCT_REQ
| static int addPartnerDefaultsToDevPropFile() | ||
| { | ||
| FILE *fp = NULL; | ||
| const char *is_bci = "IS_BCI=yes\n"; |
There was a problem hiding this comment.
Can we keep this in some kind of array and have a generic runner logic to add the paramaters in array.
This to support , if any additional parameters needed, we can add it in array
Reason for change : Modify device.properties file based on partner ID
Test Procedure : Check that /etc/device.properties contains "IS_BCI=yes"
Risks : Low
Priority: Medium
Signed-off-by: abhishek_kumaracee2@comcast.com