| Message ID | 20240516105612.15306-4-leigh@solinno.co.uk (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add bridge VLAN support | expand |
On Thu, May 16, 2024 at 6:56 AM Leigh Brown <leigh@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 following: > > 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 requires the iproute2 bridge command to be > installed. An error will be generated if the vlan parameter is > set and the bridge command is not available. > > Signed-off-by: Leigh Brown <leigh@solinno.co.uk> > > --- > tools/hotplug/Linux/xen-network-common.sh | 103 ++++++++++++++++++++++ > 1 file changed, 103 insertions(+) > > diff --git a/tools/hotplug/Linux/xen-network-common.sh b/tools/hotplug/Linux/xen-network-common.sh > index 42fa704e8d..fa7615ce0f 100644 > --- a/tools/hotplug/Linux/xen-network-common.sh > +++ b/tools/hotplug/Linux/xen-network-common.sh > +_vif_vlan_setup() { > + # References vlans and dev variable from the calling function > + local vid cmd > + > + bridge vlan del dev "$dev" vid 1 Is vid 1 always added, so you need to remove it before customizing? > + 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" Sorry if I missed this last time, but `eval` shouldn't be necessary. local args case ${vlans[$vid]} in p) args="pvid untagged" ;; u) args="untagged" ;; t) ;; esac bridge vlan add dev "$dev" vid "$vid" $args args unquoted so it expands into maybe two args. You could also make args an array and do "${args[@]}" > + done > +} > + > +_vif_vlan_membership() { > + # The vlans, pvid and dev variables are used by sub-functions > + local -A vlans=() > + local -a terms=() > + local -i i pvid=0 > + local dev=$1 > + > + # 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:]]} Because terms is not in double quotes, wouldn't it expand out? Then any whitespace would be dropped when calling _vif_vlan_parse_term. Your %% only drops trailing spaces too. Maybe you want "${terms//[[:space:]]/}" to remove all spaces? Stylistically, you use (( )) more than I would. I'd do: local term for term in "${terms[@]}" ; do _vif_vlan_parse_term "$term" But you can keep it your way if you prefer. Regards, Jason
Hi Jason, On 2024-05-17 03:19, Jason Andryuk wrote: > On Thu, May 16, 2024 at 6:56 AM Leigh Brown <leigh@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 following: >> >> 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 requires the iproute2 bridge command to be >> installed. An error will be generated if the vlan parameter is >> set and the bridge command is not available. >> >> Signed-off-by: Leigh Brown <leigh@solinno.co.uk> >> >> --- >> tools/hotplug/Linux/xen-network-common.sh | 103 >> ++++++++++++++++++++++ >> 1 file changed, 103 insertions(+) >> >> diff --git a/tools/hotplug/Linux/xen-network-common.sh >> b/tools/hotplug/Linux/xen-network-common.sh >> index 42fa704e8d..fa7615ce0f 100644 >> --- a/tools/hotplug/Linux/xen-network-common.sh >> +++ b/tools/hotplug/Linux/xen-network-common.sh > >> +_vif_vlan_setup() { >> + # References vlans and dev variable from the calling function >> + local vid cmd >> + >> + bridge vlan del dev "$dev" vid 1 > > Is vid 1 always added, so you need to remove it before customizing? Correct. I will add a comment to that effect. >> + 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" > > Sorry if I missed this last time, but `eval` shouldn't be necessary. > > > local args > > case ${vlans[$vid]} in > p) args="pvid untagged" ;; > u) args="untagged" ;; > t) ;; > esac > bridge vlan add dev "$dev" vid "$vid" $args > > args unquoted so it expands into maybe two args. You could also make > args an array and do "${args[@]}" I will make that change, using an array. >> + done >> +} >> + >> +_vif_vlan_membership() { >> + # The vlans, pvid and dev variables are used by sub-functions >> + local -A vlans=() >> + local -a terms=() >> + local -i i pvid=0 >> + local dev=$1 >> + >> + # 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:]]} > > Because terms is not in double quotes, wouldn't it expand out? Then > any whitespace would be dropped when calling _vif_vlan_parse_term. > Your %% only drops trailing spaces too. Maybe you want > "${terms//[[:space:]]/}" to remove all spaces? That is a hack because readarray adds a newline at the end of the last element (not sure why). I will change the code just to fix up the last element before the loop, and it will be clearer. > Stylistically, you use (( )) more than I would. I'd do: > > local term > for term in "${terms[@]}" ; do > _vif_vlan_parse_term "$term" > > But you can keep it your way if you prefer. You guessed correctly that like using (( )) but in this case your suggestion is simpler, so I will make that change. > Regards, > Jason I am making the changes then will test, after which I will send an updated version. Regards, Leigh.
diff --git a/tools/hotplug/Linux/xen-network-common.sh b/tools/hotplug/Linux/xen-network-common.sh index 42fa704e8d..fa7615ce0f 100644 --- a/tools/hotplug/Linux/xen-network-common.sh +++ b/tools/hotplug/Linux/xen-network-common.sh @@ -121,10 +121,105 @@ create_bridge () { fi } +_vif_vlan_add() { + # References vlans and pvid variables from the calling function + local -i vid=$1 + local flag=${2:-} + + if (( vid < 1 || vid > 4094 )) ;then + fatal "vlan id $vid not between 1 and 4094" + fi + if [[ -n "${vlans[$vid]}" ]] ;then + fatal "vlan id $vid specified more than once" + fi + case $flag in + p) if (( pvid != 0 )) ;then + fatal "more than one pvid specified ($vid and $pvid)" + fi + pvid=$vid + vlans[$vid]=p ;; + u) vlans[$vid]=u ;; + *) vlans[$vid]=t ;; + esac +} + +_vif_vlan_parse_term() { + local vid incr last term=${1:-} + + if [[ $term =~ ^([0-9]+)([pu])?$ ]] ;then + _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 + fatal "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 + fatal "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 + fatal "pvid required when using 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 and dev variables are used by sub-functions + local -A vlans=() + local -a terms=() + local -i i pvid=0 + local dev=$1 + + # 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:]]} + done + + _vif_vlan_validate_pvid + _vif_vlan_setup + return 0 +} + # 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 +229,14 @@ add_to_bridge () { else ip link set ${dev} master ${bridge} fi + if [ -n "${vlan}" ] ;then + log debug "configuring vlans for ${dev} on ${bridge}" + if which bridge >&/dev/null; then + _vif_vlan_membership "${dev}" "${vlan}" + else + fatal "vlan configuration failed: bridge command not found" + fi + fi else log debug "$dev already on bridge $bridge" fi
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 following: 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 requires the iproute2 bridge command to be installed. An error will be generated if the vlan parameter is set and the bridge command is not available. Signed-off-by: Leigh Brown <leigh@solinno.co.uk> --- tools/hotplug/Linux/xen-network-common.sh | 103 ++++++++++++++++++++++ 1 file changed, 103 insertions(+)