| Message ID | 35b942031521f25b63e60634ee86f1b52f504eb3.1576631444.git.netwiz@crc.id.au (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [tools/hotplug] Use ip on systems where brctl is not available | expand |
On Wed, Dec 18, 2019 at 12:15:23PM +1100, Steven Haigh wrote: > Newer distros like CentOS 8 do not have brctl available. As such, we > can't use it to configure networking anymore. > > This patch will fall back to 'ip' or 'bridge' commands if brctl is not > available in the working PATH. > > This would be a likely backport candidate to any version expected to be > built on CentOS 8 etc. Acked-by: Wei Liu <wl@xen.org>
Steven Haigh writes ("[PATCH] [tools/hotplug] Use ip on systems where brctl is not available"):
> Newer distros like CentOS 8 do not have brctl available. As such, we
> can't use it to configure networking anymore.
>
> This patch will fall back to 'ip' or 'bridge' commands if brctl is not
> available in the working PATH.
This looks good to me at least in the brctl case. I have two minor
comments.
For the avoidance of doubt, I guess you have tested this in the
`ip'/`bridge' case ? How thoroughly ? :-)
> -if [ -z "$bridge" ]
> -then
> - bridge=$(brctl show | awk 'NR==2{print$1}')
> -
> +if [ -z "$bridge" ]; then
The presumably-unintentional style change makes the review slightly
harder...
> - bridge=$(brctl show | cut -d "
> + if which brctl >&/dev/null; then
Maybe introduce
have_brctl () { ... }
so we can say
if have_brctl; then
?
Regards,
Ian.
On 2019-12-19 02:42, Ian Jackson wrote: > Steven Haigh writes ("[PATCH] [tools/hotplug] Use ip on systems where > brctl is not available"): >> Newer distros like CentOS 8 do not have brctl available. As such, we >> can't use it to configure networking anymore. >> >> This patch will fall back to 'ip' or 'bridge' commands if brctl is not >> available in the working PATH. > > This looks good to me at least in the brctl case. I have two minor > comments. > > For the avoidance of doubt, I guess you have tested this in the > `ip'/`bridge' case ? How thoroughly ? :-) I have tested it to the point that it's almost a port of the Fedora patch - however the Fedora patch removes brctl completely in favour of the ip / bridge commands. While I haven't specifically debugged the result on Fedora, the networking works successfully when running a Domain-0 in Fedora 31 - which was the source of the 'ip' commands to run. > >> -if [ -z "$bridge" ] >> -then >> - bridge=$(brctl show | awk 'NR==2{print$1}') >> - >> +if [ -z "$bridge" ]; then > > The presumably-unintentional style change makes the review slightly > harder... I'm intending to submit a new patch series after this (to make backporting this easier) that cleans up formatting / whitespace / syntax across the majority of scripts in the Linux directory. It'll look like a hot mess when submitting the next lot of patches - but its better than nothing. >> - bridge=$(brctl show | cut -d " >> + if which brctl >&/dev/null; then > > Maybe introduce > have_brctl () { ... } > so we can say > if have_brctl; then > ? I don't really have a preference. brctl is used through quite a few scripts - none of which really have a standard method of operation or common presentation. Some scripts call xen-network-common.sh - some do not. Would I be correct in thinking that your proposal would be to ensure all network scripts source xen-network-common.sh - but this would be a more invasive change for backporting - hence I've tried to keep it as simple as possible for now. Would a restructure of these things be better for something to be committed as yet another patch set (after formatting/style cleanups) that makes things a little more consistent?
diff --git a/tools/hotplug/Linux/colo-proxy-setup b/tools/hotplug/Linux/colo-proxy-setup index 94e2034452..d709146c47 100755 --- a/tools/hotplug/Linux/colo-proxy-setup +++ b/tools/hotplug/Linux/colo-proxy-setup @@ -76,10 +76,17 @@ function teardown_primary() function setup_secondary() { - do_without_error brctl delif $bridge $vifname - do_without_error brctl addbr $forwardbr - do_without_error brctl addif $forwardbr $vifname - do_without_error brctl addif $forwardbr $forwarddev + if which brctl >&/dev/null; then + do_without_error brctl delif $bridge $vifname + do_without_error brctl addbr $forwardbr + do_without_error brctl addif $forwardbr $vifname + do_without_error brctl addif $forwardbr $forwarddev + else + do_without_error ip link set $vifname nomaster + do_without_error ip link add name $forwardbr type bridge + do_without_error ip link set $vifname master $forwardbr + do_without_error ip link set $forwarddev master $forwardbr + fi do_without_error ip link set dev $forwardbr up do_without_error modprobe xt_SECCOLO @@ -91,10 +98,17 @@ function setup_secondary() function teardown_secondary() { - do_without_error brctl delif $forwardbr $forwarddev - do_without_error brctl delif $forwardbr $vifname - do_without_error brctl delbr $forwardbr - do_without_error brctl addif $bridge $vifname + if which brctl >&/dev/null; then + do_without_error brctl delif $forwardbr $forwarddev + do_without_error brctl delif $forwardbr $vifname + do_without_error brctl delbr $forwardbr + do_without_error brctl addif $bridge $vifname + else + do_without_error ip link set $forwarddev nomaster + do_without_error ip link set $vifname nomaster + do_without_error ip link delete $forwardbr type bridge + do_without_error ip link set $vifname master $bridge + fi do_without_error iptables -t mangle -D PREROUTING -m physdev --physdev-in \ $vifname -j SECCOLO --index $index diff --git a/tools/hotplug/Linux/vif-bridge b/tools/hotplug/Linux/vif-bridge index 6956dea66a..e722090ca8 100644 --- a/tools/hotplug/Linux/vif-bridge +++ b/tools/hotplug/Linux/vif-bridge @@ -31,10 +31,12 @@ dir=$(dirname "$0") bridge=${bridge:-} bridge=$(xenstore_read_default "$XENBUS_PATH/bridge" "$bridge") -if [ -z "$bridge" ] -then - bridge=$(brctl show | awk 'NR==2{print$1}') - +if [ -z "$bridge" ]; then + if which brctl >&/dev/null; then + bridge=$(brctl show | awk 'NR==2{print$1}') + else + bridge=$(bridge link | cut -d" " -f7) + fi if [ -z "$bridge" ] then fatal "Could not find bridge, and none was specified" @@ -82,7 +84,11 @@ case "$command" in ;; offline) - do_without_error brctl delif "$bridge" "$dev" + if which brctl >&/dev/null; then + do_without_error brctl delif "$bridge" "$dev" + else + do_without_error ip link set "$dev" nomaster + fi do_without_error ifconfig "$dev" down ;; diff --git a/tools/hotplug/Linux/vif2 b/tools/hotplug/Linux/vif2 index 2c155be68c..5bd555c6f0 100644 --- a/tools/hotplug/Linux/vif2 +++ b/tools/hotplug/Linux/vif2 @@ -7,13 +7,21 @@ dir=$(dirname "$0") bridge=$(xenstore_read_default "$XENBUS_PATH/bridge" "$bridge") if [ -z "$bridge" ] then - nr_bridges=$(($(brctl show | cut -f 1 | grep -v "^$" | wc -l) - 1)) + if which brctl >&/dev/null; then + nr_bridges=$(($(brctl show | cut -f 1 | grep -v "^$" | wc -l) - 1)) + else + nr_bridges=$(bridge link | wc -l) + fi if [ "$nr_bridges" != 1 ] then fatal "no bridge specified, and don't know which one to use ($nr_bridges found)" fi - bridge=$(brctl show | cut -d " + if which brctl >&/dev/null; then + bridge=$(brctl show | cut -d " " -f 2 | cut -f 1) + else + bridge=$(bridge link | cut -d" " -f6) + fi fi command="$1" diff --git a/tools/hotplug/Linux/xen-network-common.sh b/tools/hotplug/Linux/xen-network-common.sh index 92ffa603f7..8dd3a62068 100644 --- a/tools/hotplug/Linux/xen-network-common.sh +++ b/tools/hotplug/Linux/xen-network-common.sh @@ -111,9 +111,13 @@ create_bridge () { # Don't create the bridge if it already exists. if [ ! -e "/sys/class/net/${bridge}/bridge" ]; then - brctl addbr ${bridge} - brctl stp ${bridge} off - brctl setfd ${bridge} 0 + if which brctl >&/dev/null; then + brctl addbr ${bridge} + brctl stp ${bridge} off + brctl setfd ${bridge} 0 + else + ip link add name ${bridge} type bridge stp_state 0 forward_delay 0 + fi fi } @@ -127,7 +131,11 @@ add_to_bridge () { ip link set dev ${dev} up || true return fi - brctl addif ${bridge} ${dev} + if which brctl >&/dev/null; then + brctl addif ${bridge} ${dev} + else + ip link set ${dev} master ${bridge} + fi ip link set dev ${dev} up }