Revert diagnostics changes#2169
Conversation
WalkthroughThe script in the diagnostics plugin has been updated to alter how diagnostic messages are sent and how commands are executed. The Changes
Sequence Diagram(s)sequenceDiagram
participant Script
participant DiagnosticsEndpoint
Script->>DiagnosticsEndpoint: POST /pub/diagnostics?buffer_length=1 (via Unix socket) [for each message]
Possibly related PRs
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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 (1)
emhttp/plugins/dynamix/scripts/diagnostics (1)
47-60: Improved diagnostics messaging with direct socket communicationThe
write()function has been fundamentally changed to use a persistent cURL handle to POST messages directly to a local diagnostics endpoint via a Unix socket instead of using thepublish()function. This likely provides more reliable and direct communication.However, there's no error handling for potential cURL failures, which could occur if the nginx server isn't running or accessible.
Consider adding basic error handling:
function write(...$messages){ $com = curl_init(); curl_setopt_array($com,[ CURLOPT_URL => 'http://localhost/pub/diagnostics?buffer_length=1', CURLOPT_UNIX_SOCKET_PATH => '/var/run/nginx.socket', CURLOPT_POST => 1, CURLOPT_RETURNTRANSFER => true ]); foreach ($messages as $message) { curl_setopt($com, CURLOPT_POSTFIELDS, $message); - curl_exec($com); + if (!curl_exec($com) && $cli) { + echo "Warning: Failed to send diagnostic message.\n"; + } } curl_close($com); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
emhttp/plugins/dynamix/scripts/diagnostics(3 hunks)
🔇 Additional comments (5)
emhttp/plugins/dynamix/scripts/diagnostics (5)
29-29: Switched from publish.php to Wrappers.php dependencyThis change aligns with the modifications to the
write()function that now uses direct curl calls instead of the previouspublish()function.
62-63: Updated function comment for run()The comment indicates the run function now includes error logging, though the implementation doesn't show explicit error logging changes.
64-64: Added global variable declarationsThe function now declares
$cliand$diagas global variables, making the function more maintainable by explicitly showing its dependencies.
471-471: Made IP masking conditional based on the all flagIP masking for system/ifconfig.txt is now conditional on
$allbeing false. This is consistent with the purpose of having a comprehensive diagnostic mode that includes all information when requested.
68-69:Details
❓ Verification inconclusive
Simplified command execution by removing locale setting
The explicit locale setting (
LC_ALL=en_US.UTF-8) has been removed from the exec command, which simplifies the code. However, this could potentially lead to locale-dependent output formatting which might affect parsing in some environments.The trailing space in the string concatenation has also been removed, which is a good cleanup.
Verify if any command outputs are sensitive to locale settings:
🏁 Script executed:
#!/bin/bash # Check if any commands rely on specific locale formatting grep -r "LC_ALL" /usr/local/emhttp/ --include="*.php" | grep -v "diagnostics"Length of output: 127
🏁 Script executed:
#!/bin/bash # Search for explicit locale settings across the repository grep -R "LC_ALL" -n .Length of output: 858
🏁 Script executed:
#!/bin/bash # Display beginning of diagnostics script to inspect commands and locale usage sed -n '1,200p' emhttp/plugins/dynamix/scripts/diagnosticsLength of output: 7906
🏁 Script executed:
#!/bin/bash # Check for any usage of date commands in diagnostics script grep -R -n "date " emhttp/plugins/dynamix/scripts/diagnosticsLength of output: 379
Review locale handling in diagnostics script
The PHP calls to setlocale(LC_ALL, 'en_US.UTF-8') in local_prepend.php only affect PHP’s internal functions and do not set the shell environment for exec(). By removing the
LC_ALL=en_US.UTF-8prefix, all commands invoked via:
- File:
emhttp/plugins/dynamix/scripts/diagnostics
Location:run()function (around lines 65–75)
Snippet:function run($cmd, &$save=null, $timeout=30) { // … exec("timeout -s9 $timeout $cmd", $save); return implode("\n",$save); }will now inherit the system’s default locale, which may vary between installations. If any output parsing (e.g. of
df,smartctl, date stamps, decimal separators, month names) depends on a consistent locale, you should:
- Verify that unRAID’s default locale is uniformly set to en_US.UTF-8 (or C) in all environments
- Or explicitly prefix the command in
run()withLC_ALL=C(or your desired locale) to ensure consistent, locale-independent outputPlease confirm which approach is appropriate for your target deployments.
|
Superseded by #2170 |
2 participants
Summary by CodeRabbit
Bug Fixes
Other Improvements