Restore nchan_stub_status function#2109
Conversation
WalkthroughA new location block for Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant N as Nginx
participant M as Nchan Module
C->>N: Request `/nchan_stub_status`
N->>M: Process Nchan status request
M-->>N: Return status information
N-->>C: Deliver response
Possibly related PRs
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
etc/rc.d/rc.nginx (2)
396-398: Consider adding access restrictions for securityWhile the change itself is correct, the nchan_stub_status endpoint provides internal module information without any access controls. Consider adding IP restrictions or authentication to prevent unauthorized access to this monitoring endpoint.
location /nchan_stub_status { + # Restrict access to local networks + allow 127.0.0.1; + allow 192.168.0.0/16; + allow 10.0.0.0/8; + allow 172.16.0.0/12; + deny all; nchan_stub_status; }
396-398: Consider adding a descriptive comment for consistencyFor consistency with other location blocks in this file, consider adding a descriptive comment to explain the purpose of this endpoint.
+# +# nchan status endpoint +# location /nchan_stub_status { nchan_stub_status; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
etc/rc.d/rc.nginx(1 hunks)
🔇 Additional comments (1)
etc/rc.d/rc.nginx (1)
396-398: LGTM: Successfully restored the nchan_stub_status functionThe addition of this location block properly restores the nchan_stub_status functionality, making it accessible via HTTP/HTTPS interfaces. This complements the existing endpoint available through the unix socket at lines 154-156.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
sbin/monitor_nchan (3)
29-34: Consider adding a comment to document the purpose of the "kill" command.Adding a brief comment explaining that this command is intended for forced stops and complete cleanup would improve maintainability.
+# Kill command: completely terminate nchan processes and remove related files (used for forced stops) if [[ $1 == kill ]]; then echo "Stopping nchan processes..." nchan_stop rm -f $nchan_pid $disk_load exit fi
10-17: Consider adding error handling to the cleanup operations.The
nchan_stopfunction and file removal operations don't include error checking. It might be helpful to add basic error handling to ensure that processes are properly terminated and files are successfully removed.nchan_stop() { echo -n >$nchan_list + local kill_status=0 while IFS=$'\n' read -r nchan; do [[ ${nchan##*/} == '.*' ]] && continue echo $nchan >>$nchan_list pkill -f $nchan + [[ $? -ne 0 ]] && kill_status=1 done <<< $(ps -eo cmd | grep -Po '/usr/local/emhttp/.*/nchan/.*') + return $kill_status } if [[ $1 == kill ]]; then echo "Stopping nchan processes..." nchan_stop + [[ $? -ne 0 ]] && echo "Warning: Some nchan processes may not have been terminated properly" rm -f $nchan_pid $disk_load + [[ $? -ne 0 ]] && echo "Warning: Failed to remove some nchan files" exit fiAlso applies to: 29-34
36-40: Consider refactoring to reduce code duplication between "kill" and "stop" commands.The "kill" and "stop" commands share similar logic, with the main difference being the file removal. Consider refactoring to reduce duplication while maintaining the distinct behaviors.
nchan_stop() { echo -n >$nchan_list while IFS=$'\n' read -r nchan; do [[ ${nchan##*/} == '.*' ]] && continue echo $nchan >>$nchan_list pkill -f $nchan done <<< $(ps -eo cmd | grep -Po '/usr/local/emhttp/.*/nchan/.*') } +# Handle stop operations with optional cleanup +handle_stop() { + echo "Stopping nchan processes..." + nchan_stop + [[ $1 == "cleanup" ]] && rm -f $nchan_pid $disk_load + exit +} if [[ $1 == kill ]]; then - echo "Stopping nchan processes..." - nchan_stop - rm -f $nchan_pid $disk_load - exit + handle_stop cleanup fi if [[ $1 == stop ]]; then - echo "Stopping nchan processes..." - nchan_stop - exit + handle_stop fiAlso applies to: 29-34
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (2)
etc/rc.d/rc.nginx(2 hunks)sbin/monitor_nchan(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- etc/rc.d/rc.nginx
🔇 Additional comments (1)
sbin/monitor_nchan (1)
29-34: The new "kill" command handler looks good, but has subtle differences from the existing "stop" command.The new feature provides a clean way to stop Nchan processes and remove related files in one operation. This aligns with the PR objective of restoring the nchan_stub_status function by providing a more complete termination option.
Note that this differs from the existing "stop" command (lines 36-40) which only stops processes but doesn't remove files. According to the AI summary, this is replacing the previous pattern of using "monitor_nchan stop remove" with "monitor_nchan kill" in the
nginx_stop_forcedfunction.
2 participants
Summary by CodeRabbit