From: Leigh Brown <leigh@solinno.co.uk>
To: Jason Andryuk <jandryuk@gmail.com>
Cc: xen-devel@lists.xenproject.org,
Andrew Cooper <andrew.cooper3@citrix.com>,
Anthony Perard <anthony.perard@citrix.com>
Subject: Re: [PATCH v3 3/4] tools/hotplug/Linux: Add bridge VLAN support
Date: Fri, 17 May 2024 11:38:27 +0100 [thread overview]
Message-ID: <18967904c69aa6d4975d1a80dd825aeb@solinno.co.uk> (raw)
In-Reply-To: <CAKf6xpvX2T3xhx59b0_X3e+9BpYuMZS84P=ghisZFD3yWsFssw@mail.gmail.com>
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.
next prev parent reply other threads:[~2024-05-17 10:38 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-16 10:56 [PATCH v3 0/4] Add bridge VLAN support Leigh Brown
2024-05-16 10:56 ` [PATCH v3 1/4] tools/libs/light: Add vlan field to libxl_device_nic Leigh Brown
2024-05-16 10:56 ` [PATCH v3 2/4] tools/xl: add vlan keyword to vif option Leigh Brown
2024-05-16 10:56 ` [PATCH v3 3/4] tools/hotplug/Linux: Add bridge VLAN support Leigh Brown
2024-05-17 2:19 ` Jason Andryuk
2024-05-17 10:38 ` Leigh Brown [this message]
2024-05-16 10:56 ` [PATCH v3 4/4] docs/misc: Example Linux bridge VLAN config Leigh Brown
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=18967904c69aa6d4975d1a80dd825aeb@solinno.co.uk \
--to=leigh@solinno.co.uk \
--cc=andrew.cooper3@citrix.com \
--cc=anthony.perard@citrix.com \
--cc=jandryuk@gmail.com \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).