Re: [Xen-devel] [PATCH 2/2] tools/hotplug, Use udev rules instead of qemu script to setup the bridge.

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

Stefano Stabellini wrote:
> On Wed, 4 Aug 2010, anthony.perard@xxxxxxxxxx wrote:
> > From: Anthony PERARD <anthony.perard@xxxxxxxxxx>
> >
> > 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 <anthony.perard@xxxxxxxxxx>
> > ---
> >  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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel