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

[Anthony PERARD <anthony.perard@citrix.com>]

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