On Thu, May 16, 2024 at 6:56 AM 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 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 <le...@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
