diff mbox series

[v2,1/4] tools/hotplug: add remove_from_bridge() and improve debug output

Message ID 20200803124931.2678-2-paul@xen.org (mailing list archive)
State New, archived
Headers show
Series tools: propagate bridge MTU to vif frontends | expand

Commit Message

Paul Durrant Aug. 3, 2020, 12:49 p.m. UTC 
From: Paul Durrant <pdurrant@amazon.com>

This patch adds a remove_from_bridge() function into xen-network-common.sh
to partner with the existing add_to_bridge() function. The code in
add_to_bridge() is also slightly re-arranged to avoid duplication calls of
'ip link'.

Both add_to_bridge() and remove_from_bridge() will check if their bridge
manipulation operations are necessary and emit a log message if they are not.

NOTE: A call to remove_from_bridge() will be added by a subsequent patch.

Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wl@xen.org>
---
 tools/hotplug/Linux/xen-network-common.sh | 37 ++++++++++++++++++-----
 1 file changed, 29 insertions(+), 8 deletions(-)

Comments

Ian Jackson Aug. 4, 2020, 11:06 a.m. UTC | #1 
Paul Durrant writes ("[PATCH v2 1/4] tools/hotplug: add remove_from_bridge() and improve debug output"):
> From: Paul Durrant <pdurrant@amazon.com>
> 
> This patch adds a remove_from_bridge() function into xen-network-common.sh
> to partner with the existing add_to_bridge() function. The code in
> add_to_bridge() is also slightly re-arranged to avoid duplication calls of
> 'ip link'.
> 
> Both add_to_bridge() and remove_from_bridge() will check if their bridge
> manipulation operations are necessary and emit a log message if they are not.
> 
> NOTE: A call to remove_from_bridge() will be added by a subsequent patch.

I think there is another semantic change here which is that now it
executes the "ip link set up" even if the device is already on the
bridge.

I think this is correct, but it probably ought to be mentioned in the
commit message.

I hesitate to suggest this, but: my personal preference would have
been to split that refactoring (in particular, the inversion of the
early exit if approach) into yet another commit.  I find tiny commits
easier to review.  But this commit is already quite small so if you
prefer to keep it this way I think that is fine.

> +remove_from_bridge () {
> +    local bridge=$1
> +    local dev=$2
> +
> +    ip link set dev ${dev} down || :
> +
> +    # Don't remove $dev from $bridge if it's not on the bridge.
> +    if [ -e "/sys/class/net/${bridge}/brif/${dev}" ]; then
> +	log debug "removing $dev from bridge $bridge"
> +	if which brctl >&/dev/null; then
> +            brctl delif ${bridge} ${dev}
> +	else
> +            ip link set ${dev} nomaster
> +	fi
> +    else
> +	log debug "$dev not on bridge $bridge"
> +    fi
> +}

I think this is code motion split into two patches - here the added
code and in 2/, the other copy is removed.  Could you please shuffle
this addition into patch 2 ?

Thanks,
Ian.
diff mbox series

Patch

diff --git a/tools/hotplug/Linux/xen-network-common.sh b/tools/hotplug/Linux/xen-network-common.sh
index 8dd3a62068..37e71cfa9c 100644
--- a/tools/hotplug/Linux/xen-network-common.sh
+++ b/tools/hotplug/Linux/xen-network-common.sh
@@ -126,19 +126,40 @@  add_to_bridge () {
     local bridge=$1
     local dev=$2
 
-    # Don't add $dev to $bridge if it's already on a bridge.
-    if [ -e "/sys/class/net/${bridge}/brif/${dev}" ]; then
-	ip link set dev ${dev} up || true
-	return
-    fi
-    if which brctl >&/dev/null; then
-        brctl addif ${bridge} ${dev}
+    # Don't add $dev to $bridge if it's already on the bridge.
+    if [ ! -e "/sys/class/net/${bridge}/brif/${dev}" ]; then
+	log debug "adding $dev to bridge $bridge"
+	if which brctl >&/dev/null; then
+            brctl addif ${bridge} ${dev}
+	else
+            ip link set ${dev} master ${bridge}
+	fi
     else
-        ip link set ${dev} master ${bridge}
+	log debug "$dev already on bridge $bridge"
     fi
+
     ip link set dev ${dev} up
 }
 
+remove_from_bridge () {
+    local bridge=$1
+    local dev=$2
+
+    ip link set dev ${dev} down || :
+
+    # Don't remove $dev from $bridge if it's not on the bridge.
+    if [ -e "/sys/class/net/${bridge}/brif/${dev}" ]; then
+	log debug "removing $dev from bridge $bridge"
+	if which brctl >&/dev/null; then
+            brctl delif ${bridge} ${dev}
+	else
+            ip link set ${dev} nomaster
+	fi
+    else
+	log debug "$dev not on bridge $bridge"
+    fi
+}
+
 # Usage: set_mtu bridge dev
 set_mtu () {
     local bridge=$1