Paul Durrant writes ("[PATCH v2 1/4] tools/hotplug: add remove_from_bridge()
and improve debug output"):
> From: Paul Durrant <pdurr...@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.
