[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Paul Durrant <paul@xxxxxxx>
  • From: Ian Jackson <ian.jackson@xxxxxxxxxx>
  • Date: Tue, 4 Aug 2020 12:06:41 +0100
  • Authentication-results: esa2.hc3370-68.iphmx.com; dkim=none (message not signed) header.i=none
  • Cc: "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Paul Durrant <pdurrant@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Ian Jackson <Ian.Jackson@xxxxxxxxxx>
  • Delivery-date: Tue, 04 Aug 2020 11:07:01 +0000
  • Ironport-sdr: C7tKztucSK0C5k6Qce20AC1Orb5MB3kNMSAmv/Ia9Kbzm9pqawRt8lYYVS5AXUHs9SnGx4uG4T xXBYxbY3d7UGicccUC3TC0w6rEA1poBepkjIRUU+BH5xIxBzTsEP50lapmIZmNuiMZQ1rp2ofq k6H+ZyS78O6PXOctKKCJOTWX1J/qmH06DjkzQ9malDVTMO/0zZkoIHejP7eIxf+QCPPvlwbNfE VwOvPy0KcdUarie06K+MZnM7pQbOqyMrHbcCW7ca7uvM16QpvxWEWzhxBrQEM20039g7QKc6Vt g04=
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.