From 189e3af29ed68e2c1cfbaa5a7cfbf2ea47110b7e Mon Sep 17 00:00:00 2001 From: Franco Fichtner Date: Wed, 13 Sep 2023 10:09:26 +0200 Subject: [PATCH] interfaces: improve bridge code a bit * Allow the bridge to come up without members. While the GUI made sure to avoid empty bridges there is no reason for it and underneath if the interface was disabled the GUI made the interface disappear from the selection. So now allow an empty bridge (may be nice for migration) and show disabled interfaces in the edit page. Fix all callers to not assume the member property is always set. Can probably go away once bridges are moved to MVC. * Inline interface_bridge_add_member() since link_interface_to_bridge() is the only caller. Improve the parameter passing a bit too. * Add bridge interface return code to (_)interfaces_bridge_configure(). * Improve device resolution and a few mwexecf() replacements. * Log the reason why a device could not be attached to bridge when one device is not there as expected. --- src/etc/inc/filter.inc | 2 +- src/etc/inc/interfaces.inc | 154 +++++++++++++---------------- src/www/interfaces_assign.php | 2 +- src/www/interfaces_bridge.php | 18 ++-- src/www/interfaces_bridge_edit.php | 19 ++-- 5 files changed, 89 insertions(+), 106 deletions(-) diff --git a/src/etc/inc/filter.inc b/src/etc/inc/filter.inc index 10f3b9dbb..10a6a5c7e 100644 --- a/src/etc/inc/filter.inc +++ b/src/etc/inc/filter.inc @@ -652,7 +652,7 @@ function filter_rules_legacy(&$FilterIflist) $bridge_interfaces = []; if (!empty($config['bridges']['bridged'])) { foreach ($config['bridges']['bridged'] as $bridge) { - $bridge_interfaces = array_merge(explode(',', $bridge['members']), $bridge_interfaces); + $bridge_interfaces = array_merge(explode(',', $bridge['members'] ?? ''), $bridge_interfaces); } } foreach ($FilterIflist as $on => $oc) { diff --git a/src/etc/inc/interfaces.inc b/src/etc/inc/interfaces.inc index 571dc5411..968eae5fd 100644 --- a/src/etc/inc/interfaces.inc +++ b/src/etc/inc/interfaces.inc @@ -213,73 +213,66 @@ function interfaces_bridge_configure($device) global $config; if (!isset($config['bridges']['bridged'])) { - return; + return null; } foreach ($config['bridges']['bridged'] as $bridge) { if ($bridge['bridgeif'] == $device) { - _interfaces_bridge_configure($bridge); - return; + return _interfaces_bridge_configure($bridge); } } } function _interfaces_bridge_configure($bridge) { - if (empty($bridge['members'])) { - /* XXX really necessary? we would like our bridge anyway */ - log_msg("No members found on {$bridge['bridgeif']}", LOG_WARNING); - return; + /* XXX avoid destroy/create */ + legacy_interface_destroy($bridge['bridgeif']); + legacy_interface_create($bridge['bridgeif']); + + $checklist = get_configured_interface_with_descr(); + $members = []; + + /* find all required members */ + foreach (explode(',', $bridge['members'] ?? '') as $member) { + if (empty($checklist[$member])) { + /* ignores disabled ones */ + continue; + } + + $device = get_real_interface($member); + if (!does_interface_exist($device)) { + log_msg("Device {$bridge['bridgeif']} cannot attach non-existent member {$device}, skipping now."); + continue; + } + + $members[$member] = $device; } - $members = explode(',', $bridge['members']); - - /* Calculate smaller mtu and enforce it */ + /* calculate smaller mtu and enforce it */ $mtu = null; - foreach ($members as $member) { - $opts = legacy_interface_stats(get_real_interface($member)); + foreach ($members as $member => $device) { + $opts = legacy_interface_stats($device); if (!empty($opts['mtu']) && ($mtu == null || $opts['mtu'] < $mtu)) { $mtu = $opts['mtu']; } } - /* XXX avoid destroy/create */ - legacy_interface_destroy($bridge['bridgeif']); - legacy_interface_create($bridge['bridgeif']); - mwexecf('/sbin/ifconfig %s inet6 %sauto_linklocal', [$bridge['bridgeif'], isset($bridge['linklocal']) ? '' : '-']); - $checklist = get_configured_interface_with_descr(); - - /* Add interfaces to bridge */ - foreach ($members as $member) { - if (empty($checklist[$member])) { - continue; - } - - /* XXX why can we not iterate this once for MTU check and interface add */ - $realif = get_real_interface($member); - if (!does_interface_exist($realif)) { - log_msg("interface '{$realif}' not defined in interfaces bridge - up", LOG_WARNING); - continue; - } - - /* make sure the parent interface is up */ - configure_interface_hardware($realif); - legacy_interface_mtu($realif, $mtu); - interfaces_bring_up($realif); - legacy_bridge_member($bridge['bridgeif'], $realif); + /* add member interfaces to bridge */ + foreach ($members as $member => $device) { + configure_interface_hardware($device); + legacy_interface_mtu($device, $mtu); + interfaces_bring_up($device); + legacy_bridge_member($bridge['bridgeif'], $device); } if (isset($bridge['enablestp'])) { - /* Choose spanning tree proto */ - mwexec("/sbin/ifconfig {$bridge['bridgeif']} proto " . escapeshellarg($bridge['proto'])); - + mwexecf('/sbin/ifconfig %s proto %s', [$bridge['bridgeif'], $bridge['proto']]); if (!empty($bridge['stp'])) { $stpifs = explode(',', $bridge['stp']); foreach ($stpifs as $stpif) { - $realif = get_real_interface($stpif); - mwexec("/sbin/ifconfig {$bridge['bridgeif']} stp {$realif}"); + mwexecf('/sbin/ifconfig %s stp %s', [$bridge['bridgeif'], get_real_interface($stpif)]); } } if (!empty($bridge['maxage'])) { @@ -381,29 +374,8 @@ function _interfaces_bridge_configure($bridge) } interfaces_bring_up($bridge['bridgeif']); -} -function interface_bridge_add_member($bridgeif, $realif) -{ - $ifconfig_details = legacy_interfaces_details(); - - if (empty($ifconfig_details[$bridgeif]) || empty($ifconfig_details[$realif])) { - return; - } - - configure_interface_hardware($realif); - - /* - * The MTU of the first member interface to be added is used as the bridge - * MTU. All additional members are required to have exactly the same MTU - * value. - */ - if ($ifconfig_details[$bridgeif]['mtu'] != $ifconfig_details[$realif]['mtu']) { - legacy_interface_mtu($realif, $ifconfig_details[$bridgeif]['mtu']); - } - - interfaces_bring_up($realif); - legacy_bridge_member($bridgeif, $realif); + return $bridge['bridgeif']; } function interfaces_lagg_configure($verbose = false) @@ -2353,12 +2325,12 @@ function interface_configure($verbose = false, $interface = 'wan', $reload = fal interfaces_vips_configure($interface); - /* XXX device plugin hook */ + /* XXX device dependency plugin hook */ $loaded = array_merge($loaded, link_interface_to_gre($interface, true)); $loaded = array_merge($loaded, link_interface_to_gif($interface, true)); - /* XXX bridge hook does "something" else -- should we attach $realifv6 too? ;) */ - link_interface_to_bridge($interface, $realif); + /* XXX device member plugin hook */ + link_interface_to_bridge($interface, $realif, $ifconfig_details); /* XXX reload routes for linked devices -- not great but also not avoidable at the moment */ interfaces_restart_by_device($verbose, $loaded, false); @@ -3428,7 +3400,7 @@ function interface_parent_devices($device) } elseif (strstr($device, 'bridge')) { foreach (config_read_array('bridges', 'bridged') as $bridge) { if ($device == $bridge['bridgeif']) { - foreach (explode(',', $bridge['members']) as $member) { + foreach (explode(',', $bridge['members'] ?? '') as $member) { /* bridge stores members as configured interfaces */ $parents[] = get_real_interface($member); } @@ -3638,25 +3610,41 @@ function interfaces_restart_by_device($verbose, $devices, $reconfigure = true) } } -function link_interface_to_bridge($interface, $attach_device = null) +function link_interface_to_bridge($interface, $attach_device = null, $ifconfig_details = [/* must be set for attach to work */]) { - global $config; - - if (isset($config['bridges']['bridged'])) { - foreach ($config['bridges']['bridged'] as $bridge) { - if (!in_array($interface, explode(',', $bridge['members']))) { - continue; - } - - if ($attach_device) { - /* XXX _interfaces_bridge_configure() is probably cleaner */ - interface_bridge_add_member($bridge, $attach_device); - /* XXX bridges can only load if all interfaces are there */ - } - - return $bridge['bridgeif']; + foreach (config_read_array('bridges', 'bridged') as $bridge) { + if (!in_array($interface, explode(',', $bridge['members'] ?? ''))) { + continue; } + + if ($attach_device) { + if (empty($ifconfig_details[$bridge['bridgeif']])) { + log_msg("Device {$attach_device} cannot be added to non-existent {$bridge['bridgeif']}, skipping now."); + break; + } elseif (empty($ifconfig_details[$attach_device])) { + log_msg("Device {$attach_device} was not found so it cannot be added to {$bridge['bridgeif']}, skipping now."); + break; + } + + configure_interface_hardware($attach_device); + + /* + * The MTU of the first member interface to be added is used as the bridge + * MTU. All additional members are required to have exactly the same MTU + * value. + */ + if ($ifconfig_details[$bridge['bridgeif']]['mtu'] != $ifconfig_details[$attach_device]['mtu']) { + legacy_interface_mtu($attach_device, $ifconfig_details[$bridge['bridgeif']]['mtu']); + } + + interfaces_bring_up($attach_device); + legacy_bridge_member($bridge['bridgeif'], $attach_device); + } + + return $bridge['bridgeif']; } + + return null; } function link_interface_to_gre($interface, $update = false, $family = null) diff --git a/src/www/interfaces_assign.php b/src/www/interfaces_assign.php index 1648e766c..4dcb9c3f3 100644 --- a/src/www/interfaces_assign.php +++ b/src/www/interfaces_assign.php @@ -219,7 +219,7 @@ if ($_SERVER['REQUEST_METHOD'] === 'POST') { continue; } - $members = explode(",", strtoupper($bridge['members'])); + $members = explode(',', strtoupper($bridge['members'] ?? '')); foreach ($members as $member) { if ($member == $ifnames[0]) { $input_errors[] = sprintf(gettext("You cannot set port %s to interface %s because this interface is a member of %s."), $portname, $member, $portname); diff --git a/src/www/interfaces_bridge.php b/src/www/interfaces_bridge.php index 0ef3d2b4e..fb993a1e5 100644 --- a/src/www/interfaces_bridge.php +++ b/src/www/interfaces_bridge.php @@ -120,17 +120,15 @@ legacy_html_escape_form_data($a_bridges); $member) { + if (!isset($ifdescrs[$member])) { + unset($members[$key]); + } else { + $members[$key] = $ifdescrs[$member]; } - if ($j > 0 && $j < count($members)) { - echo ", "; - } - }?> + } + echo implode(', ', $members); ?> diff --git a/src/www/interfaces_bridge_edit.php b/src/www/interfaces_bridge_edit.php index 3ae9853c2..90a613ef9 100644 --- a/src/www/interfaces_bridge_edit.php +++ b/src/www/interfaces_bridge_edit.php @@ -36,9 +36,12 @@ $a_bridges = &config_read_array('bridges', 'bridged'); // interface list $ifacelist = []; -foreach (legacy_config_get_interfaces(['virtual' => false, 'enable' => true]) as $intf => $intfdata) { +foreach (legacy_config_get_interfaces(['virtual' => false]) as $intf => $intfdata) { if (substr($intfdata['if'], 0, 3) != 'gre' && substr($intfdata['if'], 0, 2) != 'lo') { $ifacelist[$intf] = $intfdata['descr']; + if (!isset($intfdata['enable'])) { + $ifacelist[$intf] .= ' (' . gettext('disabled') . ')'; + } } } @@ -93,12 +96,6 @@ if ($_SERVER['REQUEST_METHOD'] === 'GET') { $input_errors = []; $pconfig = $_POST; - /* input validation */ - $reqdfields = explode(" ", "members"); - $reqdfieldsn = [gettext('Member Interfaces')]; - - do_input_validation($pconfig, $reqdfields, $reqdfieldsn, $input_errors); - $not_between_int = function($val, $min, $max) { return !is_numeric($val) || $val < $min || $val > $max; }; @@ -133,8 +130,9 @@ if ($_SERVER['REQUEST_METHOD'] === 'GET') { } } - if (is_array($pconfig['members'])) { - foreach($pconfig['members'] as $ifmembers) { + $members = !empty($pconfig['members']) ? $pconfig['members'] : []; + if (!empty($members)) { + foreach($members as $ifmembers) { if (empty($config['interfaces'][$ifmembers])) { $input_errors[] = gettext("A member interface passed does not exist in configuration"); } @@ -147,7 +145,6 @@ if ($_SERVER['REQUEST_METHOD'] === 'GET') { } } - $members = !empty($pconfig['members']) ? $pconfig['members'] : []; foreach (['stp', 'edge', 'autoedge', 'ptp', 'autoptp', 'static', 'private'] as $section) { if (!empty($pconfig[$section])) { foreach ($pconfig[$section] as $if) { @@ -277,7 +274,7 @@ $(document).ready(function() { $bridge_interfaces = []; foreach ($a_bridges as $idx => $bridge_item) { if (!isset($id) || $idx != $id) { - $bridge_interfaces = array_merge(explode(',', $bridge_item['members']), $bridge_interfaces); + $bridge_interfaces = array_merge(explode(',', $bridge_item['members'] ?? ''), $bridge_interfaces); } } foreach ($ifacelist as $ifn => $ifinfo):