Date and Time enhancements and other fixes#1949
Conversation
- Support Precision Time Protocol (PTP) - time accuracy in nanoseconds - PTP requires a local time server which supports the PTP protocol - Show only the applicable fields for the selected time sync value (animated) - Add 'hints' to help the user in configuration - Do not restart time service when only date/time format is updated Note for Tom: to make PTP work the package "linuxptp-4.2-x86_64-1_BW.txz" needs to be installed with stock Unraid
- Add PTP support for bonded interfaces
- Add PTP support for bonded interfaces
- Disable PTP for bridge interfaces (unsupported)
- Give warning when no suitable interfaces are present for PTP
- Add Time State information
- Add time state info
- Better time state handling
- Add more info to NTP/PTP server message
![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
🔭 Outside diff range comments (1)
emhttp/plugins/dynamix/DashStats.page (1)
1438-1474: Enhance security in VM clone functionality.The VM clone function should include input validation and sanitization to prevent potential security issues.
function VMClone(uuid, name){ + // Validate input parameters + if (!uuid || typeof uuid !== 'string' || !name || typeof name !== 'string') { + console.error('Invalid parameters for VM clone'); + return; + } + + // Sanitize input to prevent XSS + name = $('<div>').text(name).html(); + box = $("#iframe-popup"); box.html($("#templateClone").html()); box.find('#VMBeingCloned').html(name).change(); - box.find('#target').val(name + "_clone"); + box.find('#target').val(name + "_clone").attr('maxlength', 64); document.getElementById("Free").checked = true; document.getElementById("Overwrite").checked = true; // ... rest of the function }
🧹 Nitpick comments (2)
emhttp/plugins/dynamix/DashStats.page (2)
672-674: Optimize tile visibility logic.The current implementation checks each tile's visibility individually. Consider combining these checks for better performance.
-if (count['db_box1']>0) $('div#tile1').show(); else $('div#tile1').hide(); -if (count['db_box2']>0) $('div#tile2').show(); else $('div#tile2').hide(); -if (count['db_box3']>0) $('div#tile3').show(); else $('div#tile3').hide(); +// Use a single jQuery call for better performance +$('div[id^="tile"]').each(function(index) { + $(this).toggle(count['db_box' + (index + 1)] > 0); +});
904-914: Add validation and error handling to chart functions.The
addChartCpufunction lacks input validation and error handling for edge cases.function addChartCpu(load) { + // Validate input + if (typeof load !== 'number' || isNaN(load)) { + console.error('Invalid load value:', load); + return; + } + + // Ensure load is within valid range + load = Math.max(0, Math.min(100, load)); + cputime++; var i = cpu.length - cpuline; if (i > 0) { // clear value outside graph i = i - 1; cpu[i].x = cputime - cpuline; cpu[i].y = 0; } cpu.push({x:cputime, y:load}); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
emhttp/plugins/dynamix/DashStats.page(35 hunks)
🔇 Additional comments (2)
emhttp/plugins/dynamix/DashStats.page (2)
842-844: 🛠️ Refactor suggestionAdd error handling to cookie saving mechanism.
The current implementation doesn't handle potential failures in the AJAX request or validate the cookie data before saving.
function saveCookie() { + // Validate cookie data + if (!cookie || typeof cookie !== 'object') { + console.error('Invalid cookie data'); + return; + } + + // Save cookie with error handling $.post('/webGui/include/InitCharts.php',{ cmd:'cookie', data:JSON.stringify(cookie) + }).fail(function(jqXHR, textStatus, errorThrown) { + console.error('Failed to save cookie:', textStatus, errorThrown); }); }Likely invalid or redundant comment.
846-867: 🛠️ Refactor suggestionRemove unused function.
The
sanitizeMultiCookiefunction appears to be unused after the transition to the new cookie management system.-function sanitizeMultiCookie(cookieName, delimiter, removeDuplicates=false) { - // Some browser states leave multi-value cookies with nulls, empties or duplicates. - // This function cleans up any such cookies so that they do not break functionality. - try { - var uncleanCookie = $.cookie(cookieName); - if (uncleanCookie) { - uncleanCookie = uncleanCookie.split(delimiter); - var cleanCookie = uncleanCookie.filter(n => n); - if (removeDuplicates) { cleanCookie = [...new Set(cleanCookie)]; } - if (JSON.stringify(uncleanCookie) !== JSON.stringify(cleanCookie)) { - $.cookie(cookieName,cleanCookie.join(delimiter),{expires:3650}); - return true; - } else { - return false; - } - } else { - return false; - } - } catch (ex) { - return false; - } -}Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (7)
emhttp/plugins/dynamix/include/Wireless.php (7)
16-22:⚠️ Potential issueEnhance security and error handling for configuration files.
The configuration file paths and their handling need improvements:
- Move sensitive file paths to a secure configuration file
- Add proper error handling for file operations
Apply this diff to improve error handling:
-$var = '/var/local/emhttp/var.ini'; -$cfg = '/boot/config/wireless.cfg'; -$ssl = '/etc/rc.d/rc.ssl.input'; -$tmp = '/var/tmp/attr'; -$wifi = is_readable($cfg) ? (array)parse_ini_file($cfg,true) : []; -$attr = is_readable($tmp) ? (array)parse_ini_file($tmp,true) : []; -$md5 = md5(json_encode($attr),true); +// Load configuration paths from a secure location +$config = require_once '/etc/unraid/config.php'; +$var = $config['var_path']; +$cfg = $config['wireless_config']; +$ssl = $config['ssl_input']; +$tmp = $config['temp_attr']; + +// Initialize with proper error handling +$wifi = []; +$attr = []; +try { + if (is_readable($cfg)) { + $result = @parse_ini_file($cfg, true); + if ($result === false) { + error_log("Failed to parse wireless config file: $cfg"); + } else { + $wifi = (array)$result; + } + } + if (is_readable($tmp)) { + $result = @parse_ini_file($tmp, true); + if ($result === false) { + error_log("Failed to parse wireless attributes file: $tmp"); + } else { + $attr = (array)$result; + } + } + $md5 = md5(json_encode($attr),true); +} catch (Exception $e) { + error_log("Error loading wireless configuration: " . $e->getMessage()); +}
36-52:⚠️ Potential issueAdd input validation and error handling in scanWifi function.
The function needs proper validation of the port parameter and error handling for the shell command execution.
Apply this diff to improve the function:
function scanWifi($port) { + if (!preg_match('/^[a-zA-Z0-9]+$/', $port)) { + error_log("Invalid wireless interface name: $port"); + return []; + } + if (!file_exists("/sys/class/net/$port")) { + error_log("Wireless interface not found: $port"); + return []; + } $wlan = []; - exec("iw ".escapeshellarg($port)." scan | grep -P '^BSS|signal:|SSID:|Authentication suites:'",$scan); + $result = 0; + exec("iw ".escapeshellarg($port)." scan | grep -P '^BSS|signal:|SSID:|Authentication suites:'",$scan, $result); + if ($result !== 0) { + error_log("Failed to scan wireless networks on interface: $port"); + return []; + }
54-62:⚠️ Potential issueAdd error handling in saveWifi function.
The function should handle potential file write failures.
Apply this diff to improve error handling:
function saveWifi() { global $cfg, $wifi; $text = []; foreach ($wifi as $network => $block) { $text[] = "[$network]"; foreach ($block as $key => $value) $text[] = "$key=\"$value\""; } - file_put_contents_atomic($cfg,implode("\n",$text)."\n"); + try { + if (!file_put_contents_atomic($cfg,implode("\n",$text)."\n")) { + throw new Exception("Failed to write WiFi configuration"); + } + } catch (Exception $e) { + error_log("Error saving WiFi configuration: " . $e->getMessage()); + return false; + } + return true; }
122-124:⚠️ Potential issueEnhance security for password handling.
The password decryption lacks proper validation and error handling.
Apply this diff to improve security:
- $user = _var($wifi[$ssid],'USERNAME') && isset($cipher,$key,$iv) ? openssl_decrypt($wifi[$ssid]['USERNAME'],$cipher,$key,0,$iv) : _var($wifi[$ssid],'USERNAME'); - $passwd = _var($wifi[$ssid],'PASSWORD') && isset($cipher,$key,$iv) ? openssl_decrypt($wifi[$ssid]['PASSWORD'],$cipher,$key,0,$iv) : _var($wifi[$ssid],'PASSWORD'); + $user = ''; + $passwd = ''; + if (_var($wifi[$ssid],'USERNAME') && isset($cipher, $key, $iv)) { + $decrypted = openssl_decrypt($wifi[$ssid]['USERNAME'],$cipher,$key,0,$iv); + if ($decrypted === false) { + error_log("Failed to decrypt username for network: $ssid"); + } else { + $user = $decrypted; + } + } + if (_var($wifi[$ssid],'PASSWORD') && isset($cipher, $key, $iv)) { + $decrypted = openssl_decrypt($wifi[$ssid]['PASSWORD'],$cipher,$key,0,$iv); + if ($decrypted === false) { + error_log("Failed to decrypt password for network: $ssid"); + } else { + $passwd = $decrypted; + } + }
151-159:⚠️ Potential issueAdd CSRF token validation.
The form includes a CSRF token but there's no validation before processing the form data.
Add token validation before processing the form:
+ // Add at the beginning of the switch statement + if ($cmd !== 'list') { + $stored_token = parse_ini_file($var)['csrf_token'] ?? ''; + if (!hash_equals($stored_token, $_POST['csrf_token'])) { + error_log("CSRF token validation failed"); + http_response_code(403); + exit("Access denied"); + } + }
185-187:⚠️ Potential issueAdd input validation for IP addresses.
The form accepts IP addresses without validation.
Apply this diff to add validation:
- echo "<tr class=\"static4 $hide2\"><td>"._('IPv4 address').":</td><td><input type=\"text\" name=\"IP4\" class=\"narrow\" maxlength=\"15\" autocomplete=\"off\" spellcheck=\"false\" value=\"$ip4\">/<select name=\"MASK4\" class=\"slim\">"; + echo "<tr class=\"static4 $hide2\"><td>"._('IPv4 address').":</td><td><input type=\"text\" name=\"IP4\" class=\"narrow\" maxlength=\"15\" pattern=\"^((25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\.){3}(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)$\" title=\""._('Enter a valid IPv4 address')."\" autocomplete=\"off\" spellcheck=\"false\" value=\"$ip4\">/<select name=\"MASK4\" class=\"slim\">";
211-215:⚠️ Potential issueAdd validation before network removal.
The 'forget' command should validate the network existence and user permissions before removal.
Apply this diff to add validation:
case 'forget': $ssid = rawurldecode($_POST['ssid']); + if (!isset($wifi[$ssid])) { + error_log("Attempt to remove non-existent network: $ssid"); + break; + } if ($wifi[$ssid]['GROUP'] == 'active') exec("/etc/rc.d/rc.wireless stop &>/dev/null &"); unset($wifi[$ssid]); saveWifi(); break;
🧹 Nitpick comments (2)
etc/rc.d/rc.wireless (2)
28-29: Add validation for configuration values.The script sources configuration values without validation. Consider adding checks for required configuration values to fail early if essential settings are missing.
[[ -r $INI ]] && . $INI +# Validate required configuration values +[[ -z $PORT ]] && log "Warning: PORT not configured in $INI" PORT=${PORT:-wlan0} +# Validate interface existence +[[ ! -e $SYSTEM/$PORT ]] && log "Warning: Interface $PORT does not exist"
82-125: Refactor IP address assignment for better maintainability.The
ipaddr_upfunction is handling multiple concerns (DHCP, static IP, DNS) and is quite long. Consider splitting it into smaller, focused functions.+# Handle DHCP configuration +dhcp_up() { + local OPTIONS="-q -n -p -t 10" + [[ -n $HOSTNAME ]] && OPTIONS="$OPTIONS -h $HOSTNAME" + [[ $DNS == yes ]] && OPTIONS="$OPTIONS -C resolv.conf" + [[ $IP == ipv4 ]] && OPTIONS="$OPTIONS -4" + [[ $IP == ipv6 ]] && OPTIONS="$OPTIONS -6" + if carrier_up $PORT; then + log "interface $PORT is UP, polling up to 60 sec for DHCP $IP server" + if ! run timeout 60 dhcpcd -w $OPTIONS $PORT; then + log "can't obtain IP address, continue polling in background on interface $PORT" + run dhcpcd -b $OPTIONS $PORT + fi + else + log "interface $PORT is DOWN, polling DHCP $IP server in background" + run dhcpcd -b $OPTIONS $PORT + fi +} + +# Handle static IP configuration +static_up() { + if carrier_up $PORT; then STATE="UP"; else STATE="DOWN"; fi + log "interface $PORT is $STATE, setting static $IP address" + ipv6_addr $PORT 0 1 + if [[ $IP == ipv4 ]]; then + [[ -n $IP4 && -n $MASK4 ]] && run ip -4 addr add $(unzero $IP4)/$MASK4 dev $PORT metric 3004 + [[ -n $GATEWAY4 ]] && run ip -4 route add default via $GATEWAY4 dev $PORT metric 3004 + fi + if [[ $IP == ipv6 ]]; then + [[ -n $IP6 && -n $MASK6 ]] && run ip -6 addr add $(unzero6 $IP6)/$MASK6 dev $PORT metric 3004 + [[ -n $GATEWAY6 ]] && run ip -6 route add default via $GATEWAY6 dev $PORT metric 3004 + fi +} + ipaddr_up(){ if [[ $DHCP == yes ]]; then - # bring up interface using DHCP/SLAAC - ipv6_addr 1 1 - OPTIONS="-q -n -p -t 10" - [[ -n $HOSTNAME ]] && OPTIONS="$OPTIONS -h $HOSTNAME" - [[ $DNS == yes ]] && OPTIONS="$OPTIONS -C resolv.conf" - [[ $IP == ipv4 ]] && OPTIONS="$OPTIONS -4" - [[ $IP == ipv6 ]] && OPTIONS="$OPTIONS -6" - if carrier_up $PORT; then - # interface is UP - log "interface $PORT is UP, polling up to 60 sec for DHCP $IP server" - if ! run timeout 60 dhcpcd -w $OPTIONS $PORT; then - log "can't obtain IP address, continue polling in background on interface $PORT" - run dhcpcd -b $OPTIONS $PORT - fi - else - # interface is DOWN - log "interface $PORT is DOWN, polling DHCP $IP server in background" - run dhcpcd -b $OPTIONS $PORT - fi + dhcp_up elif [[ $DHCP == no ]]; then - # bring up interface using static IP address - if carrier_up $PORT; then STATE="UP"; else STATE="DOWN"; fi - log "interface $PORT is $STATE, setting static $IP address" - ipv6_addr $PORT 0 1 - if [[ $IP == ipv4 ]]; then - [[ -n $IP4 && -n $MASK4 ]] && run ip -4 addr add $(unzero $IP4)/$MASK4 dev $PORT metric 3004 - [[ -n $GATEWAY4 ]] && run ip -4 route add default via $GATEWAY4 dev $PORT metric 3004 - fi - if [[ $IP == ipv6 ]]; then - [[ -n $IP6 && -n $MASK6 ]] && run ip -6 addr add $(unzero6 $IP6)/$MASK6 dev $PORT metric 3004 - [[ -n $GATEWAY6 ]] && run ip -6 route add default via $GATEWAY6 dev $PORT metric 3004 - fi + static_up fi # ... rest of the function }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
emhttp/plugins/dynamix/Wireless.page(1 hunks)emhttp/plugins/dynamix/include/Wireless.php(1 hunks)etc/rc.d/rc.wireless(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- emhttp/plugins/dynamix/Wireless.page
🔇 Additional comments (6)
etc/rc.d/rc.wireless (6)
31-42: Add input validation for IP address formatting functions.The
unzeroandunzero6functions lack input validation and may not handle edge cases properly.
44-57: Enhance security of SSL key generation.The SSL key/iv generation uses hardcoded lengths and lacks error handling for dmidecode failures.
67-80: Add error handling for sysfs operations.The IPv6 configuration functions don't verify if the sysfs writes succeed.
155-174: Enhance security of WPA configuration handling.The
wpa_configurationfunction writes sensitive data to the configuration file without proper file permissions management.
203-225: Add verification for network shutdown operations.The
wifi_stopfunction should verify the success of each operation.
227-269: Add error handling to the OpenSSL settings retrieval.The function should handle cases where the SSL file is unreadable or decryption fails.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
etc/rc.d/rc.wireless (3)
9-16: Consider using readonly for constant variables.These variables appear to be constants and should be protected from modification.
-DAEMON="WiFi network" -CALLER="wifi" -INI="/var/local/emhttp/wireless.ini" -CFG="/boot/config/wireless.cfg" -SSLINPUT="/etc/rc.d/rc.ssl.input" -STARTWIFI="/usr/local/emhttp/webGui/scripts/wireless" -WPA="/etc/wpa_supplicant.conf" +readonly DAEMON="WiFi network" +readonly CALLER="wifi" +readonly INI="/var/local/emhttp/wireless.ini" +readonly CFG="/boot/config/wireless.cfg" +readonly SSLINPUT="/etc/rc.d/rc.ssl.input" +readonly STARTWIFI="/usr/local/emhttp/webGui/scripts/wireless" +readonly WPA="/etc/wpa_supplicant.conf"
59-65: Add timeout configuration and improve error handling.The carrier detection loop uses hardcoded values and could benefit from configurable timeouts.
+readonly CARRIER_TIMEOUT=${CARRIER_TIMEOUT:-10} +readonly CARRIER_INTERVAL=${CARRIER_INTERVAL:-1} carrier_up(){ local n - for n in {1..10}; do + for ((n=0; n<CARRIER_TIMEOUT; n++)); do [[ $(cat $SYSTEM/$1/carrier 2>/dev/null) == 1 ]] && return 0 || sleep 1 done + log "Carrier detection timeout after ${CARRIER_TIMEOUT} seconds" return 1 }
284-303: Add validation for command-line arguments.The case statement should validate that exactly one argument is provided.
+if [[ $# -ne 1 ]]; then + echo "Error: Exactly one argument required" + echo "Usage: $BASENAME start|stop|join|restart|status" + exit 1 +fi + case "$1" in 'start') wifi_start
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
emhttp/plugins/dynamix/DashStats.page(37 hunks)etc/rc.d/rc.docker(8 hunks)etc/rc.d/rc.wireless(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- emhttp/plugins/dynamix/DashStats.page
- etc/rc.d/rc.docker
🔇 Additional comments (5)
etc/rc.d/rc.wireless (5)
31-35: Add input validation for IPv4 address format.The
unzerofunction lacks input validation which could lead to unexpected behavior with invalid IPv4 addresses.
67-74: Add error handling for sysfs operations.The IPv6 configuration function doesn't verify if the sysfs writes succeed.
135-138: Fix IPv6 route flushing condition.There appears to be a typo in the IPv6 route flushing condition.
152-171: Enhance security of WPA configuration handling.The WPA configuration function writes sensitive data to the configuration file without proper file permissions.
234-235: Add error handling for failed decryption.The password decryption doesn't validate success which could lead to connection attempts with invalid credentials.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (3)
emhttp/plugins/dynamix.docker.manager/include/DockerClient.php (3)
339-340: Ensure$ct['Networks']is not empty before usingreset()To prevent potential errors when accessing
'IPAddress', confirm that$ct['Networks']is not empty before callingreset(). This ensures the code does not produce an error if no networks are defined for the container.Apply this diff to add a check:
+if (!empty($ct['Networks'])) { $ip = reset($ct['Networks'])['IPAddress']; +} else { + $ip = null; +}
1011-1018: Ensure$driver[$netName]index exists before accessingWhen accessing
$driver[$netName], ensure the key exists to avoid undefined index notices. This can be achieved by adding anisset()check.Apply this diff to add the check:
-if ($driver[$netName]=='ipvlan' || $driver[$netName]=='macvlan') { +if (isset($driver[$netName]) && ($driver[$netName]=='ipvlan' || $driver[$netName]=='macvlan')) {
1090-1094: Avoid usingextract()to prevent potential variable collisionsUsing
extract()can lead to unexpected variable overwriting and reduces code readability. It's safer to assign variables explicitly.Apply this diff to assign variables explicitly:
-public static function ensureImageTag($image): string -{ - extract(static::parseImageTag($image)); - return "$strRepo:$strTag"; +public static function ensureImageTag($image): string +{ + $parsed = static::parseImageTag($image); + $strRepo = $parsed['strRepo']; + $strTag = $parsed['strTag']; + return "$strRepo:$strTag"; }
🧹 Nitpick comments (2)
emhttp/plugins/dynamix.docker.manager/include/DockerClient.php (2)
1009-1009: Consider avoiding reference assignment if not modifying the originalAssigning
$portsby reference using&may not be necessary if you are not modifying$info['Config']['ExposedPorts']. Assigning by value improves code readability and reduces potential side effects.Apply this diff to remove the reference:
-$ports = &$info['Config']['ExposedPorts']; +$ports = $info['Config']['ExposedPorts'];
1173-1174: Consider using built-in PHP functions instead ofexecUsing
execto execute shell commands can be avoided by leveraging PHP's built-in functions, improving portability and security.Suggested alternative:
public static function cpus() { exec('cat /sys/devices/system/cpu/*/topology/thread_siblings_list|sort -nu', $cpus); return $cpus; }Can be refactored to:
public static function cpus() { $cpus = []; foreach (glob('/sys/devices/system/cpu/*/topology/thread_siblings_list') as $filename) { $content = file($filename, FILE_IGNORE_NEW_LINES | FILE_SKIP_EMPTY_LINES); $cpus = array_merge($cpus, $content); } $cpus = array_unique($cpus); sort($cpus, SORT_NUMERIC); return $cpus; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
emhttp/plugins/dynamix.docker.manager/include/DockerClient.php(1 hunks)
🔇 Additional comments (5)
emhttp/plugins/dynamix.docker.manager/include/DockerClient.php (5)
39-43: Dynamic selection of network interface enhances robustnessThe updated logic correctly selects the network interface by checking for 'br0', 'bond0', and defaults to 'eth0', improving adaptability to various network configurations. The fallback to 'wlan0' ensures wireless interfaces are considered when no wired interfaces are available.
682-684: Verify that adding 'Mode' to XPath query does not skip matching elementsBy including
[@Mode='$mode']in the XPath query, the code may skip matchingConfigelements that lack theModeattribute. Ensure this change does not prevent updating existing port configurations and that all relevantConfigelements have theModeattribute set appropriately.
990-992: Properly retrieve additional container labelsThe added lines correctly extract
net.unraid.docker.tailscale.hostname,net.unraid.docker.shell, andnet.unraid.docker.managedlabels from the container configuration, enhancing container metadata handling.
1096-1116: Efficient parsing of image tags implementedThe method
parseImageTagcorrectly parses Docker image tags, handling cases like images starting withsha256and images without a tag by defaulting tolatest.
1120-1132: Robust image splitting logic implementedThe
splitImagemethod effectively parses the image string using regular expressions and handles edge cases appropriately, returningnullwhen unprocessable.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
emhttp/plugins/dynamix/include/Wrappers.php (1)
259-263: Consider using multiple connectivity check endpoints.The
check_network_connectivityfunction relies solely on Microsoft's NCSI endpoint. Consider adding fallback endpoints for more reliable connectivity checks.function check_network_connectivity(): bool { - $url = 'http://www.msftncsi.com/ncsi.txt'; - $out = http_get_contents($url); - return ($out=="Microsoft NCSI"); + $endpoints = [ + ['url' => 'http://www.msftncsi.com/ncsi.txt', 'expected' => 'Microsoft NCSI'], + ['url' => 'http://www.apple.com/library/test/success.html', 'expected' => 'Success'], + ['url' => 'http://connectivity-check.ubuntu.com/', 'expected' => 'NetworkManager is online'] + ]; + + foreach ($endpoints as $endpoint) { + $out = http_get_contents($endpoint['url']); + if ($out === $endpoint['expected']) return true; + } + return false; }
🧹 Nitpick comments (2)
emhttp/plugins/dynamix/include/Wrappers.php (2)
52-58: Consider adding error handling for file operations.The
parse_plugin_cfgfunction performs file operations without explicit error handling. While it uses the null coalescing operator for the array merge, it might be good to add logging for file access failures.function parse_plugin_cfg($plugin, $sections=false, $scanner=INI_SCANNER_NORMAL) { global $docroot; $ram = "$docroot/plugins/$plugin/default.cfg"; $rom = "/boot/config/plugins/$plugin/$plugin.cfg"; - $cfg = file_exists($ram) ? my_parse_ini_file($ram, $sections, $scanner) : []; - return file_exists($rom) ? array_replace_recursive($cfg, my_parse_ini_file($rom, $sections, $scanner)) : $cfg; + try { + $cfg = file_exists($ram) ? my_parse_ini_file($ram, $sections, $scanner) : []; + return file_exists($rom) ? array_replace_recursive($cfg, my_parse_ini_file($rom, $sections, $scanner)) : $cfg; + } catch (Exception $e) { + my_logger("Error parsing plugin configuration for $plugin: " . $e->getMessage()); + return []; + } }
175-191: Consider caching NVMe information for performance.The
get_nvme_infofunction makes multiple system calls which could be expensive if called frequently. Consider caching the results for a short period.+private static $nvme_cache = []; +private static $nvme_cache_ttl = 60; // 60 seconds + function get_nvme_info($device, $info) { + $cache_key = "{$device}_{$info}"; + if (isset(self::$nvme_cache[$cache_key]) && + time() - self::$nvme_cache[$cache_key]['time'] < self::$nvme_cache_ttl) { + return self::$nvme_cache[$cache_key]['value']; + } + switch ($info) { case 'temp': exec("nvme id-ctrl /dev/$device 2>/dev/null | grep -Pom2 '^[wc]ctemp +: \K\d+'",$temp); - return [$temp[0]-273, $temp[1]-273]; + $result = [$temp[0]-273, $temp[1]-273]; + break; // ... rest of the cases ... } + + self::$nvme_cache[$cache_key] = [ + 'value' => $result, + 'time' => time() + ]; + return $result; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
emhttp/plugins/dynamix.docker.manager/include/DockerClient.php(7 hunks)emhttp/plugins/dynamix.docker.manager/include/DockerContainers.php(8 hunks)emhttp/plugins/dynamix.docker.manager/include/Helpers.php(2 hunks)emhttp/plugins/dynamix/include/Wrappers.php(8 hunks)sbin/create_network_ini(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- emhttp/plugins/dynamix.docker.manager/include/Helpers.php
🚧 Files skipped from review as they are similar to previous changes (2)
- sbin/create_network_ini
- emhttp/plugins/dynamix.docker.manager/include/DockerContainers.php
🔇 Additional comments (4)
emhttp/plugins/dynamix/include/Wrappers.php (2)
2-3: LGTM! Copyright years updated.Copyright years have been consistently updated to 2025 for both Lime Technology and Bergware International.
265-269: LGTM! Well-structured network port check function.The
lan_portfunction efficiently checks both port existence and carrier state with proper error handling.emhttp/plugins/dynamix.docker.manager/include/DockerClient.php (2)
2-3: LGTM! Copyright years updated.The copyright years have been appropriately updated to reflect the current year.
1170-1183: LGTM! Well-structured network utility methods.The new
port()andhost()methods provide a robust implementation for:
- Prioritized network interface detection (br0 → bond0 → eth0 → wlan0)
- Fallback to wlan0 when appropriate
- Clean IP address retrieval using standard Linux tools
| global $host; | ||
| $port = &$ct['Ports'][0]; | ||
| $myIP = $myIP ?: $this->getTemplateValue($ct['Image'],'MyIP') ?: (_var($ct,'NetworkMode')=='host'||_var($port,'NAT') ? $host : (_var($port,'IP') ?: DockerUtil::myIP($ct['Name']))); | ||
| $myIP = $myIP ?: $this->getTemplateValue($ct['Image'],'MyIP') ?: (_var($ct,'NetworkMode')=='host'||_var($port,'NAT') ? DockerUtils::host() : (_var($port,'IP') ?: DockerUtil::myIP($ct['Name']))); |
There was a problem hiding this comment.
Fix typo in class name.
There's a typo in the class name: DockerUtils::host() should be DockerUtil::host() (without the 's').
Apply this diff to fix the typo:
-$myIP = $myIP ?: $this->getTemplateValue($ct['Image'],'MyIP') ?: (_var($ct,'NetworkMode')=='host'||_var($port,'NAT') ? DockerUtils::host() : (_var($port,'IP') ?: DockerUtil::myIP($ct['Name'])));
+$myIP = $myIP ?: $this->getTemplateValue($ct['Image'],'MyIP') ?: (_var($ct,'NetworkMode')=='host'||_var($port,'NAT') ? DockerUtil::host() : (_var($port,'IP') ?: DockerUtil::myIP($ct['Name'])));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $myIP = $myIP ?: $this->getTemplateValue($ct['Image'],'MyIP') ?: (_var($ct,'NetworkMode')=='host'||_var($port,'NAT') ? DockerUtils::host() : (_var($port,'IP') ?: DockerUtil::myIP($ct['Name']))); | |
| $myIP = $myIP ?: $this->getTemplateValue($ct['Image'],'MyIP') ?: (_var($ct,'NetworkMode')=='host'||_var($port,'NAT') ? DockerUtil::host() : (_var($port,'IP') ?: DockerUtil::myIP($ct['Name']))); |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
emhttp/plugins/dynamix.docker.manager/include/DockerClient.php (2)
2-3: Update copyright years.The copyright years have been updated to 2025, but line 4 still shows 2021 as the end year.
Apply this diff to maintain consistency in copyright years:
/* Copyright 2005-2025, Lime Technology * Copyright 2012-2025, Bergware International. - * Copyright 2014-2021, Guilherme Jardim, Eric Schultz, Jon Panozzo. + * Copyright 2014-2025, Guilherme Jardim, Eric Schultz, Jon Panozzo.
1171-1177: Add error handling and logging to port() method.The port() method provides a clean interface selection mechanism but could benefit from error handling and logging.
Consider adding logging to track interface selection:
public static function port() { + $logger = new Logger('docker'); if (lan_port('br0')) { + $logger->info('Using br0 interface'); return 'br0'; } if (lan_port('bond0')) { + $logger->info('Using bond0 interface'); return 'bond0'; } if (lan_port('eth0')) { + $logger->info('Using eth0 interface'); return 'eth0'; } if (lan_port('wlan0')) { + $logger->info('Using wlan0 interface'); return 'wlan0'; } + $logger->warning('No valid network interface found'); return ''; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
emhttp/plugins/dynamix.docker.manager/include/DockerClient.php(7 hunks)
🔇 Additional comments (2)
emhttp/plugins/dynamix.docker.manager/include/DockerClient.php (2)
17-17: LGTM! Proper dependency ordering.Moving the Helpers.php require statement before class usage ensures proper initialization.
951-952: LGTM! Good encapsulation of network interface logic.The use of DockerUtil::driver() and DockerUtil::host() centralizes network interface handling, improving maintainability.
| public static function host() { | ||
| $port = static::port(); | ||
| if (!$port) return ''; | ||
| $port = lan_port($port,true)==0 && lan_port('wlan0') ? 'wlan0' : $port; | ||
| return exec("ip -br -4 addr show $port scope global | sed -r 's/\/[0-9]+//g' | awk '{print $3;exit}'"); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add IP address validation to host() method.
The host() method should validate the IP address format and handle command failures.
Apply this diff to add validation:
public static function host() {
$port = static::port();
if (!$port) return '';
$port = lan_port($port,true)==0 && lan_port('wlan0') ? 'wlan0' : $port;
- return exec("ip -br -4 addr show $port scope global | sed -r 's/\/[0-9]+//g' | awk '{print $3;exit}'");
+ $ip = exec("ip -br -4 addr show $port scope global | sed -r 's/\/[0-9]+//g' | awk '{print $3;exit}'", $output, $return_var);
+ if ($return_var !== 0) {
+ error_log("Failed to get IP address for interface $port");
+ return '';
+ }
+ if (!filter_var($ip, FILTER_VALIDATE_IP, FILTER_FLAG_IPV4)) {
+ error_log("Invalid IPv4 address format: $ip");
+ return '';
+ }
+ return $ip;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public static function host() { | |
| $port = static::port(); | |
| if (!$port) return ''; | |
| $port = lan_port($port,true)==0 && lan_port('wlan0') ? 'wlan0' : $port; | |
| return exec("ip -br -4 addr show $port scope global | sed -r 's/\/[0-9]+//g' | awk '{print $3;exit}'"); | |
| } | |
| public static function host() { | |
| $port = static::port(); | |
| if (!$port) return ''; | |
| $port = lan_port($port, true) == 0 && lan_port('wlan0') ? 'wlan0' : $port; | |
| $ip = exec("ip -br -4 addr show $port scope global | sed -r 's/\/[0-9]+//g' | awk '{print $3;exit}'", $output, $return_var); | |
| if ($return_var !== 0) { | |
| error_log("Failed to get IP address for interface $port"); | |
| return ''; | |
| } | |
| if (!filter_var($ip, FILTER_VALIDATE_IP, FILTER_FLAG_IPV4)) { | |
| error_log("Invalid IPv4 address format: $ip"); | |
| return ''; | |
| } | |
| return $ip; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
emhttp/plugins/dynamix.docker.manager/include/DockerClient.php (1)
1179-1184: 🛠️ Refactor suggestionAdd error handling and validation to host() method.
The
host()method should include error handling for command execution failures and IP address validation.Apply this diff to improve error handling:
public static function host() { $port = static::port(); if (!$port) return ''; $port = lan_port($port,true)==0 && lan_port('wlan0') ? 'wlan0' : $port; - return exec("ip -br -4 addr show $port scope global | sed -r 's/\/[0-9]+//g' | awk '{print $3;exit}'"); + $ip = exec("ip -br -4 addr show $port scope global | sed -r 's/\/[0-9]+//g' | awk '{print $3;exit}'", $output, $return_var); + if ($return_var !== 0) { + error_log("Failed to get IP address for interface $port"); + return ''; + } + if (!filter_var($ip, FILTER_VALIDATE_IP, FILTER_FLAG_IPV4)) { + error_log("Invalid IPv4 address format: $ip"); + return ''; + } + return $ip; }
🧹 Nitpick comments (2)
emhttp/plugins/dynamix.docker.manager/include/DockerClient.php (2)
1153-1153: Improve error handling in custom() method.The
custom()method should handle the case where thedocker network lscommand fails.Apply this diff to add error handling:
- return static::docker("network ls --filter driver='bridge' --filter driver='macvlan' --filter driver='ipvlan' --format='{{.Name}}' 2>/dev/null | grep -v '^bridge$'",true); + $result = static::docker("network ls --filter driver='bridge' --filter driver='macvlan' --filter driver='ipvlan' --format='{{.Name}}' 2>/dev/null | grep -v '^bridge$'",true); + if (!is_array($result)) { + error_log("Failed to get custom network list"); + return []; + } + return $result;
1163-1164: Add error handling to cpus() method.The
cpus()method should handle the case where the command fails or the file is not accessible.Apply this diff to add error handling:
- exec('cat /sys/devices/system/cpu/*/topology/thread_siblings_list | sort -nu', $cpus); - return $cpus; + if (@exec('cat /sys/devices/system/cpu/*/topology/thread_siblings_list | sort -nu', $cpus, $return_var) === false || $return_var !== 0) { + error_log("Failed to get CPU topology information"); + return []; + } + return $cpus;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
emhttp/plugins/dynamix.docker.manager/include/DockerClient.php(8 hunks)
🔇 Additional comments (4)
emhttp/plugins/dynamix.docker.manager/include/DockerClient.php (4)
2-3: LGTM: Copyright years updated.The copyright years have been updated to include 2025.
1171-1177: LGTM: Network interface detection method.The
port()method provides a clear priority order for network interfaces (br0 > bond0 > eth0 > wlan0).
17-17: Verify the dependency order change.The
Helpers.phpinclusion has been moved before other includes. Ensure this doesn't create any dependency issues with functions or classes defined in other included files.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Check for any dependencies in Helpers.php that might require other includes rg -A 5 "require|include" "plugins/dynamix.docker.manager/include/Helpers.php"Length of output: 169
Let's gather more information about the location of
Helpers.phpto ensure we're inspecting the correct file. It appears that the previous command couldn’t find the file at
plugins/dynamix.docker.manager/include/Helpers.php. We should verify whether the file exists elsewhere in the repository.To do that, please run the following shell script:
🏁 Script executed:
#!/bin/bash # Search for any file named Helpers.php in the repository find . -type f -name "Helpers.php"Length of output: 139
Dependency order verified as safe.
After verifying the location and contents of the
Helpers.phpfile (located atemhttp/plugins/dynamix.docker.manager/include/Helpers.php), the file shows no additionalrequireorincludestatements that would create dependency issues when loaded before other includes. The absence of internal dependencies inHelpers.phpindicates that moving its inclusion earlier does not adversely affect the later code dependencies.
271-271: Verify the IP address determination logic.The IP address determination logic has been updated to use
DockerUtil::host(). Ensure this change maintains backward compatibility with existing container configurations.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Check for any existing containers that might be affected by the IP address change ast-grep --pattern $'$myIP = $_ ?: $this->getTemplateValue($ct[\'Image\'],\'MyIP\') ?: (_var($ct,\'NetworkMode\')==\'host\'||_var($port,\'NAT\') ? DockerUtil::host() : (_var($port,\'IP\') ?: DockerUtil::myIP($ct[\'Name\'])))'Length of output: 254
I'll gather additional context by inspecting a few lines around the IP determination logic in the file so we can assess the complete context and verify backward compatibility.
🏁 Script executed:
#!/bin/bash # Print lines 265-275 from the file to inspect the surrounding context of the IP logic. sed -n '265,275p' emhttp/plugins/dynamix.docker.manager/include/DockerClient.phpLength of output: 639
IP Address Determination Logic Verified
The examined assignment preserves backward compatibility:
- The logic first uses any preexisting IP in
$myIPor the template value for'MyIP'.- When neither is defined, it correctly falls back to using
DockerUtil::host()if either the container’s network mode is'host'or the NAT flag is set.- If those conditions aren’t met, it then checks for an IP in
$portor, as a last resort, callsDockerUtil::myIP().All intended conditions are represented, and existing container configurations should remain unaffected.
3 participants
Summary by CodeRabbit
New Features
wlan0network interface with real-time status updates.Improvements