Re: [PATCH v3 3/4] tools/hotplug/Linux: Add bridge VLAN support

[Jason Andryuk <jandryuk@xxxxxxxxx>]

On Thu, May 16, 2024 at 6:56 AM Leigh Brown <leigh@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
>
> ---
>  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