From mboxrd@z Thu Jan 1 00:00:00 1970 From: Anthony PERARD Subject: Re: [PATCH 2/2] tools/hotplug, Use udev rules instead of qemu script to setup the bridge. Date: Fri, 06 Aug 2010 16:49:12 +0100 Message-ID: <4C5C2EF8.1000309@citrix.com> References: <1280938265-12107-1-git-send-email-anthony.perard@citrix.com> <1280938265-12107-3-git-send-email-anthony.perard@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Stefano Stabellini Cc: "xen-devel@lists.xensource.com" List-Id: xen-devel@lists.xenproject.org Stefano Stabellini wrote: > On Wed, 4 Aug 2010, anthony.perard@citrix.com wrote: >> From: Anthony PERARD >> >> This patch adds a second argument to vif-bridge script. It can be "vif" >> or "tap". "vif" give the default behavior and "tap" just add the >> interface to the found bridge when the action is "add". >> >> Signed-off-by: Anthony PERARD >> --- >> tools/hotplug/Linux/vif-bridge | 42 +++++++++++++++++++++++++++----- >> tools/hotplug/Linux/vif-common.sh | 12 ++++++--- >> tools/hotplug/Linux/xen-backend.rules | 5 ++- >> tools/libxl/libxl.c | 4 +- >> 4 files changed, 48 insertions(+), 15 deletions(-) >> >> diff --git a/tools/hotplug/Linux/vif-bridge b/tools/hotplug/Linux/vif-bridge >> index d35144e..63223f3 100644 >> --- a/tools/hotplug/Linux/vif-bridge >> +++ b/tools/hotplug/Linux/vif-bridge >> @@ -29,6 +29,13 @@ >> # rules for its ip addresses (if any). >> #============================================================================ >> >> +# Older versions of Xen do not pass in the type as an argument >> +if [ $# -lt 2 ]; then >> + type_if=vif >> +else >> + type_if=$2 >> +fi >> + >> dir=$(dirname "$0") >> . "$dir/vif-common.sh" >> >> @@ -79,22 +86,43 @@ then >> fatal "Could not find bridge device $bridge" >> fi >> >> +case "$type_if" in >> + vif) >> + dev=$vif >> + ;; >> + tap) >> + dev=$INTERFACE >> + ;; >> +esac >> + >> case "$command" in >> online) >> - setup_bridge_port "$vif" >> - add_to_bridge "$bridge" "$vif" >> + if [ "$type_if" = vif ]; then >> + setup_bridge_port "$vif" >> + add_to_bridge "$bridge" "$vif" >> + fi >> ;; >> >> offline) >> - do_without_error brctl delif "$bridge" "$vif" >> - do_without_error ifconfig "$vif" down >> + if [ "$type_if" = vif ]; then >> + do_without_error brctl delif "$bridge" "$vif" >> + do_without_error ifconfig "$vif" down >> + fi >> + ;; >> + >> + add) >> + if [ "$type_if" = tap ]; then >> + add_to_bridge "$bridge" "$dev" >> + fi >> ;; >> esac > > Can we limit the amount of "if" introduced by this patch somehow? > If we use a different command (add instead of online), why do we need > the "if" under the online and offline cases? > If add is only used with tap devices, why do we need an if under add? The command come directly from the $ACTION in udev, and for some reason, this event is different in both cases. Here, I just check if we really want to setup a vif or a tap. This is too much? I can check the argument before that, in vif-common.sh, like is already done for a vif. >> >> -handle_iptable >> +if [ "$type_if" = vif ]; then >> + handle_iptable >> +fi >> > > why is handle_iptable only called with vifs? Because iptable wasn't handled before with the qemu script and it doesn't seem to be necessary to handle iptable for a tap device. >> -log debug "Successful vif-bridge $command for $vif, bridge $bridge." >> -if [ "$command" == "online" ] >> +log debug "Successful vif-bridge $command for $dev, bridge $bridge." >> +if [ "$type_if" = vif -a "$command" = "online" ] >> then >> success >> fi >> diff --git a/tools/hotplug/Linux/vif-common.sh b/tools/hotplug/Linux/vif-common.sh >> index 44dd342..4cf00c0 100644 >> --- a/tools/hotplug/Linux/vif-common.sh >> +++ b/tools/hotplug/Linux/vif-common.sh >> @@ -33,7 +33,9 @@ fi >> >> case "$command" in >> add | remove) >> - exit 0 >> + if [ "$type_if" != tap ]; then >> + exit 0 >> + fi >> ;; >> esac >> >> @@ -48,9 +50,11 @@ evalVariables "$@" >> ip=${ip:-} >> ip=$(xenstore_read_default "$XENBUS_PATH/ip" "$ip") >> >> -# Check presence of compulsory args. >> -XENBUS_PATH="${XENBUS_PATH:?}" >> -vif="${vif:?}" >> +if [ "$type_if" != tap ]; then >> + # Check presence of compulsory args. >> + XENBUS_PATH="${XENBUS_PATH:?}" >> + vif="${vif:?}" >> +fi > > vifs get the parameter from xenstore, how do tap devices get the > parameter? The interface name is get from $INTERFACE, and the bridge is found automatically by the script. But there must be a way to take the bridge from xenstore, by using the name of the device (tap32.0) to have the path. -- Anthony PERARD