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

[Leigh Brown <leigh@xxxxxxxxxxxxx>]

Hi Jason,

On 2024-05-17 03:19, Jason Andryuk wrote:
> 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?
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.