| Message ID | 20240508213824.55824-4-leigh@solinno.co.uk (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add bridge VLAN support | expand |
On Wed, May 8, 2024 at 6:55 PM 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 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 <leigh@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
Hi Jason, On 2024-05-15 01:57, Jason Andryuk wrote: > On Wed, May 8, 2024 at 6:55 PM 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 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 <leigh@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. I will do that. >> + 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. Agreed, I will do that. > 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. Great - I would not like to rewrite it in Posix shell! > Regards, > Jason Regards, Leigh.
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 + 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 + _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" + 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 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 <leigh@solinno.co.uk> --- tools/hotplug/Linux/xen-network-common.sh | 111 ++++++++++++++++++++++ 1 file changed, 111 insertions(+)