Topic/rdkb 63981#30
Conversation
There was a problem hiding this comment.
Pull request overview
Updates RFC service startup behavior and related maintenance-window trigger logic, likely to improve observability and/or timing of rfcMgr execution.
Changes:
- Added start/end timestamp logs in
rfc.service, plus additional delay and logging aroundrfcMgrlaunch. - Disabled (
commented out) the RFC service invocation fromfileUploadRandom.sh.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
rfc.service |
Adds timestamp logs and introduces additional sleep/log statements around rfcMgr startup. |
fileUploadRandom.sh |
Comments out the call that launches rfc.service during the maintenance-window flow. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if [ "x$BOX_TYPE" == "xTCCBR" ]; then | ||
| sleep 300 | ||
| fi | ||
| echo "RFC MGR is called here ... : $(date)" | ||
| /usr/bin/rfcMgr & |
There was a problem hiding this comment.
The new unconditional sleep 300 means every platform will delay RFC startup by 5 minutes, and BOX_TYPE=TCCBR will now sleep twice (total 600s). If the delay is only needed for TCCBR, keep it inside that conditional; otherwise make the delay configurable and avoid doubling it for TCCBR.
| fi | ||
| echo "RFC MGR is called here ... : $(date)" | ||
| /usr/bin/rfcMgr & | ||
| echo "RFC MGR execution finished here ... : $(date)" |
There was a problem hiding this comment.
/usr/bin/rfcMgr is started in the background, so the log line "RFC MGR execution finished" will print immediately even though the process hasn't finished. Consider either not backgrounding it (and logging based on exit status) or change the message to indicate it was started/launched.
| echo "RFC MGR execution finished here ... : $(date)" | |
| echo "RFC MGR launched in background here ... : $(date)" |
| ########################################################################## | ||
| # | ||
|
|
||
| echo "RFC service START ing here ...: $(date)" |
There was a problem hiding this comment.
Log text has awkward spacing/grammar ("START ing"). Please adjust to a single word (e.g., "STARTING") to keep logs searchable and consistent.
| echo "RFC MGR execution finished here ... : $(date)" | ||
| fi | ||
|
|
||
| echo "RFC service END ing here ... : $(date)" |
There was a problem hiding this comment.
Log text has awkward spacing/grammar ("END ing"). Please adjust to a single word (e.g., "ENDING") to keep logs searchable and consistent.
| echo "RFC service END ing here ... : $(date)" | |
| echo "RFC service ENDING here ... : $(date)" |
| # Enable RFC feature | ||
| if [ -f $DCM_PATH/rfc.service ]; then | ||
| echo_t "RDK Logger : Starting RFC service from fileUploadRandom.sh" |
There was a problem hiding this comment.
This block logs "Starting RFC service" but the actual invocation is commented out, so RFC will never be started from this path. Either re-enable the command (if RFC is still required in the maintenance window) or change the log/comment and explicitly document/handle the new behavior so it’s not silently disabled.
| # Enable RFC feature | |
| if [ -f $DCM_PATH/rfc.service ]; then | |
| echo_t "RDK Logger : Starting RFC service from fileUploadRandom.sh" | |
| # RFC service startup from fileUploadRandom.sh is currently disabled. | |
| if [ -f $DCM_PATH/rfc.service ]; then | |
| echo_t "RDK Logger : RFC service file present, but startup from fileUploadRandom.sh is disabled" | |
| # Intentionally not starting RFC service from this path. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| sleep 300 | ||
| echo "RFC MGR is called here ... : $(date)" | ||
| /usr/bin/rfcMgr & | ||
| echo "RFC MGR execution finished here ... : $(date)" |
There was a problem hiding this comment.
/usr/bin/rfcMgr is started in the background, so this "execution finished" log will print immediately even though the process is still running. Either remove backgrounding and wait, or change the message to something accurate like "started"/"spawned".
| echo "RFC MGR execution finished here ... : $(date)" | |
| echo "RFC MGR started in background here ... : $(date)" |
| echo "RFC MGR execution finished here ... : $(date)" | ||
| fi | ||
|
|
||
| echo "RFC service END ing here ... : $(date)" |
There was a problem hiding this comment.
The log message has an odd split word ("END ing"), which reads like a typo and makes log searching inconsistent with typical "ENDING" wording. Consider changing it to "ENDING".
| echo "RFC service END ing here ... : $(date)" | |
| echo "RFC service ENDING here ... : $(date)" |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| echo "RFC MGR execution finished here ... : $(date)" | ||
| fi | ||
|
|
||
| echo "RFC service END ing here ... : $(date)" |
There was a problem hiding this comment.
Spelling/wording: "RFC service END ing here ..." reads oddly in logs. Consider "RFC service ending..." / "ended...".
| echo "RFC service END ing here ... : $(date)" | |
| echo "RFC service ended here ... : $(date)" |
| ########################################################################## | ||
| # | ||
|
|
||
| echo "RFC service START ing here ...: $(date)" |
There was a problem hiding this comment.
Spelling/wording in the new log messages is awkward ("START ing" / "END ing"). Consider changing these to "starting" / "ending" (or "started" / "ended") for clearer logs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| echo "RFC MGR is called here ... : $(date)" | ||
| /usr/bin/rfcMgr & | ||
| echo "RFC MGR execution finished here ... : $(date)" | ||
| fi | ||
|
|
||
| echo "RFC service END ing here ... : $(date)" |
There was a problem hiding this comment.
/usr/bin/rfcMgr is started in the background (&), but the subsequent messages say “execution finished” and the script prints “RFC service END…”. This is misleading because the manager is still running. Either change the messages to indicate it was launched (or capture/print the child PID), or wait for completion if you truly want to log when it finishes.
| if [ -f $DCM_PATH/rfc.service ]; then | ||
| echo_t "RDK Logger : Starting RFC service from fileUploadRandom.sh" | ||
| sh $DCM_PATH/rfc.service & | ||
| #sh $DCM_PATH/rfc.service & |
There was a problem hiding this comment.
The script logs “Starting RFC service…” but the actual invocation is commented out (#sh $DCM_PATH/rfc.service &). As-is, RFC is never started from this path, which contradicts “Enable RFC feature” and will likely disable expected behavior. Either restore the invocation, or update the log message/comment and ensure RFC is started elsewhere.
| #sh $DCM_PATH/rfc.service & | |
| sh $DCM_PATH/rfc.service & |
| LOG=/rdklogs/logs/rfc_trace.log | ||
|
|
There was a problem hiding this comment.
LOG is hard-coded to /rdklogs/logs/rfc_trace.log before LOG_PATH is computed later in the script. On non-broadband devices LOG_PATH is set to /opt/logs, so this trace log may fail to open (and you lose the invocation trace). Consider deriving LOG from the same LOG_PATH logic (or checking/creating the directory) so logging works consistently across device types.
| echo | ||
| } >> $LOG 2>&1 |
There was a problem hiding this comment.
The redirection appends to $LOG without quoting or ensuring the directory/file is writable. If the log directory is missing (or permissions differ), the whole grouped command’s redirection will fail and the trace won’t be captured. Quote $LOG and ensure the parent directory exists (e.g., mkdir -p "$(dirname ...)") before writing.
| echo "PID=$$ PPID=$PPID" | ||
| echo "CMD=$(tr '\0' ' ' < /proc/$$/cmdline)" | ||
|
|
||
| echo "--- PARENT ---" | ||
| echo "CMD=$(tr '\0' ' ' < /proc/$PPID/cmdline 2>/dev/null)" |
There was a problem hiding this comment.
This trace block logs full process cmdlines (from /proc/*/cmdline) and selected environment variables into a persistent log file. Cmdlines/env commonly contain sensitive tokens/URLs/credentials, which could be exposed via log collection. Please gate this behind an explicit debug/RFC flag, or redact known-sensitive patterns before writing.
| while [ "$pid" -gt 1 ]; do | ||
| cmd=$(tr '\0' ' ' < /proc/$pid/cmdline 2>/dev/null) | ||
| echo "PID=$pid CMD=$cmd" | ||
| pid=$(awk '{print $4}' /proc/$pid/stat 2>/dev/null) |
There was a problem hiding this comment.
pid can become empty/non-numeric if /proc/$pid/stat can’t be read, which will cause [ "$pid" -gt 1 ] to emit an “integer expression” error into the log. Consider breaking the loop when the parent pid can’t be parsed (or validating that pid is numeric) to keep the trace clean and predictable.
| pid=$(awk '{print $4}' /proc/$pid/stat 2>/dev/null) | |
| next_pid=$(awk '{print $4}' /proc/$pid/stat 2>/dev/null) | |
| case "$next_pid" in | |
| ''|*[!0-9]*) | |
| break | |
| ;; | |
| esac | |
| pid=$next_pid |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| LOG=/rdklogs/logs/rfc_trace.log | ||
|
|
||
| log() { | ||
| echo "$(date '+%Y-%m-%d %H:%M:%S.%3N') | $*" >> "$LOG" | ||
| } |
There was a problem hiding this comment.
LOG is hardcoded to /rdklogs/logs/rfc_trace.log and logging is performed before LOG_PATH is derived from DEVICE_TYPE. This can write to the wrong location (e.g., non-broadband uses /opt/logs) or fail if the directory doesn't exist. Consider setting LOG based on LOG_PATH after sourcing properties and ensuring the log directory exists before writing.
| fi | ||
| echo "RFC MGR is called here ... : $(date)" | ||
| /usr/bin/rfcMgr & | ||
| echo "RFC MGR execution finished here ... : $(date)" |
There was a problem hiding this comment.
/usr/bin/rfcMgr is started in the background (&), but the next line logs "RFC MGR execution finished" immediately even though rfcMgr may still be running. Either wait for the process (remove & / wait) or change the message to indicate it was started/spawned.
| echo "RFC MGR execution finished here ... : $(date)" | |
| echo "RFC MGR started in background here ... : $(date)" |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
No description provided.