Service management enhancements and other fixes#2025
Conversation
…rface" This reverts commit 22e7f27.
- restart ssh daemon only when changes are present - restart telnet daemon only when changes are present
Put update requests in a job queue and replace the job queue when new update requests are presented within the scheduled wait time This approach makes only the last update request running New log entries are provided to follow execution of the jobs
WalkthroughThe changes modify several service management scripts by refining control flow and error handling. The Changes
Sequence Diagram(s)sequenceDiagram
participant Script as Service Script
participant Scheduler as "at Queue"
participant Logger as Log System
participant ServiceLoop as "Service Loop (rpc, nfsd, ...)"
Script->>Scheduler: Check for existing job (atq)
alt Job exists
Scheduler-->>Script: Return Job ID
Script->>Scheduler: Remove job (atrm)
Script->>Logger: Log job removal
Script->>ServiceLoop: Execute service reload loop
else No job found
Script->>Logger: Log no job found and exit
end
Script->>Scheduler: Schedule new job with fixed delay
Script->>Logger: Log new job ID and wait time
sequenceDiagram
participant UA as update_access Script
participant Config as Config File (/boot/config/ident.cfg)
participant SSHD as SSH Daemon
participant TELNET as inetd Service
UA->>Config: Read current SSH port
alt Current port ≠ Desired port
UA->>Config: Update SSH configuration
UA->>SSHD: Check SSH process (pgrep)
alt SSH not running
UA->>SSHD: Start SSH daemon
else SSH running but changed port
UA->>SSHD: Restart SSH daemon
end
else
UA->>SSHD: No action needed
end
UA->>Config: Read inetd configuration for TELNET
alt TELNET configuration change needed
UA->>TELNET: Update inetd config (enable/disable)
UA->>TELNET: Restart inetd service
else
UA->>TELNET: No action taken
end
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (7)
💤 Files with no reviewable changes (2)
✅ Files skipped from review due to trivial changes (4)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (6)
emhttp/plugins/dynamix/scripts/update_services (1)
14-15: Consider adding error handling for theatcommand.While the implementation works, it silently ignores
atcommand failures. Consider checking its exit status and logging any errors.-echo "/usr/local/emhttp/webGui/scripts/reload_services ${1:-1}" | at -M now 2>/dev/null -log "queue new job $(atq | grep -Pom1 '^\d+'), wait for ${1:-1}s" +if ! NEW_JOB=$(echo "/usr/local/emhttp/webGui/scripts/reload_services ${1:-1}" | at -M now 2>&1); then + log "failed to queue new job: $NEW_JOB" + exit 1 +fi +JOB_ID=$(atq | grep -Pom1 '^\d+') +log "queue new job $JOB_ID, wait for ${1:-1}s"emhttp/plugins/dynamix/scripts/reload_services (2)
4-5: Consider using a more precise sleep duration.The sleep duration uses a default of 1 second, but floating-point values could provide more precise control if needed.
-sleep ${1:-1} +sleep ${1:-1.0}
10-16: Consider logging when exiting due to no queued job.The script silently exits when no job is found. Consider logging this case for better observability.
if [[ -n $JOB ]]; then atrm $JOB 2>/dev/null log "execute queued job $JOB" else + log "no queued job found, exiting" exit 0 fiemhttp/plugins/dynamix/scripts/update_access (3)
9-9: Consider adding error handling for configuration loading.The script silently continues if the configuration file exists but has syntax errors.
-[[ -e /boot/config/ident.cfg ]] && . <(/usr/bin/fromdos </boot/config/ident.cfg) +if [[ -e /boot/config/ident.cfg ]]; then + if ! . <(/usr/bin/fromdos </boot/config/ident.cfg); then + echo "Error loading configuration file" + exit 1 + fi +fi
29-40: Consider adding error handling for SSH service operations.The implementation should handle potential failures when starting/stopping the SSH daemon.
if [[ $USE_SSH == yes ]]; then if [[ ! $(pgrep --ns $$ -cf $SSHD) -gt 0 ]]; then - /etc/rc.d/rc.sshd start + if ! /etc/rc.d/rc.sshd start; then + echo "Failed to start SSH daemon" + exit 1 + fi else [[ $PORTSSH != $CURRENT ]] && /etc/rc.d/rc.sshd restart fi elif [[ $(pgrep --ns $$ -cf $SSHD) -gt 0 ]]; then - /etc/rc.d/rc.sshd stop + if ! /etc/rc.d/rc.sshd stop; then + echo "Failed to stop SSH daemon" + exit 1 + fi fi
54-67: Consider adding error handling for inetd service operations.The implementation should handle potential failures when restarting the inetd service.
if [[ $USE_TELNET == yes ]]; then if [[ -n $(grep -o '^#telnet' $INET) ]]; then sed -ri "s/^#(telnet\s.+telnetd\$)/\1/" $INET - /etc/rc.d/rc.inetd restart + if ! /etc/rc.d/rc.inetd restart; then + echo "Failed to restart inetd service" + exit 1 + fi elif [[ $PORTTELNET != $CURRENT ]]; then - /etc/rc.d/rc.inetd restart + if ! /etc/rc.d/rc.inetd restart; then + echo "Failed to restart inetd service" + exit 1 + fi fi elif [[ -n $(grep -o '^telnet' $INET) ]]; then sed -ri 's/^(telnet\s.+telnetd$)/#\1/' $INET - /etc/rc.d/rc.inetd restart + if ! /etc/rc.d/rc.inetd restart; then + echo "Failed to restart inetd service" + exit 1 + fi fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
emhttp/plugins/dynamix/scripts/reload_services(1 hunks)emhttp/plugins/dynamix/scripts/update_access(3 hunks)emhttp/plugins/dynamix/scripts/update_services(1 hunks)emhttp/plugins/dynamix/scripts/wireless(1 hunks)sbin/create_network_ini(2 hunks)
🔇 Additional comments (8)
emhttp/plugins/dynamix/scripts/update_services (1)
6-12: LGTM! Improved job management with proper cleanup.The implementation correctly handles existing jobs by checking and removing them before scheduling new ones, preventing job accumulation.
emhttp/plugins/dynamix/scripts/wireless (1)
30-30:Details
❓ Verification inconclusive
Verify if removing the delay parameter is intentional.
The delay parameter has been removed from the
update_servicescall. This might affect the timing of service updates when stopping wireless. Consider if this change aligns with the intended behavior.
🏁 Script executed:
#!/bin/bash # Description: Check if other scripts in the codebase use update_services with or without delay # Search for update_services usage patterns rg -A 2 "update_services" --type php --type bashLength of output: 77
Action Required: Confirm Delay Parameter Removal
Please verify that the removal of the delay parameter in the call to
update_services(file:emhttp/plugins/dynamix/scripts/wireless, line 30) was intentional, as this change might affect the timing of service updates when stopping wireless. A previous search for other usages ofupdate_serviceswith a delay argument did not yield clear results (note: the initial command using--type bashfailed, so please manually inspect or re-run the search using the appropriate file types, e.g.,--type sh). If the change is deliberate, consider updating comments or documentation to clarify this altered behavior.emhttp/plugins/dynamix/scripts/update_access (3)
3-6: LGTM! Clear variable declarations.The variables are well-named and their purposes are clear from the context.
19-26: LGTM! Improved SSH port configuration.The implementation correctly handles port changes and maintains default port configuration.
50-51: LGTM! Improved TELNET port configuration.The implementation correctly handles port changes in the services file.
sbin/create_network_ini (3)
17-17: LGTM! Good practice using a variable for system paths.Using
$SYSto store the path to network interfaces improves maintainability and follows best practices by avoiding hardcoded paths.
211-211: Verify if the fixed 20-second delay is appropriate.The script now uses a fixed 20-second delay instead of a variable delay. This might be:
- Too long for systems that are ready sooner
- Too short for systems that need more time to initialize
Consider making this delay configurable or determining it dynamically based on system state.
218-220: LGTM! Consistent use of $SYS variable for interface paths.Good improvement using the
$SYSvariable consistently for checking network interfaces. This makes the code more maintainable and follows the DRY principle.Also applies to: 225-225
This prevents 'access refused', which may happen when nginx gets initially started with no IP assignment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
etc/rc.d/rc.nginx (2)
778-784: LGTM with minor suggestions for improvement.The enhanced error handling for binding failures is a good addition. The automatic recovery using
nginx_renewis particularly useful.Consider these minor improvements:
- Extract the magic number '8' into a named constant
- Make the grep pattern more specific to avoid false positives
+readonly SYSLOG_LINES_TO_CHECK=8 +readonly BIND_ERROR_PATTERN='^.*nginx.*Address already in use.*$' + nginx_reload(){ # ... - if tail -8 $SYSLOG | grep -qm1 'Address already in use'; then + if tail -n $SYSLOG_LINES_TO_CHECK $SYSLOG | grep -qm1 "$BIND_ERROR_PATTERN"; then # ...
794-801: Add error handling and logging to the renewal process.The
nginx_renewfunction provides a robust recovery mechanism, but could benefit from error handling and logging.nginx_renew(){ + log "Renewing $DAEMON configuration..." # stop unconditionally pkill -f $NGINX + nginx_waitfor_shutdown # rebuild configuration build_ssl # start unconditionally - $NGINX -c $CONF 2>/dev/null + if ! $NGINX -c $CONF 2>/dev/null; then + log "Failed to start $DAEMON during renewal" + return 1 + fi + log "$DAEMON renewed successfully" }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
etc/rc.d/rc.nginx(2 hunks)
🔇 Additional comments (1)
etc/rc.d/rc.nginx (1)
34-34: LGTM!The
SYSLOGvariable is well-named and uses the standard system log path.
| <form markdown="1" name="network_extra" method="POST" action="/update.php" target="progressFrame" onsubmit="return prepareText(this)"> | ||
| <input type="hidden" name="#file" value="<?=$cfg?>"> | ||
| <input type="hidden" name="#command" value="/webGui/scripts/reload_services"> | ||
| <input type="hidden" name="#command" value="/webGui/scripts/update_services"> |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
LGTM! Verify dependent systems are updated.
The change to use update_services instead of reload_services is a good improvement, as it introduces better job management with proper handling of queued tasks.
Run this script to check for any other files that might still be using the old service:
🏁 Script executed:
#!/bin/bash
# Search for any remaining references to the old reload_services script
rg -l "reload_services" --type-add 'web:*.{php,page,htm,html}' -g '!NetworkExtra.page'Length of output: 133
Heads Up: Verify Consistency in Service Scripts
While updating the hidden input in emhttp/plugins/dynamix/NetworkExtra.page to use /webGui/scripts/update_services is a solid improvement for better job management, our scan detected a reference to reload_services in the file emhttp/plugins/dynamix/scripts/update_services. Please review that file to confirm whether:
- The reference to
reload_servicesis intentional (e.g., for backward compatibility or logging purposes), or - It should be updated to align with the new command naming for consistency across the system.
Once verified, update as needed to ensure all dependent systems are synchronized with the change.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
emhttp/plugins/dynamix/scripts/reload_services (1)
10-16: Enhance job management robustness and documentation.While the job management logic works, consider these improvements:
- Add error handling for atrm failures
- Add comments explaining the job management logic
- Consider logging when no job is found for better debugging
+# check and remove any queued reload job JOB=$(atq | grep -Pom1 '^\d+') if [[ -n $JOB ]]; then - atrm $JOB 2>/dev/null + if ! atrm $JOB 2>/dev/null; then + log "failed to remove job $JOB" + exit 1 + fi log "execute queued job $JOB" else + log "no queued job found" exit 0 fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
emhttp/plugins/dynamix/scripts/reload_services(1 hunks)
🔇 Additional comments (2)
emhttp/plugins/dynamix/scripts/reload_services (2)
4-6: LGTM! Good use of common functions.Sourcing common run & log functions promotes code reuse and consistent logging across scripts.
7-9: LGTM! Well-implemented delay mechanism.The configurable delay with a sensible default helps prevent race conditions. Good use of parameter expansion for the delay value.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
emhttp/plugins/dynamix/scripts/reload_services (1)
22-24: 🛠️ Refactor suggestionEnhance error handling and status reporting.
The current implementation suppresses all error output and doesn't check the status of service updates. Consider:
- Logging service update status for better observability
- Handling service update failures gracefully
for cmd in $SERVICES; do - /etc/rc.d/rc.$cmd update &>/dev/null + if ! /etc/rc.d/rc.$cmd update 2>&1; then + log "Failed to update service: $cmd" + else + log "Successfully updated service: $cmd" + fi done
🧹 Nitpick comments (1)
emhttp/plugins/dynamix/scripts/reload_services (1)
11-20: Consider increasing the default sleep duration.While the sleep duration is configurable, the default of 1 second might be too short for some services to properly shut down before being restarted. Consider increasing it to a more conservative value (e.g., 3-5 seconds) to ensure service stability.
-sleep ${1:-1} +sleep ${1:-3}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
emhttp/plugins/dynamix/scripts/reload_services(1 hunks)emhttp/plugins/dynamix/scripts/update_access(3 hunks)emhttp/plugins/dynamix/scripts/update_services(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- emhttp/plugins/dynamix/scripts/update_services
- emhttp/plugins/dynamix/scripts/update_access
🔇 Additional comments (2)
emhttp/plugins/dynamix/scripts/reload_services (2)
4-5: LGTM! Good practice for consistent logging.Including the run & log functions from a shared source ensures consistent logging across the system.
7-9: LGTM! Efficient job ID extraction.The
queue()function efficiently extracts the first job ID using grep with appropriate flags and regex pattern.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
emhttp/plugins/dynamix.vm.manager/sheets/UpdateVM.css (1)
50-50: Enhance Font-Family Robustness for IPVTAP SpanThe
span.ipvtaprule sets the font tobitstream. For broader compatibility and a more robust design, consider specifying one or more fallback fonts (e.g.,bitstream, sans-serif) so that if the primary font isn't available, the browser can use a fallback.emhttp/plugins/dynamix.vm.manager/sheets/AddVM.css (1)
50-50: Specify Fallback Fonts for IPVTAP SpanFor the
span.ipvtapstyle, consider extending thefont-familydeclaration to include fallback fonts (for example,bitstream, sans-serif). This change improves reliability in font rendering if the primary font isn’t installed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
emhttp/plugins/dynamix.vm.manager/sheets/AddVM.css(1 hunks)emhttp/plugins/dynamix.vm.manager/sheets/UpdateVM.css(1 hunks)emhttp/plugins/dynamix.vm.manager/templates/Custom.form.php(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- emhttp/plugins/dynamix.vm.manager/templates/Custom.form.php
🔇 Additional comments (4)
emhttp/plugins/dynamix.vm.manager/sheets/UpdateVM.css (2)
47-47: Consistent Styling for Input ElementsThe combined selector for
input#btnvCPUSelect, input.wlan0_infoensures that both elements share the same font size, padding, and margin, which promotes a consistent appearance. Verify that these identical styles are intended for both selectors.
53-53: Proper Alignment for WLAN SectionThe new
div.wlan0rule settingtext-align: lefteffectively aligns WLAN-related content as expected within the interface. This addition reinforces alignment consistency across the form.emhttp/plugins/dynamix.vm.manager/sheets/AddVM.css (2)
47-47: Consistent Input Styling Across FormsAdding
input.wlan0_infoalongsideinput#btnvCPUSelectensures that both elements share uniform styling. This consistency is beneficial for maintaining a cohesive UI across different VM management forms.
51-51: WLAN Element AlignmentThe newly introduced
div.wlan0rule setstext-align: left, which helps in aligning WLAN information correctly on the form. This aligns well with the updated UI requirements.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
emhttp/plugins/dynamix/sheets/Eth0.css (1)
14-14: Clarify the Duplicate Styling IntentThe new rule for
.sweet-alert fieldset input[type=text]in this file provides styling identical to the one for non-fieldset inputs in the other CSS file. Please confirm that this duplication is intentional (e.g., to handle cases where inputs are wrapped in a fieldset with potentially different behavior) and does not lead to unexpected overrides. If both are needed, consider adding a comment explaining the rationale for future maintainers.emhttp/plugins/dynamix/styles/jquery.sweetalert.css (1)
29-29: Assess the Broadening of the CSS SelectorThe selector has been changed from
.sweet-alert fieldset input[type=text]to.sweet-alert input[type=text], which now targets all text inputs within.sweet-alertcontainers rather than only those inside a fieldset. Please verify that this broader scope is intended and does not inadvertently affect other input elements that should have distinct styling. If the change is deliberate, consider adding documentation or inline comments to clarify the design decision.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
emhttp/plugins/dynamix/sheets/Eth0.css(1 hunks)emhttp/plugins/dynamix/styles/jquery.sweetalert.css(1 hunks)
2 participants
Summary by CodeRabbit
wlan0interface to improve VM network configuration awareness.