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

[Ian Jackson <ian.jackson@citrix.com>]

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.