On Wed, May 8, 2024 at 6:55 PM Leigh Brown <le...@solinno.co.uk> wrote:
>
> Update add_to_bridge shell function to read the vlan parameter
> from xenstore and set the bridge VLAN configuration for the VID.
>
> Add additional helper functions to parse the vlan specification,
> which consists of one or more of the follow:
>
> a) single VLAN (e.g. 10).
> b) contiguous range of VLANs (e.g. 10-15).
> c) discontiguous range with base, increment and count
> (e.g. 100+10x9 which gives VLAN IDs 100, 110, ... 190).
>
> A single VLAN can be suffixed with "p" to indicate the PVID, or
> "u" to indicate untagged. A range of VLANs can be suffixed with
> "u" to indicate untagged. A complex example would be:
>
> vlan=1p/10-15/20-25u
>
> This capability only works when using the iproute2 bridge command,
> so a warning is issued if the vlan parameter is set and the bridge
> command is not available, as it will be ignored.
>
> Signed-off-by: Leigh Brown <le...@solinno.co.uk>
> ---
> tools/hotplug/Linux/xen-network-common.sh | 111 ++++++++++++++++++++++
> 1 file changed, 111 insertions(+)
>
> diff --git a/tools/hotplug/Linux/xen-network-common.sh
> b/tools/hotplug/Linux/xen-network-common.sh
> index 42fa704e8d..d9fb4f7355 100644
> --- a/tools/hotplug/Linux/xen-network-common.sh
> +++ b/tools/hotplug/Linux/xen-network-common.sh
> @@ -121,10 +121,113 @@ create_bridge () {
> fi
> }
>
> +_vif_vlan_add() {
> + # References vlans, pvid and error variables from the calling function
> + local -i vid=$1
> + local flag=${2:-}
> +
> + if (( vid < 1 || vid > 4094 )) ;then
> + error="vlan id $vid not between 1 and 4094"
> + return
> + fi
> + if [[ -n "${vlans[$vid]}" ]] ;then
> + error="vlan id $vid specified more than once"
> + return
You could do `fatal "vlan id $vid specified more than once"` and just
terminate the script at this point. It would simplify your later code
if you use fatal more.
> + fi
> + case $flag in
> + p) if (( pvid != 0 )) ;then
> + error="more than one pvid specified ($vid and $pvid)"
> + return
> + fi
> + pvid=$vid
> + vlans[$vid]=p ;;
> + u) vlans[$vid]=u ;;
> + *) vlans[$vid]=t ;;
> + esac
> +}
> +
> +_vif_vlan_parse_term() {
> + # References error variable from the calling function
> + local vid incr last term=${1:-}
> +
> + if [[ $term =~ ^([0-9]+)([pu])?$ ]] ;then
I like that you split the different cases into multiple REs.
> + _vif_vlan_add ${BASH_REMATCH[1]} ${BASH_REMATCH[2]}
> + elif [[ $term =~ ^([0-9]+)-([0-9]+)(u)?$ ]] ;then
> + vid=${BASH_REMATCH[1]}
> + last=${BASH_REMATCH[2]}
> + if (( last >= vid )) ;then
> + for (( ; vid<=last; vid++ )) ;do
> + _vif_vlan_add $vid ${BASH_REMATCH[3]}
> + done
> + else
> + error="invalid vlan id range: $term"
> + fi
> + elif [[ $term =~ ^([0-9]+)\+([0-9]+)x([0-9]+)(u)?$ ]] ;then
> + vid=${BASH_REMATCH[1]}
> + incr=${BASH_REMATCH[2]}
> + for (( j=${BASH_REMATCH[3]}; j>0; --j, vid+=incr ))
> + do
> + _vif_vlan_add $vid ${BASH_REMATCH[4]}
> + done
> + else
> + error="invalid vlan specification: $term"
> + fi
> +}
> +
> +_vif_vlan_validate_pvid() {
> + # References vlans and pvid variables from the calling function
> + if (( pvid == 0 )) ;then
> + if (( ${#vlans[@]} == 1 )) ;then
> + vlans[${!vlans[*]}]=p
> + else
> + error="pvid required for multiple vlan ids"
> + fi
> + fi
> +}
> +
> +_vif_vlan_setup() {
> + # References vlans and dev variable from the calling function
> + local vid cmd
> +
> + bridge vlan del dev "$dev" vid 1
> + for vid in ${!vlans[@]} ;do
> + cmd="bridge vlan add dev '$dev' vid $vid"
> + case ${vlans[$vid]} in
> + p) cmd="$cmd pvid untagged" ;;
> + u) cmd="$cmd untagged" ;;
> + t) ;;
> + esac
> + eval "$cmd"
> + done
> +}
> +
> +_vif_vlan_membership() {
> + # The vlans, pvid, dev and error variables are used by sub-functions
> + local -A vlans=()
> + local -a terms=()
> + local -i i pvid=0
> + local dev=$1 error=""
> +
> + # Split the vlan specification string into its terms
> + readarray -d / -t terms <<<$2
> + for (( i=0; i<${#terms[@]}; ++i )) ;do
> + _vif_vlan_parse_term ${terms[$i]%%[[:space:]]}
> + [[ -n "$error" ]] && break
> + done
> +
> + [[ -z "$error" ]] && _vif_vlan_validate_pvid
> + [[ -z "$error" ]] && _vif_vlan_setup
> + [[ -z "$error" ]] && return 0
> +
> + log error "$error"
> + return 1
> +}
> +
> # Usage: add_to_bridge bridge dev
> add_to_bridge () {
> local bridge=$1
> local dev=$2
> + local vlan=$(xenstore_read_default "$XENBUS_PATH/vlan" "")
>
> # Don't add $dev to $bridge if it's already on the bridge.
> if [ ! -e "/sys/class/net/${bridge}/brif/${dev}" ]; then
> @@ -134,6 +237,14 @@ add_to_bridge () {
> else
> ip link set ${dev} master ${bridge}
> fi
> + if [ -n "${vlan}" ] ;then
> + if which bridge >&/dev/null; then
> + log debug "configuring VLANs for ${dev} on ${bridge}"
> + _vif_vlan_membership "${dev}" "${vlan}"
> + else
> + log warning "bridge command not available, ignoring vlan
> config"
Do you think this should be fatal? It seems to me that setting up the
connection but not applying the vlans could be a security issue.
This file before your patch was very close to posix sh. Afterwards,
it definitely needs bash. vif-bridge is /bin/bash, so it is fine.
Regards,
Jason
