[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
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.
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |